Created attachment 75525 [details] [review]
replaces pspell with enchant

very nice! thanks, porting to enchant has been on the todo list for some time.

I do not have time to review the patch immediately, but we'll get to it.

Just glancing at the patch I see that you use getenv("LANG"), is this really the better way to retrieve the language?

well, the original plugin retrieved it from pspell but enchant does not have such a configuration option AFAIK. 

Retrieving it from gconf might be a better solution but I just wanted to make a guess without tweaking with configuration options/dialogs.

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

Comment on attachment 75525 [details] [review]
replaces pspell with enchant

Thank you very much for the patch.
I'm very interesting in it since as pbor said moving the spell checker to enchant has been on the todo list for a lot of time.
You can also give a look at the similar patch attached to bug #316031.
I'm also going to attach the work Marco Bariosione and Emanuele Aina did under my supervision to transform the gedit spell checker plugin in a library (using enchant).

The patch looks very good, here my detailed comments to you patch.

Could you please modify your patch according to my comments (if they are ok for you) and attach a new version.

>+PKG_CHECK_MODULES(ENCHANT, [enchant])
>+ENCHANT_LIBS="${ENCHANT_LIBS}"
>+ENCHANT_CFLAGS="${ENCHANT_CFLAGS}"
>+SPELL_PLUGIN_DIR="spell"

In this way the gedit compilation fails if enchant is not installed.
We prefer to conditionally compile the spell checker plugin, i.e. SPELL_PLUGIN_DIR="spell" if enchant is installed,
otherwise it should be "".

> 
>+typedef struct _SpellManager
>+{
>+	EnchantBroker *config;
>+	EnchantDict *speller;
>+} SpellManager;
>+
> struct _GeditSpellChecker 
> {
> 	GObject parent_instance;
> 	
>-	PspellManager *manager;
>+	SpellManager *manager;
> 	const GeditLanguage *active_lang;
> };
> 

Why have you created a new structure instead of simply putting
broker and speller in the _GeditSpellChecker struct?

May be "dict" is a better name for "speller".

>-		language_tag = pspell_config_retrieve (config, "language-tag");
>+		language_tag = g_getenv ("LANG");

As pbor said, using g_getenv ("LANG") is not very safe.
You can use g_get_language_names as in the function get_default_language you find in the 
patch attached to bug #316031 (I have not checked the code of Marco and Emanuele).
Note that the get_default_language could be improved using the function enchant_broker_dict_exists.

