Bug 15616 - [GTK] Add spell checking
Summary: [GTK] Add spell checking
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 523.x (Safari 3)
Hardware: All All
: P2 Enhancement
Assignee: Diego Escalante Urrelo
URL:
Keywords: Gtk
Depends on:
Blocks: 19456
  Show dependency treegraph
 
Reported: 2007-10-22 06:20 PDT by Alp Toker
Modified: 2009-04-24 10:17 PDT (History)
7 users (show)

See Also:


Attachments
Draft implementation, but not quite working (5.05 KB, patch)
2009-03-05 22:38 PST, Diego Escalante Urrelo
no flags Details | Formatted Diff | Diff
Adds enable-spell-checking property to WebKitWebSettings (4.65 KB, patch)
2009-04-09 17:19 PDT, Diego Escalante Urrelo
zecke: review-
Details | Formatted Diff | Diff
spell-checking-languages property for WebKitWebSettings (7.06 KB, patch)
2009-04-09 17:19 PDT, Diego Escalante Urrelo
no flags Details | Formatted Diff | Diff
implement checkSpellingOfString() (5.34 KB, patch)
2009-04-09 17:21 PDT, Diego Escalante Urrelo
no flags Details | Formatted Diff | Diff
implement checkSpellingOfString() with utf8 friendliness (5.26 KB, patch)
2009-04-09 21:50 PDT, Diego Escalante Urrelo
no flags Details | Formatted Diff | Diff
Implement-ignore-learn-and-guesses (2.71 KB, patch)
2009-04-09 23:51 PDT, Diego Escalante Urrelo
no flags Details | Formatted Diff | Diff
Small update to checkSpellingOfString patch (5.74 KB, patch)
2009-04-09 23:53 PDT, Diego Escalante Urrelo
no flags Details | Formatted Diff | Diff
0001-Add-enable-spell-checking-property.patch (4.67 KB, patch)
2009-04-22 19:28 PDT, Diego Escalante Urrelo
gns: review-
Details | Formatted Diff | Diff
0002-Add-spell-checking-languages-property.patch (8.39 KB, patch)
2009-04-22 19:28 PDT, Diego Escalante Urrelo
gns: review-
Details | Formatted Diff | Diff
0003-Implement-checkSpellingOfString.patch (4.32 KB, patch)
2009-04-22 19:29 PDT, Diego Escalante Urrelo
gns: review-
Details | Formatted Diff | Diff
0004-Implement-ignore-learn-and-guesses.patch (2.72 KB, patch)
2009-04-22 19:29 PDT, Diego Escalante Urrelo
gns: review-
Details | Formatted Diff | Diff
0001-Add-enable-spell-checking-property.patch (6.66 KB, patch)
2009-04-23 21:11 PDT, Diego Escalante Urrelo
gns: review+
Details | Formatted Diff | Diff
0002-Add-spell-checking-languages-property.patch (11.48 KB, patch)
2009-04-23 21:12 PDT, Diego Escalante Urrelo
no flags Details | Formatted Diff | Diff
0003-Implement-checkSpellingOfString.patch (4.75 KB, patch)
2009-04-23 21:12 PDT, Diego Escalante Urrelo
gns: review+
Details | Formatted Diff | Diff
0004-Implement-ignore-learn-and-guesses.patch (4.05 KB, patch)
2009-04-23 21:13 PDT, Diego Escalante Urrelo
gns: review+
Details | Formatted Diff | Diff
0002-Add-spell-checking-languages-property.patch (11.48 KB, patch)
2009-04-23 21:23 PDT, Diego Escalante Urrelo
gns: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alp Toker 2007-10-22 06:20:00 PDT
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.
Comment 1 Alp Toker 2008-06-09 20:38:59 PDT
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();
}
Comment 2 Marco Barisione 2008-06-10 10:56:14 PDT
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.
Comment 3 Diego Escalante Urrelo 2009-03-05 18:35:46 PST
I'm working on something, but i'm having problems understanding how it works :-). Will post draft soon.
Comment 4 Diego Escalante Urrelo 2009-03-05 22:28:10 PST
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.
Comment 5 Diego Escalante Urrelo 2009-03-05 22:38:11 PST
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 6 Gustavo Noronha (kov) 2009-03-12 07:14:46 PDT
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 7 Gustavo Noronha (kov) 2009-03-12 07:23:44 PDT
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.
Comment 8 Gustavo Noronha (kov) 2009-03-20 13:24:16 PDT
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.
Comment 9 Gustavo Noronha (kov) 2009-03-20 13:27:13 PDT
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.
Comment 10 Diego Escalante Urrelo 2009-04-09 17:19:01 PDT
Created attachment 29380 [details]
Adds enable-spell-checking property to WebKitWebSettings
Comment 11 Diego Escalante Urrelo 2009-04-09 17:19:41 PDT
Created attachment 29382 [details]
spell-checking-languages property for WebKitWebSettings
Comment 12 Diego Escalante Urrelo 2009-04-09 17:21:32 PDT
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.
Comment 13 Diego Escalante Urrelo 2009-04-09 21:50:59 PDT
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.
Comment 14 Diego Escalante Urrelo 2009-04-09 23:51:13 PDT
Created attachment 29387 [details]
Implement-ignore-learn-and-guesses

