This is related to bug #714067.

*** Bug 724570 has been marked as a duplicate of this bug. ***

Created attachment 287003 [details] [review]
Implement a crude language selector for the spell checker

I added a small patch that allows to use the preferences dialog to select the languages to be used in the spell checker in Geary. 

The entry in the preferences must be filled with a comma-separated list of the locales to use. For example: "fr_FR, en_US". 

I would say that this user interface is not adequate for inclusion, but maybe it can be used as a starting point.

I would say this setting does not belong to the preference dialog, since you may want to set the lagange every message (or even every paragraph).

This is encouraging for a number of reasons.  First, I didn't realize multiple languages could be selected for spellcheck.  That is itself interesting, since a number of people requesting this fix indicate they are multilingual.

I suppose my vision for this was either (a) WebKit or WebKitGTK would fix this and we would need to do nothing, or (b) we would detect the user's preferred language and set that automatically -- no need for Preferences.

Maybe we need to rethink these assumptions.  It would be better if the user didn't have to type their language choices.  One possibility is to present a list of installed languages and let the user choose from that list.  Or, maybe we simply gather that list and configure WebKit automatically with them.

Federic, I'm not sure I agree we want to allow the user to pick the spell checker for each paragraph.  I've never seen that in other software, including word processing.  Is this common?

As far as choosing it for each message, that makes more sense, in which case if there is a UI component, perhaps it belongs in the composer itself rather than Preferences.  The last-selected language could still be stored in GSettings for the next time the composer is opened.

I agree with Jim. 

Moreover, I initially thought about choosing the language for each message (that is what I have always done in the past with Thunderbird and other email clients) but in the end having all the languages at the same time is even better, I don't have to worry about changing back and forth (at least for me).

I think that g_get_language_names() retrieves the list of languages configured in Gnome. On my system, for example, the output is ['it_IT', 'it', 'en', 'C']. This could be a good guess for a default setting in WebKit. 

It is quite reasonable that the languages used by the user are the ones configured as languages in Gnome, and maybe there is no need to replicate the settings elsewhere. On the other hand, a language selector would be a more complete and flexible solution.

I agree the UI component should belong to the composer.

In word processor (eg LOwriter) it is possible to set the language per
paragraph (or even per character) in case you have a multi-language document.
But multi-language messages are not so common and you can still check spelling
in the different languages one after the other.

As for check spelling different languages at the same time, it is not a perfect
solution since it might not detect all the mistakes (e.g. if you write "and" in
a french message while you have both english and french set as languages). Maybe it can be the default, but then the user can have the possibility to deselect some of the languages.

I think what I'm coming around to is something like this:

