Last Comment Bug 429023 - System-wide myspell dictionaries are ignored
: System-wide myspell dictionaries are ignored
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Spelling checker (show other bugs)
: Trunk
: x86 Linux
: -- normal with 1 vote (vote)
: mozilla23
Assigned To: Ian Stakenvicius
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-04-14 14:39 PDT by Rimas Kudelis
Modified: 2013-04-22 19:41 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Allow system dictionaries (3.34 KB, patch)
2011-05-16 07:40 PDT, Jory A. Pratt
no flags Details | Diff | Splinter Review
Allow system dictionaries V1.1 (3.05 KB, patch)
2011-09-30 13:36 PDT, Jory A. Pratt
mh+mozilla: review-
Details | Diff | Splinter Review
new, generic, 'prefs' based version (1.36 KB, patch)
2013-04-19 13:29 PDT, Ian Stakenvicius
mh+mozilla: review-
anarchy: feedback-
Details | Diff | Splinter Review
generic prefs based dictionary path , try2 (1.65 KB, patch)
2013-04-20 08:36 PDT, Ian Stakenvicius
axs: review-
axs: review-
ehsan: review-
axs: feedback-
Details | Diff | Splinter Review
generic prefs based dictionary path, try2.1 (1.65 KB, patch)
2013-04-20 10:58 PDT, Ian Stakenvicius
ehsan: review+
anarchy: feedback+
Details | Diff | Splinter Review
generic prefs based dictionary path, try2.1, nit-free (1.77 KB, patch)
2013-04-20 16:18 PDT, Ian Stakenvicius
axs: review+
mh+mozilla: review+
Details | Diff | Splinter Review
generic prefs based dictionary path, final (1.77 KB, patch)
2013-04-22 06:06 PDT, Jory A. Pratt
anarchy: review+
anarchy: feedback+
Details | Diff | Splinter Review

Description Rimas Kudelis 2008-04-14 14:39:49 PDT
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) 2008-04-15 10:35:03 PDT
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 2008-04-15 13:19:20 PDT
(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 2011-05-16 07:39:44 PDT
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 2011-05-16 07:40:36 PDT
Created attachment 532629 [details] [diff] [review]
Allow system dictionaries
Comment 5 Rimas Kudelis 2011-05-16 07:48:38 PDT
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] (way behind * queues, especially ni? queue) 2011-05-16 07:52:54 PDT
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 2011-05-16 08:04:49 PDT
(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 :Ehsan Akhgari 2011-09-30 07:45:24 PDT
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 2011-09-30 13:36:27 PDT
Created attachment 563829 [details] [diff] [review]
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 :Ehsan Akhgari 2011-09-30 15:29:55 PDT
So, does this version of the patch work correctly?  Should it be reviewed?
Comment 11 Jory A. Pratt 2011-09-30 17:34:46 PDT
(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] 2011-09-30 23:01:16 PDT
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 :Ehsan Akhgari 2012-12-03 10:41:17 PST
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 2012-12-03 10:49:52 PST
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 :Ehsan Akhgari 2012-12-03 10:54:12 PST
(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 2013-04-19 13:29:06 PDT
Created attachment 739782 [details] [diff] [review]
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 2013-04-19 16:24:13 PDT
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] 2013-04-19 23:35:56 PDT
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 2013-04-20 08:36:24 PDT
Created attachment 739976 [details] [diff] [review]
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 :Ehsan Akhgari 2013-04-20 09:47:27 PDT
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 2013-04-20 10:23:04 PDT
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 2013-04-20 10:58:49 PDT
Created attachment 740000 [details] [diff] [review]
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 :Ehsan Akhgari 2013-04-20 11:04:42 PDT
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 2013-04-20 11:59:37 PDT
(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 2013-04-20 14:50:13 PDT
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 2013-04-20 16:18:14 PDT
Created attachment 740035 [details] [diff] [review]
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] 2013-04-21 22:54:28 PDT
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 2013-04-22 06:06:58 PDT
Created attachment 740258 [details] [diff] [review]
generic prefs based dictionary path, final

Final version,  nit for whitespace addressed.
Comment 29 Benoit Jacob [:bjacob] (mostly away) 2013-04-22 06:33:38 PDT
_AxS_ asked for this to be assigned to him.
Comment 32 Landry Breuil (:gaston) 2013-04-22 12:31:14 PDT
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) 2013-04-22 19:19:28 PDT
https://hg.mozilla.org/mozilla-central/rev/dfe59fafcaf0
Comment 34 :Ehsan Akhgari 2013-04-22 19:41:13 PDT
(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!

Note You need to log in before you can comment on or make changes to this bug.