429023 - System-wide myspell dictionaries are ignored

Status: RESOLVED FIXED

(Core :: Spelling checker, defect)

Description

[Rimas Kudelis [use needinfo to ping]]

Firefox 2 used to present quite a bunch o language choices when right-clicking and input field. Firefox 3 doesn't. I guess it's because Fx3 simply ignores myspell dictionaries.

I think it would be good present those dictionaries when available.

Comment 1

[Ria Klaassen (not reading all bugmail)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008041503 Minefield/3.0pre

My Dutch dictionary still works here, so this is WFM. Are you sure they are enabled in your add-ons list?

Comment 2

[Rimas Kudelis [use needinfo to ping]]

(In reply to comment #1)
No, because they are not add-ons. These dictionaries are installed into a system-wide location, here's an example:
rq@sugar:~ $ dpkg -L myspell-en-us
/.
/usr
/usr/share
/usr/share/myspell
/usr/share/myspell/dicts
/usr/share/myspell/dicts/en_US.aff
/usr/share/myspell/dicts/en_US.dic
/usr/share/myspell/infos
/usr/share/myspell/infos/ooo
/usr/share/myspell/infos/ooo/myspell-en-us
/usr/share/doc
/usr/share/doc/myspell-en-us
/usr/share/doc/myspell-en-us/copyright
/usr/share/doc/myspell-en-us/changelog.Debian.gz
/usr/share/myspell/dicts/en-US.dic
/usr/share/myspell/dicts/en-US.aff
rq@sugar:~ $ 

As you can see, myspell dictionaries are put to /usr/share/myspell/dicts/ in this case, so myspell know where to find them. The problem as I see it is that Firefox doesn't check that location.

BTW, it seems like hunspell dictionaries are installed into that same location, at least in Ubuntu: http://packages.ubuntu.com/gutsy/all/hunspell-de-de/filelist .

Comment 3

[Jory A. Pratt [:anarchy]]

I will attach a patch that I am currently using in gentoo, this is not an official patch, this does need modifications. I am posting it here to get the bug on track. Libreoffice and OpenOffice can both make use of system myspell dictionaries should be no reason we can not get mozilla products to the same point.

Comment 4

[Jory A. Pratt [:anarchy]]

Attachment: Allow system dictionaries

Comment 5

[Rimas Kudelis [use needinfo to ping]]

Jory, your patch hardcodes /usr/share/myspell. Is it indeed the only possible location where these dictionaries can live in all distros?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

I think we should allow using system dictionaries.
But perhaps using a pref would be better than #ifdef.

Also, should we support the case when OS doesn't provide some
dictionary, but browser does. Should the language be supported?


Rimas, the patch is just a WIP "need modifications".

Comment 7

[Jory A. Pratt [:anarchy]]

(In reply to comment #6)
> I think we should allow using system dictionaries.
> But perhaps using a pref would be better than #ifdef.
> 
> Also, should we support the case when OS doesn't provide some
> dictionary, but browser does. Should the language be supported?
> 
> 
> Rimas, the patch is just a WIP "need modifications".

Addon dictionaries are not effected, this just allows those that built with system-hunspell to use system dictionaries first.

Comment 8

[(no longer active)]

Anarchy pinged me on IRC yesterday asking why he sees http://pastebin.mozilla.org/1339719 when using this patch.  I can't say for sure because the patch in this bug can no longer be applied cleanly, and I need to see they patch that you're trying to try to figure out why this happens.

Comment 9

[Jory A. Pratt [:anarchy]]

Attachment: Allow system dictionaries V1.1

Error was due to me being tired and adding code in wrong spot of the Makefile all is fixed in V2

Comment 10

[(no longer active)]

So, does this version of the patch work correctly?  Should it be reviewed?

Comment 11

[Jory A. Pratt [:anarchy]]

(In reply to Ehsan Akhgari [:ehsan] from comment #10)
> So, does this version of the patch work correctly?  Should it be reviewed?

I am not sure if we still want to try to handle this as a pref or not, if not we atleast need to make it so the path to dictionaries can be passed via configure when using system-hunspell. I will leave that up to you all to decide. If you decided this would work for most distros and users I would recommend a review.

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 563829 [details] [diff] [review]
Allow system dictionaries V1.1

Review of attachment 563829 [details] [diff] [review]:
-----------------------------------------------------------------

Consider the build bits to be a review, the code bits are just my opinion.

::: extensions/spellcheck/hunspell/src/mozHunspell.cpp
@@ +322,5 @@
>  
>    nsCOMPtr<nsIFile> dictDir;
> +  #ifdef MOZ_NATIVE_HUNSPELL
> +    nsCOMPtr<nsILocalFile>  localFile;
> +    rv = NS_NewNativeLocalFile(nsDependentCString("/usr/share/myspell"),PR_TRUE, getter_AddRefs(localFile));

I'd make that a configure-able directory which defaults to /usr/share/myspell. I don't think there is a need for a pref. This is also the wrong way to do that, you're entirely replacing all dictionaries, even those users could install from extensions.

Also note that it /may/ be interesting to do that unconditionally, so that plain Firefox would also benefit from system installed dictionaries (and there could very well be, for libreoffice.org)

::: extensions/spellcheck/Makefile.in
@@ +46,4 @@
>  DIRS		= idl locales hunspell src
> +else
> +DIRS		= idl src
> +endif

I understand why you don't want locales, I don't understand why you go through so many hoops to build mozHunspell and mozHunspellDirProfider, when the current rules just do that...

Comment 13

[(no longer active)]

I think it probbaly makes sense for this to be behind a runtime pref, so that you don't have to rebuild to opt in/out of this.  That way distros can easily turn this on or off if they want.

Comment 14

[Rimas Kudelis [use needinfo to ping]]

I wonder why distos would want to turn that off. Or why a user would want to opt out of this. Does anyone have any usage scenarios in mind? IMO, this could be just a build-time config option. I personally see no need to make it a preference...

Comment 15

[(no longer active)]

(In reply to comment #14)
> I wonder why distos would want to turn that off. Or why a user would want to
> opt out of this. Does anyone have any usage scenarios in mind? IMO, this could
> be just a build-time config option. I personally see no need to make it a
> preference...

In general preferences are easier to maintain that build time options are.  At least they guarantee that the code always builds. :-)

That said, I'm not quite sure why a distro would want to disable this pref, but I think it's going to be easier for them this way...  (not a strong argument, I know!)

Comment 16

[Ian Stakenvicius]

Attachment: new, generic, 'prefs' based version

New version; if acceptable will obsolete the old one.  

This is based more on the patch that is currently used in the *BSDs, as provided to me initially by Landry Breuil.  It applies after the mozilla-installed dictionaries rather than instead-of, and works based on a preference 'spellchecker.externaldictionarypath' being set or not.

I don't know if a stub undefined preference should be set for this by default or if the preference should be left unset.  I am leaning towards leaving it unset and leaving it up to the distros (or users) themselves to set it in their own prefs files if they want it.  I suppose this will require documentation somewhere but I have no idea where at the moment.

Comment 17

[Jory A. Pratt [:anarchy]]

Comment on attachment 739782 [details] [diff] [review]
new, generic, 'prefs' based version

Well the patch does work, We do not want to load system dictionary and then builtin dictionary, this will only confuse the user why we have two dictionaries installed.

Comment 18

[Mike Hommey [:glandium]]

Comment on attachment 739782 [details] [diff] [review]
new, generic, 'prefs' based version

Review of attachment 739782 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozilla-release/extensions/spellcheck/hunspell/src/mozHunspell.cpp
@@ +402,4 @@
>      }
>    }
>  
> +  // find and load external dictionary if spellchecker.externaldictionarypath is set in prefs  

trailing whitespace

@@ +405,5 @@
> +  // find and load external dictionary if spellchecker.externaldictionarypath is set in prefs  
> +  nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID));
> +  if (prefs) {
> +    nsCString extDictPath;
> +    rv = prefs->GetCharPref("spellchecker.externaldictionarypath", getter_Copies(extDictPath));

spellchecker.dictionary_path, maybe

@@ +412,5 @@
> +      if (dictDir && NS_SUCCEEDED(rv)) {
> +        LoadDictionariesFromDir(dictDir);
> +      }
> +    }
> +  }

