The GTK+ port could do with spell checking support, now that we have the required graphics backend support for squiggly red underlines. Attention should be paid to the precedent set by Mac and Fx and popular GNOME apps when it comes to deciding what libraries and dictionaries should be used.
We could use Enchant. Or we could use GSpell: http://techn.ocracy.org/gspell EditorClientGtk.cpp stubs: void EditorClient::ignoreWordInSpellDocument(const String&) { notImplemented(); } void EditorClient::learnWord(const String&) { notImplemented(); } void EditorClient::checkSpellingOfString(const UChar*, int, int*, int*) { notImplemented(); } void EditorClient::checkGrammarOfString(const UChar*, int, Vector<GrammarDetail>&, int*, int*) { notImplemented(); } void EditorClient::updateSpellingUIWithGrammarString(const String&, const GrammarDetail&) { notImplemented(); } void EditorClient::updateSpellingUIWithMisspelledWord(const String&) { notImplemented(); } void EditorClient::showSpellingUI(bool) { notImplemented(); } bool EditorClient::spellingUIIsShowing() { notImplemented(); return false; } void EditorClient::getGuessesForWord(const String&, Vector<String>&) { notImplemented(); }
GSpell died when I started to work on telepathy stuff, so it's not usable and Enchant is the only decent solution. I was already planning to work on this but it's very low in my priority list.
I'm working on something, but i'm having problems understanding how it works :-). Will post draft soon.
I reported this: https://bugs.webkit.org/show_bug.cgi?id=24419 which seems to be blocking the correct functioning of any patch we could do. I'll attach my half baked patch now.
Created attachment 28348 [details] Draft implementation, but not quite working I reported this: https://bugs.webkit.org/show_bug.cgi?id=24419 which seems to be blocking the correct functioning of any patch we could do. The attached patch is drafty and just here for reference.
Comment on attachment 28348 [details] Draft implementation, but not quite working First, some style issues: > --- a/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp > +++ b/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp > @@ -1,3 +1,4 @@ > +/* vim: set sw=4 ts=4 sts=4 et: */ I'm all for having markers for vim and emacs, to ease our lives, but I think they should be done in a different patch. If you work on this, please cover all of the GTK+-related files with vim and emacs markings =D > -void EditorClient::checkSpellingOfString(const UChar*, int, int*, int*) > +void EditorClient::checkSpellingOfString(const UChar* text, int length, int* misspellingLocation, int* misspellingLength) > { > - notImplemented(); > + const gchar *ctext; The * should be to the left here. > + ctext = g_strstrip (g_utf16_to_utf8(const_cast<gunichar2*>(text), length, > + NULL, NULL, NULL)); Use 0 instead of NULL on C++-only files. Also, there is an additional space between the function name and the parens. I think you can keep the 0's in the same line, too. It won't be overly long. > + g_debug("checking spelling of string %s, %d", > + ctext, length); Remember to remove those when submitting the final patch. > + GList *langs = NULL; * to the left > + int i = 0; > + while (ctext[i] != ' ' && i < length) > + i++; Looks like a for would be more idiomatic here, since this while is exactly mimmicking for behavior. > + if (result == 0) > + { > + *misspellingLocation = -1; > + } > + else if (result > 0) > + { > + *misspellingLocation = 1; > + } No bracers when you only have one line. If you were going to use bracers, though, they should be like this: if (blah) { ... } else { ... }
Comment on attachment 28348 [details] Draft implementation, but not quite working OK, now for what's probably not working > + ctext = g_strstrip (g_utf16_to_utf8(const_cast<gunichar2*>(text), length, > + NULL, NULL, NULL)); [...] > + int i = 0; > + while (ctext[i] != ' ' && i < length) > + i++; [...] > + result = enchant_dict_check(lang->speller, > + ctext, -1); So here's my theory on why this is not working. You are asking enchant to test the whole string, even though you have gone out of your way to find the first word, looking for a space. You really need to feed only the first word to it, as I understand it. [...] > + if (misspellingLength) > + *misspellingLength = i; > + } Then you only set misspellingLength, but you did not set misspellingLocation. Notice that the code in Editor.cpp only does anything if misspellingLocation is >= 0. It will be -1 the way your code looks currently. I believe the code in Editor.cpp is working correctly, but you need to give it correct return values in both misspellingLocation and misspellingLength.
Spelling check table I promised to diegoe Input/Ouput expectations for void EditorClient::checkSpellingOfString(const UChar* text, int length, int* misspellingLocation, int* misspellingLength) Notice that this method returns data through int pointers. INPUT misspellingLocation misspellingLength "asd" 0 3 " qwe" 1 3 " " -1 0 "apple kov" 6 3 " apple kov " 7 3 "apple orange kov tree" 13 3 The input list is realistic; I have checked that all of those are sent to the function in a way or another. The problem we have is that our Enchant library doesn't help us here, apparently; we cannot give it arbitrary text and have it tell us where the spelling problem starts, and what is its length, like the library used by Mac does, so we need to do our own string handling (such as finding word boundaries, stripping spaces) to feed enchant with the word we want to check, and nothing more, while still being able to give WebCore the proper location.
A little more sane: INPUT/misspellingLocation/misspellingLength "asd"/0/3 " qwe"/1/3 " "/-1/0 "apple kov"/6/3 " apple kov "/7/ 3 "apple orange kov tree"/13/3 "apple orange tree"/-1/0 Notice, too, that misspellingLocation is initially set to -1, and mispellingLength is set to 0, by the caller.
Created attachment 29380 [details] Adds enable-spell-checking property to WebKitWebSettings
Created attachment 29382 [details] spell-checking-languages property for WebKitWebSettings
Created attachment 29383 [details] implement checkSpellingOfString() First two patches are needed properties, first one by webkit, and second one by us because we want to be able to check more than system default language. This is still not finished. This is working ok with any kind of word, but if you use non ascii chars it chokes and fails because we are feeding enchant a char length, not byte length. I welcome any suggestion to fix that.
Created attachment 29385 [details] implement checkSpellingOfString() with utf8 friendliness A quick look at google for enchant+utf8 magic took me to various examples, and I was able to adapt the patch :-). All hail to codesearch, concrete inspiration came from libsexy.
Created attachment 29387 [details] Implement-ignore-learn-and-guesses :)
Created attachment 29388 [details] Small update to checkSpellingOfString patch This fixes some small bugs in the previous patch.
Things to think/decide on: - suggestions limit, i set it to 10, but perhaps that's already too much, and don't think we could do sorting, would be a bit overkill and enchant probably already does something smart. - replacing misspelled word by a suggested word -> deleted text; don't think this is related to spelling, i'm betting that this is an unimplemented replace function somewhere in this same file - spelling UI: I'm not particularly interested, and I guess we can have it as a second iteration of this, I would like to land the current patches and later do this comments welcome!
Comment on attachment 29380 [details] Adds enable-spell-checking property to WebKitWebSettings There are some style issues and I don't know if you guys want Diego to fix them (then I would r=-) or if we want to fix them when landing (then I would r=+) them. So I will just comment on the patches and talk about that.
Comment on attachment 29380 [details] Adds enable-spell-checking property to WebKitWebSettings A ChangeLog entry is missing. :) > bool EditorClient::isContinuousSpellCheckingEnabled() > { > - notImplemented(); > - return false; > + WebKitWebSettings* settings; > + gboolean enabled; > + > + settings = webkit_web_view_get_settings(m_webView); > + g_object_get(settings, "enable-spell-checking", &enabled, NULL); NULL or 0... the old question :) > + g_object_class_install_property(gobject_class, > + PROP_ENABLE_SPELL_CHECKING, > + g_param_spec_boolean( > + "enable-spell-checking", > + "Enable Spell Checking", > + "Enables check-as-you-type spell checking", > + FALSE, > + flags)); There is a patch from Gustavo that is adding i18n to the properties as well, it might make sense to assume this patch was landed (if it wasn't) and use the glorious _() gettext macro for the summary and description. > + case PROP_ENABLE_SPELL_CHECKING: > + g_value_set_boolean(value, priv->enable_spell_checking); > + break; break is misplaced.
(In reply to comment #18) > > bool EditorClient::isContinuousSpellCheckingEnabled() > > { > > - notImplemented(); > > - return false; > > + WebKitWebSettings* settings; > > + gboolean enabled; > > + > > + settings = webkit_web_view_get_settings(m_webView); > > + g_object_get(settings, "enable-spell-checking", &enabled, NULL); > > NULL or 0... the old question :) I've been working with diego through jabber on some of this work, and we found that this specific case makes the compiler unhappy (because it apparently looks for NULL when figuring out whether a sentinel has been added). > > > + g_object_class_install_property(gobject_class, > > + PROP_ENABLE_SPELL_CHECKING, > > + g_param_spec_boolean( > > + "enable-spell-checking", > > + "Enable Spell Checking", > > + "Enables check-as-you-type spell checking", > > + FALSE, > > + flags)); > > There is a patch from Gustavo that is adding i18n to the properties as well, it > might make sense to assume this patch was landed (if it wasn't) and use the > glorious _() gettext macro for the summary and description. It is now landed. Please #include <glib/gi18n-lib.h>, and add the _() markings, yes =).
(In reply to comment #17) > (From update of attachment 29380 [details] [review]) > There are some style issues and I don't know if you guys want Diego to fix them > (then I would r=-) or if we want to fix them when landing (then I would r=+) > them. So I will just comment on the patches and talk about that. > I think it is a good idea to let Diego fix the issues, so that he gets a bit more used to the style issues we usually have problems with, for future contributions.
Comment on attachment 29380 [details] Adds enable-spell-checking property to WebKitWebSettings Please update the patch according to the comments.
Created attachment 29700 [details] 0001-Add-enable-spell-checking-property.patch
Created attachment 29701 [details] 0002-Add-spell-checking-languages-property.patch
Created attachment 29702 [details] 0003-Implement-checkSpellingOfString.patch
Created attachment 29703 [details] 0004-Implement-ignore-learn-and-guesses.patch
Updated the patches to address comments and also cleaned them a bit. Regarding the ChangeLog I thought about shaping one after agreeing code is good. Gustavo can give me a hand, don't you? :-) Let me know if something is still missing.
Comment on attachment 29700 [details] 0001-Add-enable-spell-checking-property.patch > bool EditorClient::isContinuousSpellCheckingEnabled() > { > - notImplemented(); > - return false; > + WebKitWebSettings* settings; > + gboolean enabled; > + > + settings = webkit_web_view_get_settings(m_webView); > + g_object_get(settings, "enable-spell-checking", &enabled, NULL); Move the initialization of settins to where it is declared; there is another ocurrence of this a few lines down, fix it too =). Otherwise looks good. Please fix this, and upload another patch adding a ChangeLog entry, too.
Comment on attachment 29701 [details] 0002-Add-spell-checking-languages-property.patch > @@ -49,6 +49,7 @@ > #include "WindowFeatures.h" > > #include <glib.h> > +#include <enchant.h> I believe this include should go on the list above, between BackForwardList.h and HistoryItem.h. We need a full style review for the gtk files, I think, but let's try to at least keep the new code sane. > WEBKIT_API void > webkit_web_settings_add_extra_plugin_directory (WebKitWebView *web_view, const gchar* directory); > + > + GSList* > + webkit_web_settings_get_spell_languages (WebKitWebView *web_view); Drop the space between the function name and the parens; the * is misplaced. > + /** > + * WebKitWebSettings:spell-checking-languages: > + * > + * The languages to be used for spell checking, separated by commas. It would be good to say in what standard (such as ISO or an RFC) apps are supposed to specify languages. A few examples should do good here, too. > static void webkit_web_settings_init(WebKitWebSettings* web_settings) > { > web_settings->priv = WEBKIT_WEB_SETTINGS_GET_PRIVATE(web_settings); > + web_settings->priv->spell_checking_languages_list = 0; > } I don't think this initialization is necessary. > @@ -398,6 +418,9 @@ static void webkit_web_settings_finalize(GObject* object) > g_free(priv->sans_serif_font_family); > g_free(priv->serif_font_family); > g_free(priv->user_stylesheet_uri); > + g_free(priv->spell_checking_languages); > + > + g_slist_free(priv->spell_checking_languages_list); Is it safe to free this without checking? I tend to think it is, but reading the code was not enough to make me confident. > + case PROP_SPELL_CHECKING_LANGUAGES: > + { > + priv->spell_checking_languages = g_strdup(g_value_get_string(value)); > + > + SpellLanguage* lang; > + GSList* spell_languages = 0; I would prefer having no additional braces here, and have the variable declarations outside the switch. > + spell_languages = g_slist_prepend(spell_languages, lang); Use append or reverse the list after adding all the languages; I think we should honor the assumption that languages will be checked in the order they are listed in the string. > + * Since: 1.1.5 Forgot to mention; you'll want to update these to 1.1.6. > + WebKitWebSettings* settings; > + WebKitWebSettingsPrivate* priv; > + GSList *list; > + > + settings = webkit_web_view_get_settings(web_view); > + priv = WEBKIT_WEB_SETTINGS_GET_PRIVATE (settings); > + > + list = priv->spell_checking_languages_list; Similar to the previous patch; initialization where declared.
(In reply to comment #28) > > + g_slist_free(priv->spell_checking_languages_list); > > Is it safe to free this without checking? I tend to think it is, but reading > the code was not enough to make me confident. I've asked around on IRC and people I trust tell me this is safe, so keep it.
Comment on attachment 29702 [details] 0003-Implement-checkSpellingOfString.patch > +/* vim: set sw=4 ts=4 sts=4 et: */ No marking for this patch, though I would welcome vim/emacs markings in a separate patch. > + PangoLanguage* language; > + PangoLogAttr* attrs = g_new(PangoLogAttr, utflen+1); What's this +1 for? A comment will do. > + int orig_mispellingLocation = *misspellingLocation; > + int orig_mispellingLength = *misspellingLength; Make these originalMispellingLocation/Length. > + int start; > + int end; > + int word_length; wordLength > + lang = (SpellLanguage*) langs->data; Use a C++-style cast here: static_cast<SpellLanguage*>(langs->data). > + cstart = g_utf8_offset_to_pointer(ctext, start); > + bytes = (gint) (g_utf8_offset_to_pointer(ctext, end) - cstart); Same here. > + result = enchant_dict_check(lang->speller, word, -1); > + if (result > 0) > + { > + *misspellingLocation = start; > + *misspellingLength = word_length; > + } > + if (result == 0) > + { Make that if (re...) { ... } else if (...) { ... } > --- a/WebKit/gtk/WebCoreSupport/EditorClientGtk.h > +++ b/WebKit/gtk/WebCoreSupport/EditorClientGtk.h > @@ -33,6 +33,7 @@ > #include "EditorClient.h" > > #include <wtf/Forward.h> > +#include <glib.h> This should go in the .cpp, right after Frame.h?
Comment on attachment 29703 [details] 0004-Implement-ignore-learn-and-guesses.patch > + GSList* langs; > + langs = webkit_web_settings_get_spell_languages(m_webView); Same line. > + for (; langs; langs = langs->next) > + { Brace should go on the previous line; there are other instances of this I forgot to complain about. > + lang = (SpellLanguage*) langs->data; Cast and initialiation on same line. > + for (; langs; langs = langs->next) > + { Brace. > + SpellLanguage* lang; > + gchar** suggestions; > + size_t n_suggestions; numberOfSuggestions > + size_t i; > + > + lang = (SpellLanguage*) langs->data; Initialization/cast. > + for (i = 0; i < n_suggestions && i < 10; i++) > + { > + String string = String::fromUTF8(suggestions[i]); > + guesses.append(string); No need to declare the variable. Make that guesses.append(String::from...).
(In reply to comment #30) > > + int orig_mispellingLocation = *misspellingLocation; > > + int orig_mispellingLength = *misspellingLength; > > Make these originalMispellingLocation/Length. On second thought, these are pretty unnecessary, from my reading of the code. You should be good with setting misspellingLocation to -1 and misspellingLength to 0 in case no problem is found.
(In reply to comment #30) > > + result = enchant_dict_check(lang->speller, word, -1); > > + if (result > 0) > > + { > > + *misspellingLocation = start; > > + *misspellingLength = word_length; > > + } > > + if (result == 0) > > + { > > Make that > > if (re...) { > ... > } else if (...) { > ... > } ok, make that, instead; no need to do those specific checks: if (result) { ... } else { ... }, actually.
Created attachment 29726 [details] 0001-Add-enable-spell-checking-property.patch
Created attachment 29727 [details] 0002-Add-spell-checking-languages-property.patch
Created attachment 29728 [details] 0003-Implement-checkSpellingOfString.patch
Created attachment 29729 [details] 0004-Implement-ignore-learn-and-guesses.patch
Pheww, this should be it. Thanks kov for your comments. Let me know if there's anything else to fix.
Created attachment 29730 [details] 0002-Add-spell-checking-languages-property.patch Previous patch had a warning because I was using spellLanguages unintialized, = 0 does the trick.
Comment on attachment 29726 [details] 0001-Add-enable-spell-checking-property.patch Looks good now.
Comment on attachment 29730 [details] 0002-Add-spell-checking-languages-property.patch > + if (priv->spell_checking_languages) > + { > + char** langs = g_strsplit(priv->spell_checking_languages, ",", -1); > + for (int i = 0; langs[i]; i++) > + { Braces are still problematic here. I'll fix when landing, though. > +GSList* webkit_web_settings_get_spell_languages(WebKitWebView *web_view) > +{ > + g_return_val_if_fail(WEBKIT_IS_WEB_VIEW(web_view), 0); > + > + WebKitWebSettings* settings = webkit_web_view_get_settings(web_view); > + WebKitWebSettingsPrivate* priv = WEBKIT_WEB_SETTINGS_GET_PRIVATE (settings); You want to avoid using GET_PRIVATE macros wherever possible. They are a way for people who need to keep binary compatibility and didn't add a priv pointer to their object structures to fetch a private one, but is a considerable performance hit when comparing with simply using the pointer. I'll change this when landing, too.
Comment on attachment 29728 [details] 0003-Implement-checkSpellingOfString.patch Yay. This code looks so much different and more powerful than our first iterations. Pretty sure we'll find more weaknesses, but this is good for debut, IMO.
Comment on attachment 29729 [details] 0004-Implement-ignore-learn-and-guesses.patch Yep.
+ + GSList* + webkit_web_settings_get_spell_languages(WebKitWebView* web_view); Shouldn't this go in webkitprivate.h? Also, what is our policy about return values, do we explicitly say you DO NOT have to free something? Or if nothing is said it is assumed you don't have to. + GSList* spellLanguages = 0; We use NULL in these files. + if (priv->spell_checking_languages) + { The brace is wrong. + char** langs = g_strsplit(priv->spell_checking_languages, ",", -1); Don't have to free this? + GSList* list = priv->spell_checking_languages_list; + + return list; Just return the thing?
Comment on attachment 29728 [details] 0003-Implement-checkSpellingOfString.patch > #include "CString.h" > #include "EditCommand.h" > #include "Editor.h" > +#include <enchant.h> > #include "FocusController.h" > #include "Frame.h" > +#include <glib.h> > #include "KeyboardCodes.h" > #include "KeyboardEvent.h" > #include "NotImplemented.h" Those headers go separated from the others, not mixed.
(In reply to comment #44) > + > + GSList* > + webkit_web_settings_get_spell_languages(WebKitWebView* web_view); > > Shouldn't this go in webkitprivate.h? It's where it is, by my reading. > Also, what is our policy about return values, do we explicitly say you DO NOT > have to free something? Or if nothing is said it is assumed you don't have to. That's a good question. We usually use const to make it obvious, but in the case of a GSList, this is not easily done. > + GSList* spellLanguages = 0; > > We use NULL in these files. Right. > + if (priv->spell_checking_languages) > + { > > The brace is wrong. I have fixed those in preparation for landing. > + char** langs = g_strsplit(priv->spell_checking_languages, ",", > -1); > > Don't have to free this? Do. Good catch, I'll add a g_strfreev > + GSList* list = priv->spell_checking_languages_list; > + > + return list; > > Just return the thing? Sounds sane. I'll apply the necessary changes to the patches while landing. Thanks for the review =)
Landed as r42817-r42820. I updated the style issues diegoe forgot to fix, changed the preppend to append, as I have said should be done, too. The leak Xan spotted got plugged, and the style fixes he raised are in, too.
+ g_object_class_install_property(gobject_class, + PROP_ENABLE_SPELL_CHECKING, + g_param_spec_boolean( + "enable-spell-checking", + _("Enable Spell Checking"), + _("Enables check-as-you-type spell checking"), This is just a minor thing, but that reads very awkwardly. What about "Enables spell checking while typing"? + * WebKitWebSettings:spell-checking-languages: + * + * The languages to be used for spell checking, separated by commas. + * + * The locale string typically is in the form lang_COUNTRY, where lang + * is an ISO-639 language code, and COUNTRY is an ISO-3166 country code. + * For instance, sv_FI for Swedish as written in Finland or pt_BR + * for Portuguese as written in Brazil. + * + * If no value is specified then the value returned by + * pango_language_get_default will be used. + * + * Since 1.1.6 Very nice explanation, I like that. However it's technically wrong since the code uses gtk_get_default_language. It may be that both functions work the same way but still I'd like to see the same name in the code as in the documentation :) Hm... noticing too late that all this was committed already - did I mention I am missing an indication that a patch was committed in this bug tracker :-]