:)
Comment 15 Diego Escalante Urrelo 2009-04-09 23:53:09 PDT
Created attachment 29388 [details]
Small update to checkSpellingOfString patch

This fixes some small bugs in the previous patch.
Comment 16 Diego Escalante Urrelo 2009-04-10 00:00:23 PDT
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 17 Holger Freyther 2009-04-11 20:35:43 PDT
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 18 Holger Freyther 2009-04-11 20:45:07 PDT
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.
Comment 19 Gustavo Noronha (kov) 2009-04-12 18:21:39 PDT
(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 =).
Comment 20 Gustavo Noronha (kov) 2009-04-12 18:23:38 PDT
(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 21 Holger Freyther 2009-04-12 19:50:48 PDT
Comment on attachment 29380 [details]
Adds enable-spell-checking property to WebKitWebSettings

Please update the patch according to the comments.
Comment 22 Diego Escalante Urrelo 2009-04-22 19:28:23 PDT
Created attachment 29700 [details]
0001-Add-enable-spell-checking-property.patch
Comment 23 Diego Escalante Urrelo 2009-04-22 19:28:45 PDT
Created attachment 29701 [details]
0002-Add-spell-checking-languages-property.patch
Comment 24 Diego Escalante Urrelo 2009-04-22 19:29:00 PDT
Created attachment 29702 [details]
0003-Implement-checkSpellingOfString.patch
Comment 25 Diego Escalante Urrelo 2009-04-22 19:29:14 PDT
Created attachment 29703 [details]
0004-Implement-ignore-learn-and-guesses.patch
Comment 26 Diego Escalante Urrelo 2009-04-22 19:31:51 PDT
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 27 Gustavo Noronha (kov) 2009-04-23 18:42:53 PDT
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 28 Gustavo Noronha (kov) 2009-04-23 19:12:27 PDT
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.
Comment 29 Gustavo Noronha (kov) 2009-04-23 19:17:23 PDT
(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 30 Gustavo Noronha (kov) 2009-04-23 19:30:52 PDT
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 31 Gustavo Noronha (kov) 2009-04-23 19:48:20 PDT
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...).
Comment 32 Gustavo Noronha (kov) 2009-04-23 20:20:26 PDT
(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.
Comment 33 Gustavo Noronha (kov) 2009-04-23 20:34:06 PDT
(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.
Comment 34 Diego Escalante Urrelo 2009-04-23 21:11:57 PDT
Created attachment 29726 [details]
0001-Add-enable-spell-checking-property.patch
Comment 35 Diego Escalante Urrelo 2009-04-23 21:12:18 PDT
Created attachment 29727 [details]
0002-Add-spell-checking-languages-property.patch
Comment 36 Diego Escalante Urrelo 2009-04-23 21:12:47 PDT
Created attachment 29728 [details]
0003-Implement-checkSpellingOfString.patch
Comment 37 Diego Escalante Urrelo 2009-04-23 21:13:07 PDT
Created attachment 29729 [details]
0004-Implement-ignore-learn-and-guesses.patch
Comment 38 Diego Escalante Urrelo 2009-04-23 21:19:44 PDT
Pheww, this should be it. Thanks kov for your comments. Let me know if there's anything else to fix.
Comment 39 Diego Escalante Urrelo 2009-04-23 21:23:14 PDT
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 40 Gustavo Noronha (kov) 2009-04-24 05:15:51 PDT
Comment on attachment 29726 [details]
0001-Add-enable-spell-checking-property.patch

Looks good now.
Comment 41 Gustavo Noronha (kov) 2009-04-24 05:15:57 PDT
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 42 Gustavo Noronha (kov) 2009-04-24 05:16:35 PDT
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 43 Gustavo Noronha (kov) 2009-04-24 05:17:04 PDT
Comment on attachment 29729 [details]
0004-Implement-ignore-learn-and-guesses.patch

Yep.
Comment 44 Xan Lopez 2009-04-24 05:28:59 PDT
+
+    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 45 Xan Lopez 2009-04-24 05:30:51 PDT
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.
Comment 46 Gustavo Noronha (kov) 2009-04-24 05:57:36 PDT
(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 =)
Comment 47 Gustavo Noronha (kov) 2009-04-24 06:18:41 PDT
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.
Comment 48 Christian Dywan 2009-04-24 10:17:57 PDT
+    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 :-]