* Choose one (1) language for spell-checking in the composer drop-down menu or the context menu.  (I'm not firm on which, but the context menu looks like a better fit.)  We normally eschew nested menus, but it seems like in this case we'll need one, i.e.:

Spell-check language --> English (en_US)
                         English (en_GB)
                         English (en_AU)
                         French (fr)
                         French (fr_CA)

* The list should be the user's/system's installed languages.  I'm fuzzy on how this works, but if there's some way to get the *user's* preferred languages, that would be best.

* Geary should store the last-chosen language in GSettings.  Use the user's default language if not stored in GSettings.

* No change to Preferences.

As far as spell-checking per paragraph, another thought occurred to me: I don't know if WebKit supports that.  The API in the patch appears to be per WebView.

With Leonardo's patch as a starting point, I think someone could make this happen fairly quickly.  The only technical question I see is getting the list of languages, and that seems answered by Leonardo as well.

Would be awesome if this could be implemented!

Personnally, I would prefer having all the installed languages selected by default. This way, the spell checking works without human intervention for all messages whatever their language (including multi-language messages) and corrects 95% of the mistakes. Then you have the possibility to de-select some languages if you want to correct the remaining 5% mistakes.

If you select the last-selected language, people will be surprised that the default language changes all the time. And newbies will have to figure out where to change the language.

Just my 0.02$

Another possibility would be to guess what is the language based on the first words of the message. I don't know how easy it is.

For a first step toward getting this highly-requested feature landed, I'm going to stick with the user-chooses-one-language approach.  My reasoning:

* I suspect that will solve the 90% use case immediately.

* We really don't know how well WebKit's multiple-language spellcheck works.  (I would like to look at the API closer, for one.)  If we want to add this later, modifying the code added in my proposal above will be easy.

* A heuristic to guess the user's language from the first few words concerns me for a whole lot of reasons.

My basic feeling about this task is that we should be relying on WebKit's spellchecking features, not trying to invent our own.  This patch should be about exposing existing functionality.

*** Bug 714067 has been marked as a duplicate of this bug. ***

In principle it should be as simple as setting `lang` to the desired language. Unfortunately this does not work reliably (but this is not a sole webkit problem). Wouldn't it be better to rely on GTK+ for composing?

(In reply to comment #15)
> Wouldn't it be better to rely on GTK+ for composing?

No.

For better or worse, we support composing HTML email.  Therefore, we need some kind of rich-text editor, and we need to convert its format into HTML.  I'm sure there's a GTK widget that could do this.  But we also need to be able to save Drafts, and we need to do this as HTML, so that you can resume them from other clients.  So we need to be able to round trip rich text -> HTML -> rich text without any loss.  And we need to be able to quote emails that contain arbitrary HTML, so our rich text format needs to be a superset of HTML.

In short, using HTML as our rich text format is the only reasonable choice, and WebKit is the only modern option for doing HTML in GTK.  If we're stuck with a difficult-to-configure spellcheck, then that's the price we have to pay.

+1.
I write emails in English and Spanish and I'd love to have a way to switch languages for the spell checker.

+1

I have found this and spend some hours implementing a rough version of this, which can be configured using dconf. You can find the code at

  https://github.com/martinra/geary

I'm running this for my own work now, and so far (1 hour) it seems to be fine. Obviously, I want to have this upstream, to avoid merging.

Till this morning I have never worked with either Vala nor GTK, so I'm asking for a bit of assistance with finishing this.
  (1) How do I get the list of all Languages available for spell checking? I need
    (i) the language code,
    (ii) a translateble description
  (2) How do I find out about the currently used language?
  (3) How can I make webkit recheck the whole text?

Beyond that I suggest to got like this for finding out the languages to be displayed:
(1) get the list of available spell checkings
(2) intersect with list of languages in gsettings, if not empty
(3) use last language as fallback if there is no intersection
(4) if last language is not set, then us the current system language
(5) if no spell checking for the system language is available, give up
(6) display these
  (i) if less than 3 are to be displayed avoid submenus and display inline
  (ii) if only one language is to be displayed then suppress the choice.
(7) on change of language recheck everything if text is small enough, and recheck in an area around the cursor if text is too large (should be very rare, i.e. size limit should be generous)

Generally, I appreciate all kinds of feedback. Even more so since this is my GTK code.

Guys, any solution? 2 years have passed. An option in dconf would be fine as a first step.

Created attachment 327997 [details] [review]
Updated patch

I updated the patch that I wrote (almost) two years ago to behave as Jim suggested at the time: a dropdown list of available languages are now proposed in the composer. The language can be changed while typing and the last choice is stored in GSettings. 

Here are the main issues that I see right now: 
1) The list of available languages is obtained using GLib.Intl.get_language_names() which is likely non-optimal. 
2) The languages are shown with their codes (say, 'en_US', 'it_IT', ...). 
3) The document is not checked again when the language is changed, but I see that Webkit based browsers behave in the same way, so it is possible that we have to accept this. 

I looked at what most browsers do, and it seems that they maintain a list of all the possible languages together with their codes. This would solve point 1) and 2) but, on the other hand, it would require an UI component to select the preferred languages (otherwise the user will have to choose in a list with thousands of elements), and selecting a language for which no dictionary is present will silently fail (as far as I understand from the WebKit documentation). I have no idea if there is some reasonable way to check this. 

I would be very happy if, maybe after some iterations with feedback by the current maintainers, I could get this patch merged upstream. It is one of the last things that stop me from using Geary on a daily basis.

Hey Leonardo & martraum,

Thanks for working on this! I'm keen to get it in the next release.

It seems that GLib.Intl.get_language_names() is the correct way to get a list of user's current preferred languages. If the spell-check-languages setting is unset, the list returned by that function should be used as the default.

We want to let the user choose from a list of all installed languages, not all possible languages. One way to get that list and their presentation names would be to use the language util functions in libgnomedesktop3[0], but we can't actually rely on that since it does not offer any ABI/ABI stability. The only functions we need from that library however are `gnome_get_all_locales()` and `gnome_get_language_from_locale()`, so a minimal version of those could be ported to Vala and added to the International namespace in the client's util source.