>@@ -488,33 +489,25 @@ lazy_init (GeditSpellChecker *spell, con
> const GSList *
> gedit_spell_checker_get_available_languages (void)

Current gedit_spell_checker_get_available_languages uses a list of known languages and this create
lot of problems since the list is always out-of-date and since translators don't want to translate
the languages names, etc.
Do you think it is possible to remove the list of known languages and use the
enchant_broker_list_dicts function to dynamically get a list of installed/known languages?


>@@ -618,11 +614,10 @@ gedit_spell_checker_get_suggestions (Ged
> 			gint len, 
> 			GError **error)

If it simplifies the code we can change the gedit_spell_checker_get_suggestions to return a gchar **
instead of a GSList. What do you think?

>@@ -675,8 +666,6 @@ gedit_spell_checker_add_word_to_personal
> 			gint len, 
> 			GError **error)
> {
>-	gint pspell_result;
>-	
> 	g_return_val_if_fail (spell != NULL, FALSE);
> 	g_return_val_if_fail (GEDIT_IS_SPELL_CHECKER (spell), FALSE);
> 	
>@@ -690,19 +679,7 @@ gedit_spell_checker_add_word_to_personal
> 	if (len < 0)
> 		len = -1;
> 
>-	pspell_result = pspell_manager_add_to_personal (spell->manager, word, len);
>-
>-	if (pspell_result == 0)
>-	{
>-		g_set_error (error, GEDIT_SPELL_CHECKER_ERROR, 
>-					GEDIT_SPELL_CHECKER_ERROR_PSPELL,
>-					"pspell: %s", 
>-					pspell_manager_error_message (spell->manager));
>-
>-		return FALSE;
>-	}	
>-
>-	pspell_manager_save_all_word_lists (spell->manager);
>+	enchant_dict_add_to_pwl (spell->manager->speller, word, len);
> 
AFAIK, enchant_dict_add_to_pwl required requires Enchant 1.1.6.
May be we should check for Enchant version 1.2.x (or maybe 1.3.0)

I don't know enchant, so I have a stupid question. Does enchant_dict_add_to_pwl save the new pwl in a persistent
way? In pspell you had to call pspell_manager_save_all_word_lists to save the modifications to the personal dictionary.

>@@ -750,6 +715,7 @@ gedit_spell_checker_add_word_to_session 
> gboolean
> gedit_spell_checker_clear_session (GeditSpellChecker *spell, GError **error)
> {
>+#if 0
> 	gint pspell_result;
> 	
> 	g_return_val_if_fail (spell != NULL, FALSE);
>@@ -769,6 +735,7 @@ gedit_spell_checker_clear_session (Gedit
> 
> 		return FALSE;
> 	}	
>+#endif

Why are you commenting out most of the clear_session code? See patch attached to bug #316031.

> 
> 	g_signal_emit (G_OBJECT (spell), signals[CLEAR_SESSION], 0);
> 
>@@ -785,8 +752,6 @@ gedit_spell_checker_set_correction (Gedi
> 			const gchar *replacement, gint r_len,
> 			GError **error)
> {
>-	gint pspell_result;
>-	
> 	g_return_val_if_fail (spell != NULL, FALSE);
> 	g_return_val_if_fail (GEDIT_IS_SPELL_CHECKER (spell), FALSE);
> 	
>@@ -804,21 +769,9 @@ gedit_spell_checker_set_correction (Gedi
> 	if (r_len < 0)
> 		r_len = -1;
> 
>-	pspell_result = pspell_manager_store_replacement (spell->manager,
>+	enchant_dict_store_replacement (spell->manager->speller,
> 				word, w_len,
> 				replacement, r_len);

It seems a lot of functions in enchant do not report errors while their equivalent
in pspell did and so most of the error parameters in our functions can be removed.

Created attachment 76039 [details]
As promised you can find here the work done by Marco and Emanuele

> In this way the gedit compilation fails if enchant is not installed.
> We prefer to conditionally compile the spell checker plugin, i.e.
> SPELL_PLUGIN_DIR="spell" if enchant is installed,
> otherwise it should be "".

Do you prefer to keep the pspell bits and compile conditionally or dump
pspell in favor of enchant? For example in fedora enchant (and its
consumers) is in extras while gedit is in core. Distributions in a
similar position might end up disabling this plugin due to enchant
dependency.

> Why have you created a new structure instead of simply putting
> broker and speller in the _GeditSpellChecker struct?

While testing, I was replacing only this plugin after changing and
compiling it separately. Just wanted to be on the safe side and don't
possibly break some ABI. It's probably redundant.

> May be "dict" is a better name for "speller".

Yes it's better to use broker/dict for enchant. It was aspell using
config/speller.

> As pbor said, using g_getenv ("LANG") is not very safe.  You can use
> g_get_language_names as in the function get_default_language you find
> in the patch attached to bug #316031 (I have not checked the code of
> Marco and Emanuele).  Note that the get_default_language could be
> improved using the function enchant_broker_dict_exists.

Thanks for the link to the other patch, I'll try to merge.

> Current gedit_spell_checker_get_available_languages uses a list of
> known languages and this create lot of problems since the list is
> always out-of-date > and since translators don't want to translate the
> languages names, etc.  Do you think it is possible to remove the list
> of known languages and use the enchant_broker_list_dicts function to
> dynamically get a list of installed/known language4?

I'm not sure all back-ends will provide a string that can be passed to
the user or enchant has an API to acquire that string. But the
list_dicts function can be used to populate available_languages instead
of iterating over known_languages.

> If it simplifies the code we can change the
> edit_spell_checker_get_suggestions to return a gchar ** instead of a
> GSList.  What do you think?

That might be better if pspell parts won't be conditional.

> AFAIK, enchant_dict_add_to_pwl required requires Enchant 1.1.6.
> May be we should check for Enchant version 1.2.x (or maybe 1.3.0)

Ok.

> I don't know enchant, so I have a stupid question. Does
> enchant_dict_add_to_pwl save the new pwl in a persistent way? In
> pspell you had to call pspell_manager_save_all_word_lists to save the
> modifications to the personal dictionary.

Enchant just provides a common API for programs to access the existing
libraries via plugins that wrap them. So, if for example enchant is
configured to use pspell for a particular language, then add_to_pwl
function will call pspell_manager_add_to_personal() and then
pspell_manager_save_all_word_lists() from the plugin. And similarly
other plugins should do the right thing for their respective back-ends.

> Why are you commenting out most of the clear_session code? See patch
> attached to bug #316031.

I wasn't using a temporary session for the enchant back-end I was
testing.  That's why I forgot to implement this.

> It seems a lot of functions in enchant do not report errors while
> their equivalent in pspell did and so most of the error parameters in
> our functions can be removed.

Yes, again if pspell won't be used.

don't bother to keep the old pspell stuff conditional. I think we should use enchant all the way. I don't think it will be a problem for distros to promote enchant in main... if it is they can always ship the old plugin :)

Yep, but in the case enchant is not available the spell plugin must not be compiled. So it is the plugin itself to be conditional (like it is now).

In the case we are going to (even if only conditionally) require enchant as a dependency we should probably communicate this to the relase-team.

Created attachment 76163 [details] [review]
with requested changes

changes:
* --disable-spell configuration option
* renamed Enchant{Broker,Dict} and merged into GeditSpellChecker
* g_get_language_names() for default language
* clear_session implemented

todo:
* function signature changes

Comment on attachment 76163 [details] [review]
with requested changes


>+AC_ARG_ENABLE([spell],
>+	AS_HELP_STRING([--disable-spell],[Disable spell plugin]),
>+	[enable_enchant=$enableval],
>+	[enable_enchant=yes])
>+
>+if test "x$enable_enchant" = "xyes" ; then
>+	ENCHANT_REQUIRED=2.3.0

Why are you asking for 2.3.0?
AFAIK, latest version is 1.3.0 and it is probably too new.
For example, Ubuntu Edgy has 1.2.3.
I think we should ask for 1.2.0.

I will comment the remaining part of the patch later.

apart from the version issue, I tested the patch a bit and it seems to work well.

Yes, the required version has a typo. I meant 1.2 not 2.3 (though it will probably work with 1.1.6). Shall I attach a corected one?

No need to attach a new patch.
I will review and test your patch and if it is ok I will commit it in the next days.

Created attachment 76532 [details] [review]
Modified patch by Paolo

I have modified the Sertaç's patch in order to remove the infamous list of known languages (see bug #150535).

The patch is partially based on code I have taken from Epiphany.

I have created two new files that I will attach later.

TODO:
- finish to review the Sertaç's patch
- remove the unused GError parameters

Created attachment 76533 [details]
gedit-spell-checker-language.h file

Created attachment 76534 [details]
gedit-spell-checker-language.c file

Testing my patch I see some warning on the terminal, but I have still to investigate where they are generated.

** (gedit:24546): WARNING **: enchant: (null)

** (gedit:24546): WARNING **: enchant: (null)

** (gedit:24546): WARNING **: enchant: (null)


I have seen there are a lot of other interesting bug reports against the spell plugin.

Sertaç: would you like to give them a try? We really appreciate your help and we are happy with the high quality of your previous patches.

Created attachment 76537 [details] [review]
don't set error if there isn't *really* an error

this patch (after your patch) should fix the warning.

Committed in cvs and part of gedit 2.17.1. Thanks a lot for your work!


2006-12-04  Paolo Maggi  <paolo@gnome.org>

	* configure.ac:
	* plugins/spell/*:

	Switch the spell plugin to the enchant library and use
	the iso-codes package to retrieve language names.
	The port to the enchant spell checking library is based
	on a patch by Sertaç Ö. Yıldız. Bug #365899.