I agree with Jory, that shouldn't fall through in case the pref is set and no dictionary can be found there.

Comment 19

[Ian Stakenvicius]

Attachment: generic prefs based dictionary path , try2

(In reply to Mike Hommey [:glandium] from comment #18)
> 
> spellchecker.dictionary_path, maybe
> 

Friendly, done.

> 
> I agree with Jory, that shouldn't fall through in case the pref is set and
> no dictionary can be found there.

I took Jory's comments more about the fact that both the prefs dictionaries and the internal ones would load, and that is undesirable.  I'm not sure that a prefs-specified dir failing to load should mean the internal ones aren't used as a fallback.  However, I coded this update to do that anyways.  The specific line that specifies whether the internal dicts will always fallback or will only fall back if a pref isn't set, is:

+  if (!(extDictPath && NS_SUCCEEDED(rv))) { // .....


Aside from this, the load-internals-only-on-fallback is a departure from what *BSDs and possibly others are using.  I think it's important to check with them and make sure this is going to cover their use-case.

Comment 20

[(no longer active)]

Comment on attachment 739976 [details] [diff] [review]
generic prefs based dictionary path , try2

Review of attachment 739976 [details] [diff] [review]:
-----------------------------------------------------------------

I think that we do want to fall back to the builtin dictionaries so that if you set the pref to a non-existing directory for example, you don't break the spell checker completely.

::: mozilla-release/extensions/spellcheck/hunspell/src/mozHunspell.cpp
@@ +379,5 @@
>    nsCOMPtr<nsIFile> dictDir;
> +
> +  // check preferences first
> +  nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID));
> +  if (prefs) { 

Nit: trailing space.

@@ +386,5 @@
> +    if (NS_SUCCEEDED(rv)) { // set the spellchecker.dictionary_path
> +      rv = NS_NewNativeLocalFile(extDictPath, true, getter_AddRefs(dictDir));
> +    }
> +  }
> +  if (!(extDictPath && NS_SUCCEEDED(rv))) { // spellcheck.dictionary_path unset, set internal path

Does this even compile?  extDictPath should be out of scope here.

I think you should just do:

if (!dictDir) {

Comment 21

[Ian Stakenvicius]

Comment on attachment 739976 [details] [diff] [review]
generic prefs based dictionary path , try2

Review of attachment 739976 [details] [diff] [review]:
-----------------------------------------------------------------

It doesn't; this patch is full of fail.  The patch didn't actually integrate into my test build which is why I thought it passed.  Apologies, i'll obsolete and redo.

Comment 22

[Ian Stakenvicius]

Attachment: generic prefs based dictionary path, try2.1

This one actually applies.

I considered using just 'if (!dictDir)' for the conditional but given that all other conditionals rely on NS_SUCCEEDED(rv) I don't know if I feel comfortable with that.

Runtime tests with this patch show it behaves according to what Mike specified in his comments:

spellchecker.dictionary_path unset:
     internal dictionaries loaded

spellchecker.dictionary_path set to valid path:
     dictionaries loaded from that path

spellchecker.dictionary_path set to invald path:
     no dictionaries loaded unless they are addons

...I had originally thought along the same lines as Ehsan, in terms of the fallback, but with the behaviour as-is it would allow an end-user to keep the internal dictionaries from loading if all they wanted was their addon ones to show up, and there might be value in that.

Still hoping for feedback/review from *BSD folks about the patch's behaviour in general.

Comment 23

[(no longer active)]

Comment on attachment 740000 [details] [diff] [review]
generic prefs based dictionary path, try2.1

Review of attachment 740000 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozilla-release/extensions/spellcheck/hunspell/src/mozHunspell.cpp
@@ +386,5 @@
> +    if (NS_SUCCEEDED(rv)) { // set the spellchecker.dictionary_path
> +      rv = NS_NewNativeLocalFile(extDictPath, true, getter_AddRefs(dictDir));
> +    }
> +  }
> +  if (!(dictDir && NS_SUCCEEDED(rv))) { // spellcheck.dictionary_path not found, set internal path

Nit: if (!dictDir)
Nit 2: please move the comment down to the line below.

@@ +392,1 @@
>                     NS_GET_IID(nsIFile), getter_AddRefs(dictDir));

Nit: indentation.

@@ +392,3 @@
>                     NS_GET_IID(nsIFile), getter_AddRefs(dictDir));
> +  }
> +  if (dictDir && NS_SUCCEEDED(rv)) {

No need to check rv here.

Comment 24

[Ian Stakenvicius]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #23)
> Review of attachment 740000 [details] [diff] [review]:
> 
> Nit: if (!dictDir)
> Nit 2: please move the comment down to the line below.
> Nit[ 3]: indentation.
> [Nit 4:] No need to check rv here.

Noted, will update patch after others have reviewed.  I've tested dropping NS_SUCCEEDED(rv) and runtime behaviour is the same.

Comment 25

[Jory A. Pratt [:anarchy]]

Comment on attachment 740000 [details] [diff] [review]
generic prefs based dictionary path, try2.1

Review of attachment 740000 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the update, I am fine with this for all.

Comment 26

[Ian Stakenvicius]

Attachment: generic prefs based dictionary path, try2.1, nit-free

Update addressing Ehsan's nits; carrying forward the review+

Also grabbed via hg mq against m-c for quicker landing.

Comment 27

[Mike Hommey [:glandium]]

Comment on attachment 740035 [details] [diff] [review]
generic prefs based dictionary path, try2.1, nit-free

Review of attachment 740035 [details] [diff] [review]:
-----------------------------------------------------------------

::: extensions/spellcheck/hunspell/src/mozHunspell.cpp
@@ +382,5 @@
> +  nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID));
> +  if (prefs) {
> +    nsCString extDictPath;
> +    rv = prefs->GetCharPref("spellchecker.dictionary_path", getter_Copies(extDictPath));
> +    if (NS_SUCCEEDED(rv)) { 

trailing whitespace

@@ +387,5 @@
> +      // set the spellchecker.dictionary_path
> +      rv = NS_NewNativeLocalFile(extDictPath, true, getter_AddRefs(dictDir));
> +    }
> +  }
> +  if (!dictDir) { 

trailing whitespace

Comment 28

[Jory A. Pratt [:anarchy]]

Attachment: generic prefs based dictionary path, final

Final version,  nit for whitespace addressed.

Comment 29

[Benoit Jacob [:bjacob] (mostly away)]

_AxS_ asked for this to be assigned to him.

Comment 30

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=1c9216f942fb

Comment 31

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dfe59fafcaf0

Comment 32

[Landry Breuil (:gaston)]

Sorry for being so long at replying on this - that looks good to me, and will allow me to replace a mozHunspell.cpp patch by a prefs.js patch...

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dfe59fafcaf0

Comment 34

[(no longer active)]

(In reply to comment #32)
> Sorry for being so long at replying on this - that looks good to me, and will
> allow me to replace a mozHunspell.cpp patch by a prefs.js patch...

Shoot! Apologies from me for forgetting that we were still waiting for your feedback, and glad to hear that you like this patch!