Also, I wonder if WebKit can be convinced to re-check the spelling in an existing WebView if the Settings object is re-set using WebView.set_settings(), after having updated `spell_checking_languages` on it?

Adding this to a submenu of the composer's context menu is probably fine for the moment - that menu should be populated with Gtk.CheckMenuItem instances using the complete list of languages, which are checked if that language appears in the current list of languages. Although since it's multi-select, a Popover containing a ListBox triggered from a MenuButton on the toolbar might be more appropriate. Bonus points for implementing that, especially if it looks works like a multi-select version of the Language dialog's listbox in the Region & Language control panel.

[0] - https://developer.gnome.org/gnome-desktop3/3.20/gnome-desktop3-Language-Utilities.html

Created attachment 328102 [details] [review]
Patch using GtkCheckMenuItem and readable language names

Dear Michael,
thanks for the comments. I spent some time updating the previous patch and here is a brief recap of what works and what doesn't: 

-) I changed the MenuItem to CheckMenuItem so more than 1 language can be selected at the same time (and I updated the underlying code accordingly). 

-) I added a function International.official_name_from_locale() which is basically a very minimal re-implementation of what libgnomedesktop does. It only parses /usr/share/xml/iso-codes/iso_639.xml and looks for codes with two letters (are the other ones also used for dictionaries?). I am now using that to format the entries in the context menu.

-) I added a function International.get_available_locales() which looks for the installed dictionaries on the system by listing the files in /usr/share/hunspell. Epiphany developers also had a similar discussion last year [1] and it seems to me that they just decided to check what is installed on the system manually. However, this function is currently unused and on my system returns a very large number of dictionaries (I would say too many to be useful in the context menu - it takes almost two times the height of my screen). 

I agree that the choice on the menubar might be better than this solution, I just went this way because it was simpler, as of now. I would be happy to have feedback on the above choices, since I am not sure how optimal they are.

Possible issues: 

1) I do not know how standard the paths are, that is, is it safe to look for the xml file in that location directly? It seems to me that libgnomedesktop does it, so it is probably safe, but I could not find any "official" statement supporting this. 

2) There is also a Vala library called libisocodes which implement something similar to my very basic code, but it is not clear if there some advantage in using it compared to the effort of adding another dependency. [2]

Any comment/suggestion is appreciated! 

[1]: https://lists.webkit.org/pipermail/webkit-gtk/2015-January/002203.html
[2]: http://pkg-isocodes.alioth.debian.org/

Hey Leonardo, thanks for the updated patch!

It's good know that WebKitGTK uses Enchant to implement spell checking. We should probably be using that then to get a list of installed dictionaries - since as you say, the directory path to check may change depending on whether the user is using hunspell, aspell or ispell. Looking at valadoc.org, there is already an Enchant binding for Vala. Maybe the `Enchant.Broker.list_dicts()` method may help?

In that case, we probably want to do the following:

1. Have Configuration._get_spell_check_languages() check the GSettings value and if it is null or empty, return the list provided GLib.Intl.get_language_names (Note that the GSettings schema should be array of strings, i.e. have type "as", so the list doesn't need to be parsed and formatted manually)

2. Make International.get_available_locales() return a list of installed dictionaries using Enchant (so probably wants to be renamed to be something like "get_available_dictionaries")

3. When building the context menu's submenu, loop over the list of installed dictionaries, and only make the menu item checked if it is listed in Configuration._get_spell_check_languages().

I think your approach for International.official_name_from_locale() is good, but two things:
 - The string itself should be translated - maybe gnome_get_language_from_locale() will help here again?
 - The path to the XML should be set by the build system so distros can override it if they use a different path. We already are doing that for LANGUAGE_SUPPORT_DIRECTORY - so check how that works.

Created attachment 328152 [details] [review]
Updated patch

Dear Michael, 
here is your updated daily patch :) As a side note, I also pushed the code in the spell-checker branch of a fork of Geary on Github [1], if that's easier for you. 

I hope I have addressed all the issues that you raised. Here are some things that I am not completely happy with or that might be worth mentioning because I handled them differently with respect to what you were suggesting. Just let me know if you have any other comments/suggestions. 

