I have added preliminary support for spell checking of annotations against git master. I just have to figure out how to properly integrate with jhbuild and I will submit a patch. 

I am using the "standard" gtkspell library to accomplish this. Is that okay? 

Just let me know!

It would be great to have this feature in Evince!

These days gspell — https://wiki.gnome.org/Projects/gspell — is much preferred over gtkspell. It’s actively maintained, widely used, and reportedly better in every aspect.

Thanks for the feedback! I will absolutely switch. It took more work to get library linking working than adding code so the change will be easy, I think. Patches soon. Thanks again for the feedback!

Okay, change to gspell is done. I have basic spell checking for new "text" annotations. I am going to tackle the problem of jhbuild integration of the new library.

Is there a good README on how we configure the evince build system to know that we have a new dependency on gspell (that jhbuild itself *can* handle [ie, jhbuild build gspell])? I have googled and tried to find information but couldn't find anything that really helped. 

Also, I think that I will need to integrate it with configure. This is definitely going to be more difficult than actually using the library :-) 

Thank you for any information that you can help me dig up!

Created attachment 370426 [details] [review]
configure: Add dependency for gspell

Will, you can use this patch as a base for adding the dependency to configure.

(In reply to José Aliste from comment #6)
> Will, you can use this patch as a base for adding the dependency to
> configure.

Wow! Really appreciate it! Thanks. 

Now, I am investigating which of the module sets that I am using by default with jhbuild. It sounds like that is the place where I will have to change the configuration of evince so that it knows to build gspell before evince.

Is that accurate?

(In reply to Will Hawkins from comment #7)
> (In reply to José Aliste from comment #6)
> > Will, you can use this patch as a base for adding the dependency to
> > configure.
> 
> Wow! Really appreciate it! Thanks. 
> 
> Now, I am investigating which of the module sets that I am using by default
> with jhbuild. It sounds like that is the place where I will have to change
> the configuration of evince so that it knows to build gspell before evince.
> 
> Is that accurate?

jhbuild is only a help tool to build things, It is not mandatory, for instance, I am runing the gspell from my distribution. But yes, you need to add gspell as a dependency of evince in jhbuild. (Please beware that jhbuild is being phased out, people have migrated to other tools)

(In reply to José Aliste from comment #8)
> (In reply to Will Hawkins from comment #7)
> > (In reply to José Aliste from comment #6)
> > > Will, you can use this patch as a base for adding the dependency to
> > > configure.
> > 
> > Wow! Really appreciate it! Thanks. 
> > 
> > Now, I am investigating which of the module sets that I am using by default
> > with jhbuild. It sounds like that is the place where I will have to change
> > the configuration of evince so that it knows to build gspell before evince.
> > 
> > Is that accurate?
> 
> jhbuild is only a help tool to build things, It is not mandatory, for
> instance, I am runing the gspell from my distribution. But yes, you need to
> add gspell as a dependency of evince in jhbuild. (Please beware that jhbuild
> is being phased out, people have migrated to other tools)

Understood. configure will rely on the local system's pkg-config to tell the build where to find (if any) gspell. If that comes from jhbuild, then so be it. If it comes from a local (compatible) install, then that's great, too.

Created attachment 370427 [details] [review]
Add spell check for annotations

I screwed up with my Bugzilla foo. I meant to mark that as obsoletes/supersedes what Jose helpfully sent earlier. Sorry!

Review of attachment 370427 [details] [review]:

::: libview/ev-annotation-window.h
@@ +31,1 @@
 

I think this declaration should be declared in ev-annotation-window.c. There is nothing in this header file that requires it, as far as I see it.

(In reply to Germán Poo-Caamaño from comment #12)
> Review of attachment 370427 [details] [review] [review]:
> 
> ::: libview/ev-annotation-window.h
> @@ +31,1 @@
>  
> 
> I think this declaration should be declared in ev-annotation-window.c. There
> is nothing in this header file that requires it, as far as I see it.

Good call. I will make that change! Thanks for the feedback!

Created attachment 370428 [details] [review]
Add spell check for annotations

(In reply to Will Hawkins from comment #13)
> (In reply to Germán Poo-Caamaño from comment #12)
> > Review of attachment 370427 [details] [review] [review] [review]:
> > 
> > ::: libview/ev-annotation-window.h
> > @@ +31,1 @@
> >  
> > 
> > I think this declaration should be declared in ev-annotation-window.c. There
> > is nothing in this header file that requires it, as far as I see it.
> 
> Good call. I will make that change! Thanks for the feedback!

Patch updated. As ever, thank you for the comments on the previous patch!

Will

Hello, everyone! I just wanted to make sure that this little patch met everyone's standards. I am looking forward to getting feedback on this before I dig in to the next bug/feature. I have had lots of fun getting started with contributing to evince and want to continue but want to make sure I am being "helpful" before doing so!

I don't want to waste anyone's precious time!

Thanks for all the help getting me involved!
Will

Review of attachment 370428 [details] [review]:

It looks good to me.

I think we are missing a way of deactivating/activating the spell check

(In reply to José Aliste from comment #18)
> I think we are missing a way of deactivating/activating the spell check

How about in the contextual menu within the annotation?

I mean, enabled by default, and change the setting while working on annotations. Leave the change permanent.

(In reply to Germán Poo-Caamaño from comment #19)
> (In reply to José Aliste from comment #18)
> > I think we are missing a way of deactivating/activating the spell check
> 
> How about in the contextual menu within the annotation?
> 
> I mean, enabled by default, and change the setting while working on
> annotations. Leave the change permanent.

Great idea. I will start implementation and update the patch when I have it working!

Thanks again for all the review you are giving this!
Will

(In reply to Germán Poo-Caamaño from comment #19)
> (In reply to José Aliste from comment #18)
> > I think we are missing a way of deactivating/activating the spell check
> 
> How about in the contextual menu within the annotation?
> 
> I mean, enabled by default, and change the setting while working on
> annotations. Leave the change permanent.

It sounds good to me

(In reply to José Aliste from comment #21)
> (In reply to Germán Poo-Caamaño from comment #19)
> > (In reply to José Aliste from comment #18)
> > > I think we are missing a way of deactivating/activating the spell check
> > 
> > How about in the contextual menu within the annotation?
> > 
> > I mean, enabled by default, and change the setting while working on
> > annotations. Leave the change permanent.
> 
> It sounds good to me

To what, exactly, are you referring when you say "contextual menu"? The menu that would popup when you right click in the text editing area? Or, the menu that pops up when you right click on an annotation before expanding it?

I think the first, like together with the spelling suggestions and the language option when clicking in the text editing area of the annotation window  you need an option "Highlight misspelled words" that should be checked by default and if you uncheck it it should disable the spelling of annotations for this file for ever (or until you change it again)... Thinking about this, this would mean we need a new setting in the metadata probably...

(In reply to José Aliste from comment #23)
> I think the first, like together with the spelling suggestions and the
> language option when clicking in the text editing area of the annotation
> window  you need an option "Highlight misspelled words" that should be
> checked by default and if you uncheck it it should disable the spelling of
> annotations for this file for ever (or until you change it again)...
> Thinking about this, this would mean we need a new setting in the metadata
> probably...

That's exactly what I was planning to add! I will keep you posted! Thanks for the feedback!

(In reply to José Aliste from comment #23)
> I think the first, like together with the spelling suggestions and the
> language option when clicking in the text editing area of the annotation
> window  you need an option "Highlight misspelled words" that should be
> checked by default and if you uncheck it it should disable the spelling of
> annotations for this file for ever (or until you change it again)...
> Thinking about this, this would mean we need a new setting in the metadata
> probably...

Why would you do it per file? I would do it permanently (gsettings).

Imagine, if I do not want to use spellchecker because I do not like it (or I use so much slang), it will be a nightmare to annotate documents.

Do I want the spellchecker back? I will probably know that I want it back when annotating another document.

(In reply to Germán Poo-Caamaño from comment #25)
> (In reply to José Aliste from comment #23)
> > I think the first, like together with the spelling suggestions and the
> > language option when clicking in the text editing area of the annotation
> > window  you need an option "Highlight misspelled words" that should be
> > checked by default and if you uncheck it it should disable the spelling of
> > annotations for this file for ever (or until you change it again)...
> > Thinking about this, this would mean we need a new setting in the metadata
> > probably...

Read the probably :P 

> 
> Why would you do it per file? I would do it permanently (gsettings).
> 
Yeah,that sounds good... I couldn't decide whether we want to persist this setting for the file, or just a general setting, but yes...leaving it enabled /disabled for ALL documents sounds not disturbing and simple.

(In reply to José Aliste from comment #26)
> (In reply to Germán Poo-Caamaño from comment #25)
> > (In reply to José Aliste from comment #23)
> > > I think the first, like together with the spelling suggestions and the
> > > language option when clicking in the text editing area of the annotation
> > > window  you need an option "Highlight misspelled words" that should be
> > > checked by default and if you uncheck it it should disable the spelling of
> > > annotations for this file for ever (or until you change it again)...
> > > Thinking about this, this would mean we need a new setting in the metadata
> > > probably...
> 
> Read the probably :P 
> 
> > 
> > Why would you do it per file? I would do it permanently (gsettings).
> > 
> Yeah,that sounds good... I couldn't decide whether we want to persist this
> setting for the file, or just a general setting, but yes...leaving it
> enabled /disabled for ALL documents sounds not disturbing and simple.

This is exactly the route that I am taking. I have wired up a metadata setting that reads in a spell check setting. That setting eventually gets put through the window through the view to the annotations. The only question I have is this:

Is there a designated spot in shell/ev-window.c where I would "persist" changes in the window back to the metadata? Annotations emit a signal when the user toggles the spell check setting and I suppose that I could watch that signal and write it back there. But, I just thought there might be another place that is more centralized. 

Thank you!

Forget the metadata. Use a global setting (gsettings).

(In reply to Germán Poo-Caamaño from comment #28)
> Forget the metadata. Use a global setting (gsettings).

Perfecto! I will do that. I am up to my eyes in getting this to work. It's hilarious because the actual spellcheck implementation was WAY easier than wiring up these settings :-)

Created attachment 370672 [details] [review]
Add spell check for annotations (v3)

(In reply to Will Hawkins from comment #30)
> Created attachment 370672 [details] [review] [review]
> Add spell check for annotations (v3)

Patch updated. I'm sure there is plenty to fix, but this is functional and includes the ability to change whether spellcheck is enabled or disabled!

I can't wait to hear feedback and improve this so it is up to everyone's standards.

(In reply to Will Hawkins from comment #31)
> (In reply to Will Hawkins from comment #30)
> > Created attachment 370672 [details] [review] [review] [review]
> > Add spell check for annotations (v3)
> 
> Patch updated. I'm sure there is plenty to fix, but this is functional and
> includes the ability to change whether spellcheck is enabled or disabled!
> 
> I can't wait to hear feedback and improve this so it is up to everyone's
> standards.

Just wanted to see if there were any comments on the patch and what you thought needed to be added/deleted/changed to get this up to the group's exacting standards!

Review of attachment 370672 [details] [review]:

Thanks for the patch. I wonder whether we really need to enable it conditionally, why not simply use it when compiled with gspell?

::: data/org.gnome.Evince.gschema.xml.in
@@ +41,3 @@
+		<key name="spellcheck-annotations" type="b">
+      <default>true</default>
+      <_summary>Enable inline spellcheck for annotations.</_summary>

If we are going to use a setting, I would make it more generic. enable-spell-checking. The fact that is only used for annotation popup windows doesn't really matter.

::: libview/ev-annotation-window.c
@@ +39,3 @@
+	PROP_PARENT,
+#if WITH_GSPELL
+        PROP_SPELLCHECK

I would call this ENABLE_SPELLCHECKING. Also I think we can reduce the amount of ifdefs, if we add the property unconditionally, but we do nothing if gspell is not available.

@@ +212,3 @@
+#if WITH_GSPELL
+void
+ev_annotation_window_set_spellcheck (EvAnnotationWindow *window,

set_enable_spellchecking

@@ +214,3 @@
+ev_annotation_window_set_spellcheck (EvAnnotationWindow *window,
+                                     gboolean spellcheck)
+{

Use g_return macros in public functions.

@@ +226,3 @@
+
+gboolean
+ev_annotation_window_get_spellcheck (EvAnnotationWindow *window)

get_enable_spellchecking

@@ +229,3 @@
+{
+        return window->spellcheck;
+}

Both getter and setter should be moved below, after the public methods comment.

@@ +377,3 @@
+        gtk_menu_shell_prepend (GTK_MENU_SHELL (menu), separator);
+        gtk_widget_show (separator);
+        menu_item = gtk_check_menu_item_new_with_mnemonic (_("_Spellcheck Annotations"));

If we really need a context menu option for this, I would call it Enable spellchecking

::: libview/ev-view-private.h
@@ +229,3 @@
 	GHashTable        *annot_window_map;
+#if WITH_GSPELL
+        gboolean           spellcheck_annotations;

enable_spellchecking or spellchecking_enabled

::: libview/ev-view.c
@@ +83,3 @@
+	PROP_CAN_ZOOM_OUT,
+#if WITH_GSPELL
+        PROP_SPELLCHECK_ANNOTATIONS

ENABLE_SPELLCHECKING

@@ +3161,3 @@
+        g_signal_connect_object (EV_ANNOTATION_WINDOW (window), "notify::spellcheck",
+                                 G_CALLBACK (ev_view_annotation_window_save_spellcheck_setting),
+                                 view, 0);

So every window can disable spellchecking on all others and also the window can disable it?

@@ +5740,3 @@
+#if WITH_GSPELL
+void
+ev_view_set_spellcheck_annotations (EvView *view,

set_enable_spelchecking

Thanks for your comments, Carlos! I really appreciate your detailed feedback. I will get started on making the changes to the names as quickly as possible. 

I only had one other specific comment and I will make it inline, below.


(In reply to Carlos Garcia Campos from comment #33)
> Review of attachment 370672 [details] [review] [review]:
> 
> Thanks for the patch. I wonder whether we really need to enable it
> conditionally, why not simply use it when compiled with gspell?

Well, I thought the same thing too but there was a consensus between Jose and German that we should make it conditionally available to the user through a preference. I honestly do not care one way or the other, but I will leave it this way unless I hear otherwise from Jose and German!

Again, thanks for sending this feedback. I will send an updated patch asap!

Will

> 
> ::: data/org.gnome.Evince.gschema.xml.in
> @@ +41,3 @@
> +		<key name="spellcheck-annotations" type="b">
> +      <default>true</default>
> +      <_summary>Enable inline spellcheck for annotations.</_summary>
> 
> If we are going to use a setting, I would make it more generic.
> enable-spell-checking. The fact that is only used for annotation popup
> windows doesn't really matter.
> 
> ::: libview/ev-annotation-window.c
> @@ +39,3 @@
> +	PROP_PARENT,
> +#if WITH_GSPELL
> +        PROP_SPELLCHECK
> 
> I would call this ENABLE_SPELLCHECKING. Also I think we can reduce the
> amount of ifdefs, if we add the property unconditionally, but we do nothing
> if gspell is not available.
> 
> @@ +212,3 @@
> +#if WITH_GSPELL
> +void
> +ev_annotation_window_set_spellcheck (EvAnnotationWindow *window,
> 
> set_enable_spellchecking
> 
> @@ +214,3 @@
> +ev_annotation_window_set_spellcheck (EvAnnotationWindow *window,
> +                                     gboolean spellcheck)
> +{
> 
> Use g_return macros in public functions.
> 
> @@ +226,3 @@
> +
> +gboolean
> +ev_annotation_window_get_spellcheck (EvAnnotationWindow *window)
> 
> get_enable_spellchecking
> 
> @@ +229,3 @@
> +{
> +        return window->spellcheck;
> +}
> 
> Both getter and setter should be moved below, after the public methods
> comment.
> 
> @@ +377,3 @@
> +        gtk_menu_shell_prepend (GTK_MENU_SHELL (menu), separator);
> +        gtk_widget_show (separator);
> +        menu_item = gtk_check_menu_item_new_with_mnemonic (_("_Spellcheck
> Annotations"));
> 
> If we really need a context menu option for this, I would call it Enable
> spellchecking
> 
> ::: libview/ev-view-private.h
> @@ +229,3 @@
>  	GHashTable        *annot_window_map;
> +#if WITH_GSPELL
> +        gboolean           spellcheck_annotations;
> 
> enable_spellchecking or spellchecking_enabled
> 
> ::: libview/ev-view.c
> @@ +83,3 @@
> +	PROP_CAN_ZOOM_OUT,
> +#if WITH_GSPELL
> +        PROP_SPELLCHECK_ANNOTATIONS
> 
> ENABLE_SPELLCHECKING
> 
> @@ +3161,3 @@
> +        g_signal_connect_object (EV_ANNOTATION_WINDOW (window),
> "notify::spellcheck",
> +                                 G_CALLBACK
> (ev_view_annotation_window_save_spellcheck_setting),
> +                                 view, 0);
> 
> So every window can disable spellchecking on all others and also the window
> can disable it?
> 
> @@ +5740,3 @@
> +#if WITH_GSPELL
> +void
> +ev_view_set_spellcheck_annotations (EvView *view,
> 
> set_enable_spelchecking

Created attachment 371478 [details] [review]
Add spell check for annotations (v4)

(In reply to Will Hawkins from comment #35)
> Created attachment 371478 [details] [review] [review]
> Add spell check for annotations (v4)

I hope that the latest version of this patch addresses all your helpful comments, Carlos. 

Thanks again for taking the time to provide detailed feedback!

Will

Review of attachment 371478 [details] [review]:

Ok, I don't mind to keep the setting if you guys prefer it, I'll review it again assuming we will keep it. I would still reduce the amount of ifdefs in the code too.

::: data/org.gnome.Evince.gschema.xml.in
@@ +39,3 @@
       <_summary>Allow links to change the zoom level.</_summary>
     </key>
+		<key name="enable-spellchecking" type="b">

indentation doesn't look correct here.

::: libview/ev-annotation-window.c
@@ +38,3 @@
 	PROP_ANNOTATION,
+	PROP_PARENT,
+#if WITH_GSPELL

We can always define the property, even if it always returns FALSE when building without gspell. It doesn't really need to be a property though, we don't expect callers to watch it, we can simply add public setter/getter to se the internal value.

@@ +72,3 @@
+#ifdef WITH_GSPELL
+        GspellTextView *spellcheck_view;
+        gboolean      spellcheck;

I would rename it as enable_spellchecking to match the property name.

@@ +355,3 @@
+        gtk_menu_shell_prepend (GTK_MENU_SHELL (menu), separator);
+        gtk_widget_show (separator);
+        menu_item = gtk_check_menu_item_new_with_mnemonic (_("_Enable Spellchecking"));

If we are going to add a global setting, I don't think the option should be attached to an annot popup window. I would add it to the view menu like inverted colors or odd pages left. This way the window handles the setting and it only need to update the view and the view updates its windows, everything ins a single direction.

@@ +670,3 @@
+        g_object_class_install_property (g_object_class,
+                                         PROP_ENABLE_SPELLCHECKING,
+                                         g_param_spec_boolean ("spellcheck",

enable-spellchecking, but I still think, we don't really need a property here.

@@ +789,3 @@
 }
+
+#if WITH_GSPELL

Move the ifdef inside the function, doing nothing if built without gspell and always returning FALSE in the getter.

@@ +794,3 @@
+                                               gboolean spellcheck)
+{
+

Remove this empty line

@@ +795,3 @@
+{
+
+        g_return_if_fail (EV_IS_ANNOTATION_WINDOW (window));

And leave an empty line after the g_return

@@ +809,3 @@
+ev_annotation_window_get_enable_spellchecking (EvAnnotationWindow *window)
+{
+        g_return_val_if_fail (EV_IS_ANNOTATION_WINDOW (window), FALSE);

Ditto.

::: libview/ev-annotation-window.h
@@ +52,3 @@
 void          ev_annotation_window_grab_focus     (EvAnnotationWindow *window);
 void          ev_annotation_window_ungrab_focus   (EvAnnotationWindow *window);
+#if WITH_GSPELL

And then we remove this ifdef too.

::: libview/ev-view.c
@@ +83,3 @@
+	PROP_CAN_ZOOM_OUT,
+#if WITH_GSPELL
+        PROP_SPELLCHECK_ANNOTATIONS

ENABLE_SPELLCHECKING, but again I don't think we need to make this a property if the window handles the global setting and simply notifies the view when it changes.

@@ +3127,3 @@
+#if WITH_GSPELL
+static void
+ev_view_annotation_window_save_spellcheck_setting (EvAnnotationWindow *window,

Then we don't need this

@@ +3161,3 @@
+        g_signal_connect_object (EV_ANNOTATION_WINDOW (window), "notify::spellcheck",
+                                 G_CALLBACK (ev_view_annotation_window_save_spellcheck_setting),
+                                 view, 0);

Nor this.

@@ +3175,3 @@
 				  doc_rect.x1, doc_rect.y1);
+#if WITH_GSPELL
+        ev_annotation_window_set_enable_spellchecking (EV_ANNOTATION_WINDOW (window), ev_view_get_enable_spellchecking (view));

Yhis is simpler if you use view->enable_spellchecking, we don't need to use the public getters from private functions.

@@ +5738,3 @@
 }
 
+#if WITH_GSPELL

Move the ifdef inside the functions here.

::: shell/ev-window.c
@@ +268,3 @@
 #define GS_ALLOW_LINKS_CHANGE_ZOOM "allow-links-change-zoom"
+#if WITH_GSPELL
+#define GS_SPELLCHECK_ANNOTATIONS  "enable-spellchecking"

ENABLE_SPELLCHECKING

@@ +1469,3 @@
+                          GS_SPELLCHECK_ANNOTATIONS,
+                          ev_view_get_enable_spellchecking (view));
+        g_settings_apply (window->priv->settings);

Do we really need the apply here?

@@ +1483,3 @@
+        g_signal_connect_object (window->priv->view, "notify::enable-spellchecking",
+                                 G_CALLBACK (spellcheck_annotations_changed_cb),
+                                 window, 0);

We can do all this in the init, right after creating the view, like we do with other settings. You should also connect to the changed signal of the setting in ev_window_ensure_settings.

Thank you again for the review! I appreciate all your comments. I hope that the prior submission I made addressed your initial comments -- I don't want you to think that I was ignoring anything that you suggested!

I will submit a revised patch, but I wanted to, in general, note that the suggestion was made by German and Jose that we have the spell check on/off option in each annotation window. Since it was a decision made by senior people on the project and you, also, are a senior person on the project, I will wait for you three to come to a consensus on your suggestion about changing placement of the option before changing the patch.

Until then I will work through the other critiques that you made and submit a revised patch as soon as possible. 

Again, thank you for taking the time to review this patch. I am learning many things about the Evince code style. I hope that I am getting closer.

Will 

(In reply to Carlos Garcia Campos from comment #37)
> Review of attachment 371478 [details] [review] [review]:
> 
> Ok, I don't mind to keep the setting if you guys prefer it, I'll review it
> again assuming we will keep it. I would still reduce the amount of ifdefs in
> the code too.
> 
> ::: data/org.gnome.Evince.gschema.xml.in
> @@ +39,3 @@
>        <_summary>Allow links to change the zoom level.</_summary>
>      </key>
> +		<key name="enable-spellchecking" type="b">
> 
> indentation doesn't look correct here.
> 
> ::: libview/ev-annotation-window.c
> @@ +38,3 @@
>  	PROP_ANNOTATION,
> +	PROP_PARENT,
> +#if WITH_GSPELL
> 
> We can always define the property, even if it always returns FALSE when
> building without gspell. It doesn't really need to be a property though, we
> don't expect callers to watch it, we can simply add public setter/getter to
> se the internal value.
> 
> @@ +72,3 @@
> +#ifdef WITH_GSPELL
> +        GspellTextView *spellcheck_view;
> +        gboolean      spellcheck;
> 
> I would rename it as enable_spellchecking to match the property name.
> 
> @@ +355,3 @@
> +        gtk_menu_shell_prepend (GTK_MENU_SHELL (menu), separator);
> +        gtk_widget_show (separator);
> +        menu_item = gtk_check_menu_item_new_with_mnemonic (_("_Enable
> Spellchecking"));
> 
> If we are going to add a global setting, I don't think the option should be
> attached to an annot popup window. I would add it to the view menu like
> inverted colors or odd pages left. This way the window handles the setting
> and it only need to update the view and the view updates its windows,
> everything ins a single direction.
> 
> @@ +670,3 @@
> +        g_object_class_install_property (g_object_class,
> +                                         PROP_ENABLE_SPELLCHECKING,
> +                                         g_param_spec_boolean ("spellcheck",
> 
> enable-spellchecking, but I still think, we don't really need a property
> here.
> 
> @@ +789,3 @@
>  }
> +
> +#if WITH_GSPELL
> 
> Move the ifdef inside the function, doing nothing if built without gspell
> and always returning FALSE in the getter.
> 
> @@ +794,3 @@
> +                                               gboolean spellcheck)
> +{
> +
> 
> Remove this empty line
> 
> @@ +795,3 @@
> +{
> +
> +        g_return_if_fail (EV_IS_ANNOTATION_WINDOW (window));
> 
> And leave an empty line after the g_return
> 
> @@ +809,3 @@
> +ev_annotation_window_get_enable_spellchecking (EvAnnotationWindow *window)
> +{
> +        g_return_val_if_fail (EV_IS_ANNOTATION_WINDOW (window), FALSE);
> 
> Ditto.
> 
> ::: libview/ev-annotation-window.h
> @@ +52,3 @@
>  void          ev_annotation_window_grab_focus     (EvAnnotationWindow
> *window);
>  void          ev_annotation_window_ungrab_focus   (EvAnnotationWindow
> *window);
> +#if WITH_GSPELL
> 
> And then we remove this ifdef too.
> 
> ::: libview/ev-view.c
> @@ +83,3 @@
> +	PROP_CAN_ZOOM_OUT,
> +#if WITH_GSPELL
> +        PROP_SPELLCHECK_ANNOTATIONS
> 
> ENABLE_SPELLCHECKING, but again I don't think we need to make this a
> property if the window handles the global setting and simply notifies the
> view when it changes.
> 
> @@ +3127,3 @@
> +#if WITH_GSPELL
> +static void
> +ev_view_annotation_window_save_spellcheck_setting (EvAnnotationWindow
> *window,
> 
> Then we don't need this
> 
> @@ +3161,3 @@
> +        g_signal_connect_object (EV_ANNOTATION_WINDOW (window),
> "notify::spellcheck",
> +                                 G_CALLBACK
> (ev_view_annotation_window_save_spellcheck_setting),
> +                                 view, 0);
> 
> Nor this.
> 
> @@ +3175,3 @@
>  				  doc_rect.x1, doc_rect.y1);
> +#if WITH_GSPELL
> +        ev_annotation_window_set_enable_spellchecking (EV_ANNOTATION_WINDOW
> (window), ev_view_get_enable_spellchecking (view));
> 
> Yhis is simpler if you use view->enable_spellchecking, we don't need to use
> the public getters from private functions.
> 
> @@ +5738,3 @@
>  }
>  
> +#if WITH_GSPELL
> 
> Move the ifdef inside the functions here.
> 
> ::: shell/ev-window.c
> @@ +268,3 @@
>  #define GS_ALLOW_LINKS_CHANGE_ZOOM "allow-links-change-zoom"
> +#if WITH_GSPELL
> +#define GS_SPELLCHECK_ANNOTATIONS  "enable-spellchecking"
> 
> ENABLE_SPELLCHECKING
> 
> @@ +1469,3 @@
> +                          GS_SPELLCHECK_ANNOTATIONS,
> +                          ev_view_get_enable_spellchecking (view));
> +        g_settings_apply (window->priv->settings);
> 
> Do we really need the apply here?
> 
> @@ +1483,3 @@
> +        g_signal_connect_object (window->priv->view,
> "notify::enable-spellchecking",
> +                                 G_CALLBACK
> (spellcheck_annotations_changed_cb),
> +                                 window, 0);
> 
> We can do all this in the init, right after creating the view, like we do
> with other settings. You should also connect to the changed signal of the
> setting in ev_window_ensure_settings.

For what it is worth, Carlos is the most senior developer of Evince.

That said, after thinking about it, I tend to agree with Carlos. Perhaps, leave the option as a setting not exposed in the user interface.

I have been wondering under which circumstances someone might not want the spellchecker enabled. That is, considering that it is only noticeable when there are spelling mistakes.

I can only come to think that when an user can annotate a document in multiple languages (meaning, one document in one language, another document in a second language, and so on). I do know how spellcheck handles that.

Will, yes, moving the option to the view menu is the right thing to do, as it should be about spellchecking and not spellchecking annotations. We should after this is merged add maybe a spellcheck option for forms.

(In reply to José Aliste from comment #40)
> Will, yes, moving the option to the view menu is the right thing to do, as
> it should be about spellchecking and not spellchecking annotations. We
> should after this is merged add maybe a spellcheck option for forms.

This will make implementation so much easier :-) 

I will have an updated patch submitted this evening. Working out one last kink as we speak!

Will

Created attachment 371528 [details] [review]
Add spell check for annotations (v5)

Updated patch is now attached to the bug! With a centralized option for the spell checking, the implementation is much cleaner. After prodding from Carlos, I think that I finally understood what he meant about not having as many #if. Took me a while to understand what he meant. Sorry about that. I think that this version of the patch is much better in that regard. 

I am really looking forward to your feedback on this version of the patch. Thanks to everyone's comments, I think that it is much better than earlier versions. 

Thanks again for the opportunity to contribute to the project!

Will

Review of attachment 371528 [details] [review]:

This looks much better now, but I still have a few minor comments.

::: libview/ev-annotation-window.c
@@ +70,3 @@
+	GspellTextView *spellcheck_view;
+#endif
+	gboolean      enable_spellchecking;

I could move this inside the ifdef, since we don't really use it when gspell is not available.

@@ +367,3 @@
+
+#if WITH_GSPELL
+	window->spellcheck_view = NULL;

This is already NULL at this point, and you are setting it in the next line.

@@ +721,3 @@
+{
+        g_return_if_fail (EV_IS_ANNOTATION_WINDOW (window));
+

Move the ifdef here and simply do nothing.

@@ +736,3 @@
+ev_annotation_window_get_enable_spellchecking (EvAnnotationWindow *window)
+{
+        g_return_val_if_fail (EV_IS_ANNOTATION_WINDOW (window), FALSE);

And here add the ifdef else to return FALSE unconditionally in cae of gspell not being available, like you are doing in the view.

::: libview/ev-view.c
@@ +5734,3 @@
+                n_pages = ev_document_get_n_pages (view->document);
+
+        for (current_page = 0; current_page<n_pages; current_page++) {

Add a space before and after the <

@@ +5735,3 @@
+
+        for (current_page = 0; current_page<n_pages; current_page++) {
+

Remove this empty line.

(In reply to Carlos Garcia Campos from comment #44)
> Review of attachment 371528 [details] [review] [review]:
> 
> This looks much better now, but I still have a few minor comments.
> 
> ::: libview/ev-annotation-window.c
> @@ +70,3 @@
> +	GspellTextView *spellcheck_view;
> +#endif
> +	gboolean      enable_spellchecking;
> 
> I could move this inside the ifdef, since we don't really use it when gspell
> is not available.
> 
> @@ +367,3 @@
> +
> +#if WITH_GSPELL
> +	window->spellcheck_view = NULL;
> 
> This is already NULL at this point, and you are setting it in the next line.
> 
> @@ +721,3 @@
> +{
> +        g_return_if_fail (EV_IS_ANNOTATION_WINDOW (window));
> +
> 
> Move the ifdef here and simply do nothing.
> 
> @@ +736,3 @@
> +ev_annotation_window_get_enable_spellchecking (EvAnnotationWindow *window)
> +{
> +        g_return_val_if_fail (EV_IS_ANNOTATION_WINDOW (window), FALSE);
> 
> And here add the ifdef else to return FALSE unconditionally in cae of gspell
> not being available, like you are doing in the view.
> 
> ::: libview/ev-view.c
> @@ +5734,3 @@
> +                n_pages = ev_document_get_n_pages (view->document);
> +
> +        for (current_page = 0; current_page<n_pages; current_page++) {
> 
> Add a space before and after the <
> 
> @@ +5735,3 @@
> +
> +        for (current_page = 0; current_page<n_pages; current_page++) {
> +
> 
> Remove this empty line.

Carlos,

Thank you so much for the review and the comments. I will definitely make sure to take this into account when I attempt to contribute further to evince.

Would you like me to make these changes and submit another version of the patch? I saw that you change the status of the patch, but didn't know what that. I'd be happy to make the changes if you'd like and it would save you some time. 

Just let me know!

Thanks again for helping me get this patch up to standards!

Will

(In reply to Will Hawkins from comment #45)
> [...]
> Would you like me to make these changes and submit another version of the
> patch? I saw that you change the status of the patch, but didn't know what
> that. I'd be happy to make the changes if you'd like and it would save you
> some time. 

"accepted-commit_now + comments" means: the reviewer trust you are going to make the changes before committing them, and those changes do not require further revision.

If you have commit access, then you make the changes, commit them, push them, and update the status in the bug. Otherwise, you update the patch, and upload the patch. Anyone with commit access will push them on your behalf, and update the status in the bug report.

(In reply to Germán Poo-Caamaño from comment #46)
> (In reply to Will Hawkins from comment #45)
> > [...]
> > Would you like me to make these changes and submit another version of the
> > patch? I saw that you change the status of the patch, but didn't know what
> > that. I'd be happy to make the changes if you'd like and it would save you
> > some time. 
> 
> "accepted-commit_now + comments" means: the reviewer trust you are going to
> make the changes before committing them, and those changes do not require
> further revision.
> 
> If you have commit access, then you make the changes, commit them, push
> them, and update the status in the bug. Otherwise, you update the patch, and
> upload the patch. Anyone with commit access will push them on your behalf,
> and update the status in the bug report.

German,

Thank you for clarifying that status. I will update the patch later this evening and submit a "final" version. I do not have commit access so I suppose that the "otherwise" clause of your second paragraph applies to me. 

Thanks again for explaining exactly what that status means! It's so helpful to have welcoming people like you to usher new contributers through the process.

Will

Created attachment 371789 [details] [review]
Add spell check for annotations (v6)

(In reply to Will Hawkins from comment #48)
> Created attachment 371789 [details] [review] [review]
> Add spell check for annotations (v6)

Thanks to Carlos for the most recent round of feedback. I believe that the version I just submitted addresses all his concerns and makes all the suggested changes. 

Again, thank you to everyone for guiding me through the process of adding some functionality to evince. I can't wait to get this committed so that I can start on some other outstanding bugs or feature requests!

Will

Pushed in master. Thanks for your contribution.

FWIW, I feel that we should not expose the setting in the UI.