Bug 96518 – Add method to get the list of all available dictionaries

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Grzegorz Czajkowski]

There is lack of the method in TextCheckerEnchant class which allows the client to get the list of all available/installed dictionaries. To set the dictionaries the user have to 'guess/hardcode' the names of dictionaries that might be non obvious. The list can be used to set dictionaries used by Enchant.

To avoid names conflict this patch proposed a new one for former getSpellCheckingLanguages. It has been changed to loadedSpellCheckingLanguages as it gets the current (in use) dictionaries. Additionally the 'get' prefix has been removed according to WebKit coding style.

[Grzegorz Czajkowski]

Created attachment 163795 [details]
proposed patch

This getter can be used by WebKit's GTK and EFL API for instance:
 - webkit_web_context_set_spell_checking_languages(),
 - ewk_text_checker_setting_spell_checking_languages_set()
to set dictionaries used by Enchant.

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Mario Sanchez Prada]

Comment on attachment 163795 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163795&action=review

The patch looks good to me, although I'm a bit confused because I was expecting to see this new function used in the patch for bug 91854, but I guess that will come in a separate patch?

Otherwise, I just have one humble suggestion (see below).

> Source/WebCore/platform/text/enchant/TextCheckerEnchant.cpp:196
> +    enchant_broker_list_dicts(m_broker, enchantDictDescribeCallback, &allDictionaries);

Maybe early return if allDictionaries.isEmpty()?

[Grzegorz Czajkowski]

(In reply to comment #3)
> (From update of attachment 163795 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163795&action=review
> 
> The patch looks good to me, although I'm a bit confused because I was expecting to see this new function used in the patch for bug 91854, but I guess that will come in a separate patch?

The last patch for bug 91854 adds ewk_text_checker_setting_spell_checking_languages_get() which gets already loaded dictionaries not available once that this patch proposes.
We decided to keep old API as it is and add a new one ewk_text_checker_setting_available_spell_checking_languages_get() to get installed languages.

> 
> Otherwise, I just have one humble suggestion (see below).
> 
> > Source/WebCore/platform/text/enchant/TextCheckerEnchant.cpp:196
> > +    enchant_broker_list_dicts(m_broker, enchantDictDescribeCallback, &allDictionaries);
> 
> Maybe early return if allDictionaries.isEmpty()?
I thought about it too. IMO this method is very short and if the allDictionaries.isEmpty() the iterations won't happen of course and method returns an empty vector. To early return we have to move 'Vector<String> languages' somewhere above. If you prefer early return here just let me know I will change it. Thanks.

[Mario Sanchez Prada]

(In reply to comment #4)
> [...]
> The last patch for bug 91854 adds ewk_text_checker_setting_spell_checking_languages_get() which gets already loaded dictionaries not available once that this patch proposes.
> We decided to keep old API as it is and add a new one ewk_text_checker_setting_available_spell_checking_languages_get() to get installed languages.

Ok. Thanks for the clarification.

> [...]
> > Maybe early return if allDictionaries.isEmpty()?
> I thought about it too. IMO this method is very short and if the allDictionaries.isEmpty() the iterations won't happen of course and method returns an empty vector. To early return we have to move 'Vector<String> languages' somewhere above. If you prefer early return here just let me know I will change it. Thanks.

Not a big deal. It already looks good to me as it is now.

[Gyuyoung Kim]

Comment on attachment 163795 [details]
proposed patch

I think this is the way to go for both gtk and efl ports.

[Carlos Garcia Campos]

Comment on attachment 163795 [details]
proposed patch

LGTM, thanks!

[WebKit Review Bot]

Comment on attachment 163795 [details]
proposed patch

Clearing flags on attachment: 163795

Committed r128567: <http://trac.webkit.org/changeset/128567>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.