1) Enchant works and provides a list of dictionaries. However, the list is quite large so that it is not so convenient to scroll through it to find the one that you want. A possible solution could be to have an interface to select a subset of the available dictionaries which are the ones most often used, and only switch between those, but this would definitely make things more complicated. Maybe it is fine to leave it like it is. 

2) I added the Enchant dependency. A VAPI file is shipped with Vala, so everything should be smooth in theory, but the VAPI file on my system (Vala 0.30, Ubuntu 16.04) gives a warning so compilation fails with NO_FATAL_WARNING=false. I fixed the warning and added the VAPI file inside bindings/vapi; I will let you judge if this a sensible solution or we should think of something else. 

3) I added the check for the directory of ISO Xml files at build time. I used CMake's find_path, since there is no guarantee that ${CMAKE_INSTALL_PREFIX} will contain the right files (one might want to install in /usr/local and still have those files in /usr). 

4) I mostly copied the implementation of the translation for the languages of the dictionaries from GNOME; it tries to show each dictionary in its own language, which is probably sensible, but in practice this fails on my system since I do not have so many locales installed (even if the dictionaries are there), so I see all of them in Italian. I think this is still fine (since in the end the user will see them in its own native language, so it shouldn't be a problem). 

[1]: https://github.com/robol/geary/

Hey Leonardo, this is coming together really well!

For (1), we probably want to take martraum's approach and use the common subset of the list of installed dicts and the list of all available locales. This is where `gnome_get_all_locales` would be handy but I think all it's doing is calling `locale -a, - that seems to do the right thing however, on my system at least. I would be inclined to start with the list of all locales, since it should be somewhat more canonical than the list of dicts, then check each one to see if there is a dictionary installed for it and set menu item insensitive if not. That way the user gets a vague prompt to go install the dictionary if it isn't found.

This may well also take care of (4), since if a user has the locale installed, there's perhaps a reasonable chance they will have translations for it as well.

I agree that including the VAPI is the best thing to do for now, but please file a bug in b.g.o against vala so this gets fixed upstream eventually and we can remove it again.

I see what you mean about the install prefix and the ISO XML file. Also, we might want to specify the full filename rather than the directory in the CMAKE var, since some distros may call the file itself something else. So maybe we need to do something like the following:
 - Check if some user-specified CMake var ("ISO_CODE_639_XML" maybe?) is set and if so use that (maybe exit with an error if the specified file it specifies does not exist?)
 - Check to see if the file exists in the default location under /usr and if so, use that -(this seems to be the default for Debian, Fedora and Arch, at least)
 - Check to see if exists in the default location under ${CMAKE_INSTALL_PREFIX}, and if so, use that
 - Otherwise fail with an error asking the user to either install "iso-codes" or set the CMake var

Minor comment: in Configuration::spell_check_languages:get, I'd be inclined to remove the call to settings.set_strv when the list is empty - I think we want to keep the setting unset as long as the user doesn't explicitly change it, that way if they change their desktop's language, the spell checker language will go with it without any extra effort.

So do we have WebKit re-checking the document when it changes yet?

Once we get this stuff addressed, it'll be worth-while calling for testing here (and maybe on the list?) - to see how well it works for different people on systems. Having the GitHub branch should make that easier, ta!

Thanks for the feedback, Michael! The new code is on Github.

Some quick comments first: 
- No, the spelling is not checked again after changing dictionaries, 
  re-setting the settings does not work, we will need to find another solution. 
- Ok, I will report the bug upstream for Vala. 
- The CMake configuration has been changed as you suggest. 
- I decided to show the dictionaries in one language only, since it is simpler    
  and calling setlocale() does not affect dgettext(). In fact, also the 
  implementation inside libgnomedesktop seems to return always the same 
  language on my system. The only thing that works is setting the LANGUAGE
  environment variable, but I think it might not worth it. On my system, that 
  is also what Thunderbird and Chrome do, for example.
- A thing that needs to be implemented is to show the countries instead of the 
  codes, that is "English (United Kingdom)" in place of "English (en_GB)". 
  That is another XML file, it is on my TODO list. :)

Some more long-ish comments: 

I changed the code so that it only shows the intersection of the set of available locales and dictionaries. The code is a little bit messy since GLib.Intl.get_language_names(), that we use when the user has no preferences, returns a third set of locales/language_codes which is not contained in any of the above (so an intersection between the three sets is needed), plus some
fuzzy matching since they are not perfectly equal...

I am having a second thought on the choice of what is more reasonable to show, though. My main doubt is that, even if one wants to write an email in a certain language, there is no reason why there should be the corresponding locale installed. 

As an example, consider the following use case: I am Italian, and write most of my emails in Italian or English (and both locales are installed because the system is in Italian and English is always there) but I also speak a little bit of Dutch and occasionally I write an email in Dutch. However, I do not have the Dutch locale available, even if I have the corresponding dictionary installed. With this setup I would not be able to select the Dutch dictionary.

Another quite artificial effect right now is that if the user deselects all the languages then the default choice comes into play and so the spell-checker is enabled again (in contrast to what one might expect, since all the visible languages are deselected). I think this is a little bit counter-intuitive.

I would propose something along these lines: 
 -) We keep a GSettings field for the _selected_ languages, and one for the
    _visible_ ones. One can switch between all of the visible ones
    using the context menu. 
 -) We put in the context menu an extra entry "More dictionaries" which shows 
    a dialog where the user can select more dictionaries to show there. 
    Potentially this could be extended to call PackageKit or alternatives in the 
    future (maybe this is a little too much for now - btw, can Enchant take 
    care of this?). 
 -) If the user has no selection of dictionaries to make visible, we take a 
    reasonable guess by looking at GLib.Intl.get_language_names(), and we 
    set the GSettings field.

What do you think? If you think this is a reasonable approach I will be happy to implement it. Then maybe we could try to get some testing by advertising these changes on the mailing list (which seems a good idea, given that all the setup will be different for this use case).

Hey Leonardo,

Oh yeah, I see your point about installed locales not being a good indicator, so having the two prefs does sound like a good idea.

It's probably reasonable to use GLib.Intl.get_language_names for the default values of both of those two settings when empty. Obviously, when building the UI for display, Geary would need to check what dictionaries are actually installed as well, since that could change without the setting changing.

I also like the idea of providing access to the additional dictionaries from the menu because it means when users want to use another one that isn't listed, the option is right there in front of them and they don't need to go off looking for it in the prefs.

The issue of having no dictionaries is annoying though. For now, if the last one is unchecked, just silently do nothing - keep it as the only locale.

Given that, and from as Frédéric points out this is a document-wide pref, I'm convinced that the context menu isn't the right place for this. It really should be a popover that works like the Language chooser in the control panel - this way managing the situation where there is only one selected can be handled properly (just do nothing while staying visible) and also the "More dictionaries" UI can be incorporated into the same widget - another dialog wouldn't be needed.

If you're not interested in that UI work I can look into it, but I don't think it would be any more work than say implementing a More Dictionaries dialog.

> So do we have WebKit re-checking the document when it changes yet?

I think this is a bad idea and is also not like for example iOS is behaving (only newly written words are checked). What if you start writing an e-Mail in Language A and continue writing in Language B?

I don’t know how Webkit implements spell checking, but naïvely I would assume that Webkit just sets the “lang”-attribute on a per paragraph basis and acts accordingly.

(In reply to Nils from comment #29)
> > So do we have WebKit re-checking the document when it changes yet?
> 
> I think this is a bad idea and is also not like for example iOS is behaving
> (only newly written words are checked). What if you start writing an e-Mail
> in Language A and continue writing in Language B?
> 
> I don’t know how Webkit implements spell checking, but naïvely I would
> assume that Webkit just sets the “lang”-attribute on a per paragraph basis
> and acts accordingly.

Yeah, I tend to agree - just so long as it does start checking in the newly selected languages, it should be fine for now.

> Yeah, I tend to agree - just so long as it does start checking in the newly
> selected languages, it should be fine for now.

Seems reasonable. Also, here is a brief update on the work in progress. I have an implementation that is almost satisfying, and the only feature that is missing is a sensible handling of which languages to show always, and which ones should only be displayed when the user wants to look for more. 

At the moment I am taking the preferred languages from GLib as a starting set of "always-visible" languages, and adding all the ones that are selected at least once to this list. However, in this way the list keeps growing and there is no way of hiding those new languages. I am thinking of a good way to handle that. 

The interface is implemented with a popover with search/filtering in it, similar to the language select in GNOME that Michael was mentioning. If you want to try it, everything is in the spell-checker branch of Github [1], but I will let you know as soon as I have something more complete. 

[1]: https://github.com/robol/geary/

Leonardo, I just cloned and built that branch and it is looking great. Once that last issue is sorted we can polish and get it committed to master.

Hi, I implemented the missing part (or at least a possible solution): when the user deactivates a language he/she gets the possibility to also remove it from the quicklist by clicking again. This is also stressed by a small text beside the entry. After 5 seconds the text disappears and the status of the button goes back to normal. 

I find this behavior quite ok in practice, and it seems to me a relatively good compromise between simplicity and ease of use (so we do not have to add another window to make the selection of quick-list of dictionaries...). 

The code is on Github, feel free to test it (the usual spell-checker branch). The code might benefit from another cleanup here and there, but I guess this could be a good moment if you want to have feedback also from other sources (say, other users with different use cases, maybe mentioning this on the mailing list?). In the meantime I can go through it and put in a better shape. 

I appreciate also feedback on the user interaction side, so if you think some other solution for inserting/removing item from the quick list would be better, I would be happy to know!

Regarding removing unwanted items, I'd suggest just following the HIG per this example: https://developer.gnome.org/hig/stable/lists.html.en#editable-lists - using "list-remove-symbolic" for the button's icon (and hence maybe use an image with "object-select-symbolic" for the tick to match) would work quite well. That way it's consistent with the rest of the desktop.

Also, it would be good to keep the list sorted alphabetically and remove plain languages when country-specific variants exist (e.g. remove "en" if "en_AU" exists).

Finally for some polish, the toolbar icon should be symbolic to match the rest of the buttons ("accessories-dictionary-symbolic" maybe?) and the ScrolledWindow probably needs a IN border, as is the custom.

If you can take care of this stuff and attach the changes in the branch here as one or two patches for code review, then we're basically done!

Created attachment 328993 [details] [review]
Selection of the spellchecker using a popover

Dear Michael, thanks for the comment! I implemented what you suggested, and I agree that the list looks better now (thanks for the hint about HIG on lists!). 

Here is a patch rebased onto the current master, with the code a little bit cleaned up. Let me know if you think this is a feasible solution, or if I need to do other changes.

Created attachment 329004 [details] [review]
Selection of the spellchecker using a popover

Hi, 
I updated the patch to hide the selector for the dictionaries when the spell-check is not enabled in the preferences (and connected the visibility property of the action to the relevant configuration in GSettings). 

Now the dictionary selection is only present when the spell-checker is active.

Review of attachment 329004 [details] [review]:

Looking good. Just a few minor comments to address.

::: CMakeLists.txt
@@ +92,3 @@
+pkg_check_modules(WEBKITGTK110X QUIET webkitgtk-3.0>=1.10.0 webkitgtk-3.0<=1.10.2)
+
+pkg_check_modules(GTK312X QUIET gtk+-3.0>=3.12.0)

I think there might be a rebase error here - the GTK+ 3.12 and four WebKitGTK checks here were recently removed from master.

@@ +113,3 @@
 include(GlibCompileResourcesSupport)
 
+

Unecessary whitespace change.

::: src/client/application/geary-config.vala
@@ +29,3 @@
     public Settings settings { get; private set; }
     public Settings gnome_interface;
+    

Need to remove the trailing whitespace added here.

@@ +83,3 @@
+        owned get {
+            string[] langs = settings.get_strv(SPELL_CHECK_LANGUAGES);
+            return langs;

Can this just return directly?

@@ +93,3 @@
+            if (langs.length == 0) {
+                langs = International.get_user_preferred_languages();
+                settings.set_strv(SPELL_CHECK_VISIBLE_LANGUAGES, langs);

I would remove this call to set the value - it should only ever get set if the user explicitly changes it.

::: src/client/composer/spell-check-popover.vala
@@ +48,3 @@
+        private uint timer_id = 0;
+ 
+        public SpellCheckLangRow (string lang_code) {

I'd be inclined to use two GtkImage instances for these rows - one for the tick and one for the close button. So for example with en_AU selected and en_UK and en_US not selected, it would look like:

 | | English (Australia) [✔]      [×] | |
 | | English (United Kingdom) [ ] [×] | |
 | | English (United States) [ ]  [×] | |

Where "[ ]" is a blank image. I don't have any examples to back this up though, so if it's a lot of work won't we can worry about it later.

@@ +188,3 @@
+ 
+        content = new Gtk.Box(Gtk.Orientation.VERTICAL, 6);
+        search_box = new Gtk.Entry();

This should probably be a Gtk.SearchEntry, so you don't need to manually add the search icon, etc, manually.

@@ +272,3 @@
+    }
+ 
+    private void on_row_toggled(string lang_code, bool active) {

When removing a lang, needs to check that active_langs.size > 1 before doing anything?

Oh, one last thing - use `git format-patch` if you can so when it is committed it shows up as being your contribution in the git log. Ta!

Created attachment 329418 [details] [review]
Implementation of the spell-checker selection popover

Dear Michael, 
here is the updated patch, which should address (I hope!) all your concerns :)

It should now work as you propose. If you think other changes are required let me know, and I will try to handle that. Moreover, if you think the commit message requires some modifications feel free to do that, I was not sure about the conventions used in Gnome/Geary for this, so I mostly guessed based on other commits. I also included the bug number, assuming it could be closed, but that is up to you, obviously. 

Btw, you were right and I got some spurious things in the previous rebase, hopefully they are all gone now.

Nope, this looks good for now, committed to master as cae4b44. Thanks! I expect people will start trying it out, so we can polish it further if we get any feedback once it is put to use.

Oh, also, since all of this spellcheck stuff is fresh in your mind, feel free to have a look at Bug 767398 if you'd like. ;)

*** Bug 757683 has been marked as a duplicate of this bug. ***

Hmm, there were some whitespace issues - tabs were used I think, but I'll fix that up. Thanks again!

I've just compiled from master and it works very well. Thanks Leonardo!

I have a question about the list of languages:

- when I click on the dictionary icon, by default I see only the language(s) enabled for spellchecking
- when I click on the search bar, the other languages (not enabled) show up

This is slightly misleading.
I think that the default view should be all the languages available and not only the enabled one(s). The tick symbol make it clear which languages are enabled.
Suggestion: list the enabled languages on top of the list.

Thanks for the feedback! 

In the list you should see all the dictionaries marked as "visible", that 
should be the ones you use more often; you can add/remove elements
from the always visible ones with [-] and [+]. Likely at the first initialization 
the list of visible dictionaries and the one of active dictionaries coincide, 
but this is not necessarily true after the user customizes them. Is this the
behavior that you get?

Maybe listing always all the languages, with the active ones of top, might
be a more intuitive solution? If there is agreement on this, I could implement
it. However, it might make it more difficult to switch back and forth between
a set of commonly used languages, which I guess is the most relevant use
case for most people. 

Or maybe, the point is that it is not so intuitive that you can disable a language
by just clicking over the row, while the [-] button is reserved to hide it from there?

I'm thinking twice about this.
Listing only the languages enabled is a good default IMO.

Yes, the hide/show clicking (-/+) is not straitghtforward (while enable/disable is ok, because of the check icon).
A tooltip would be enough.

Created attachment 329604 [details] [review]
Add dependency on libenchant to debian packaging files

In the previous patch I forgot to update the files in debian/ to reflect the new
dependency on libenchant. This is likely breaking the daily builds on Launchpad 
[1], even though I do not know if that repository is actively maintained. 

Concerning Federico's comments, maybe we can wait a little more and see if we get
other feedbacks on how to improve the UI (like the tooltip or other means), and 
then, if you wish, I can implement that. 

[1]: https://code.launchpad.net/~yorba/+recipe/geary-daily-build

Thanks Leonardo, I've just pushed the patch to fix for daily build.

I'm also inclined to leave it as-is for the moment as well and wait for feedback. I do feel like that the +/- icons may be a bit too much clutter, but acknowledge that people who might want to add/remove languages frequently would find it useful.

Some tooltips for the +/- like this should work:
 - "Remove this language from the list"
 - "Add this language to the list"

Also, I wonder if the + button is actually needed, since selecting a language not already on the list should automatically add it?

I'll put together a patch for these unless Leonardo beats me to it. :)

Just chasing this up, tool tips for those buttons have been pushed to master as commit 01b8f4c.

Please file new bugs relating to the spell checking UI or other related things, I consider this closed.