System-wide myspell dictionaries are ignored

RESOLVED FIXED in mozilla23

Status

()

Core
Spelling checker
RESOLVED FIXED
10 years ago
4 years ago

People

(Reporter: Rimas Kudelis, Assigned: Ian Stakenvicius)

Tracking

Trunk
mozilla23
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

10 years ago
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.
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?
(Reporter)

Comment 2

10 years ago
(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 .

Updated

9 years ago
Assignee: mscott → nobody

Comment 3

6 years ago
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

6 years ago
Created attachment 532629 [details] [diff] [review]
Allow system dictionaries
(Reporter)

Comment 5

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

Comment 6

6 years ago
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

6 years ago
(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.
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

6 years ago
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
Attachment #532629 - Attachment is obsolete: true
So, does this version of the patch work correctly?  Should it be reviewed?

Comment 11

6 years ago
(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 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...
Attachment #563829 - Flags: review-
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.
(Reporter)

Comment 14

5 years ago
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 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!)
(Assignee)

Comment 16

4 years ago
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.
Attachment #739782 - Flags: review?(mh+mozilla)
Attachment #739782 - Flags: review?(ehsan)
Attachment #739782 - Flags: feedback?(landry)
Attachment #739782 - Flags: feedback?(anarchy)

Comment 17

4 years ago
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.
Attachment #739782 - Flags: feedback?(anarchy) → feedback-
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.
Attachment #739782 - Flags: review?(mh+mozilla) → review-
(Assignee)

Comment 19

4 years ago
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.
Attachment #739782 - Attachment is obsolete: true
Attachment #739782 - Flags: review?(ehsan)
Attachment #739782 - Flags: feedback?(landry)
Attachment #739976 - Flags: review?(mh+mozilla)
Attachment #739976 - Flags: review?(landry)
Attachment #739976 - Flags: review?(ehsan)
Attachment #739976 - Flags: feedback?(anarchy)
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) {
Attachment #739976 - Flags: review?(ehsan) → review-
(Assignee)

Comment 21

4 years ago
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.
Attachment #739976 - Flags: review?(mh+mozilla)
Attachment #739976 - Flags: review?(landry)
Attachment #739976 - Flags: review-
Attachment #739976 - Flags: feedback?(anarchy)
Attachment #739976 - Flags: feedback-
(Assignee)

Comment 22

4 years ago
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.
Attachment #739976 - Attachment is obsolete: true
Attachment #740000 - Flags: review?(mh+mozilla)
Attachment #740000 - Flags: review?(landry)
Attachment #740000 - Flags: review?(ehsan)
Attachment #740000 - Flags: feedback?(mozilla)
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.
Attachment #740000 - Flags: review?(ehsan) → review+
(Assignee)

Comment 24

4 years ago
(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

4 years ago
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.
Attachment #740000 - Flags: feedback?(mozilla) → feedback+
(Assignee)

Comment 26

4 years ago
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.
Attachment #740000 - Attachment is obsolete: true
Attachment #740000 - Flags: review?(mh+mozilla)
Attachment #740000 - Flags: review?(landry)
Attachment #740035 - Flags: review+
Attachment #740035 - Flags: feedback?(landry)
(Assignee)

Updated

4 years ago
Attachment #740035 - Flags: review?(mh+mozilla)
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
Attachment #740035 - Flags: review?(mh+mozilla) → review+

Comment 28

4 years ago
Created attachment 740258 [details] [diff] [review]
generic prefs based dictionary path, final

Final version,  nit for whitespace addressed.
Attachment #740258 - Flags: review+
Attachment #740258 - Flags: feedback+

Updated

4 years ago
Keywords: checkin-needed
_AxS_ asked for this to be assigned to him.
Assignee: nobody → axs
Attachment #563829 - Attachment is obsolete: true
Attachment #740035 - Attachment is obsolete: true
Attachment #740035 - Flags: feedback?(landry)
https://tbpl.mozilla.org/?tree=Try&rev=1c9216f942fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfe59fafcaf0
Keywords: checkin-needed
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...
https://hg.mozilla.org/mozilla-central/rev/dfe59fafcaf0
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
(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!
You need to log in before you can comment on or make changes to this bug.