After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 770356 - [Patch] Composer: replace Gtk.Action with GLib.action
[Patch] Composer: replace Gtk.Action with GLib.action
Status: RESOLVED FIXED
Product: geary
Classification: Other
Component: composer
unspecified
Other Linux
: Normal minor
: 0.12.0
Assigned To: Geary Maintainers
Geary Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-08-24 22:23 UTC by Niels De Graef
Modified: 2016-09-21 05:05 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Proposed patch (v1) (96.54 KB, patch)
2016-08-24 22:23 UTC, Niels De Graef
none Details | Review
Reworked patch (v2) (199.89 KB, patch)
2016-09-05 18:50 UTC, Niels De Graef
none Details | Review
Reworked patch (v3) (177.47 KB, patch)
2016-09-08 10:21 UTC, Niels De Graef
none Details | Review
Reworked patch (v4) (239.85 KB, patch)
2016-09-15 22:58 UTC, Niels De Graef
needs-work Details | Review
Fix for underline and strikethrough detection. (2.25 KB, patch)
2016-09-17 21:31 UTC, Niels De Graef
none Details | Review
Fix for context menu (2.25 KB, patch)
2016-09-18 10:57 UTC, Niels De Graef
none Details | Review
Fix for context menu (2.25 KB, patch)
2016-09-18 11:33 UTC, Niels De Graef
none Details | Review
Fix for context menu (for real this time) (4.08 KB, patch)
2016-09-18 11:58 UTC, Niels De Graef
none Details | Review
Context menu patched (11.22 KB, patch)
2016-09-21 01:46 UTC, Niels De Graef
none Details | Review
Context menu patched (without double separators) (11.67 KB, patch)
2016-09-21 02:53 UTC, Niels De Graef
none Details | Review

Description Niels De Graef 2016-08-24 22:23:15 UTC
Created attachment 334105 [details] [review]
Proposed patch (v1)

I got a bit frustrated with some ugly code while working on another bug, and to take my mind of things, I decided to take a first step in bug 713991 by replacing all Gtk.Action(Entry)s with GLib.Action(Entry)s. This should reduce the amount of deprecated warnings by quite a bit, and prepare us for future steps regarding the aforementioned bug.

I'm afraid this will be one big patch --sorry mjog :(-- as replacing the old kind of actions meant crashing a lot of the code in other composer-related widgets. I do think this should be thoroughly tested (I tried whatever I could to replicate the current behaviour). I also tried to add comments in several places where there was none to make it easier to follow.

See the commit message for more info.
Comment 1 Michael Gratton 2016-08-25 03:28:36 UTC
Review of attachment 334105 [details] [review]:

Hey, thanks heaps for looking into this. I've been itching to get rid of GtkAction!

The patch mostly looks good. The patch description should include this bug's number, now that it exists. Most of the comments relate to GAction use in ComposerWidget, but except for what to do about the action accels, most of it should be pretty trivial to address.

::: po/POTFILES.in
@@ +388,1 @@
 [type: gettext/glade]ui/composer.glade

Can you put the three composer lines in lexicographic order, per ui/CMakeLists.txt?

::: src/client/composer/composer-headerbar.vala
@@ +38,3 @@
+            detach_start_button.set_margin_end(6);
+            detach_end_button.set_margin_start(6);
+        }

I'd kind of expect GTK+ to do the right thing here automatically - if the margins were set in the .ui file so that it worked fine in LTR, I'd be surprised if it didn't also work fine RTL.

::: src/client/composer/composer-widget.vala
@@ +68,3 @@
+        {"composer.close",                    on_close                                },
+        {"composer.close and save",           on_close_and_save                       },
+        {"composer.close and discard",        on_close_and_discard                    },

GAction convention to use underscores rather than spaces, but I guess these are needed by on_action because they get sent to WebKit. This probably wants to be cleaned up by either sending the command as a GAction target value, or by having a GAction to WebKit command mapping, but I'm not too fussed about it at this stage.

@@ -68,3 @@
-    public const string ACTION_ADD_ORIGINAL_ATTACHMENTS = "add original attachments";
-    public const string ACTION_SELECT_DICTIONARY = "select dictionary";
-    

I'd like to see the use of the const strings retained - just having a static name, not keeping the specific values of the strings used here. I guess you got rid of them because in the changes below they often have prefixes and suffixes added, but if per the comment below you switch to using a SimpleActionGroup, then a fair bit of that should go away.

@@ +282,3 @@
+
+        // Add actions to the window once it's been added
+        this.hierarchy_changed.connect(() => initialize_actions());

Instead of this approach, this would probably be better handled by creating a SimpleActionGroup here, constructing and adding the actions to that, then setting it on the composer widget using insert_action_group().

There's an example of this in the new conversation-email.vala source file in the wip/765516-gtk-widget-conversation-viewer branch. This is also a good read: https://wiki.gnome.org/HowDoI/GAction

@@ +358,1 @@
+        header = new ComposerHeaderbar();

This can now be constructed when the property is declared rather than having to be in the ctor now, right?

@@ +584,3 @@
+        app.set_accels_for_action("win.composer.undo",                  {"<Ctrl>z"});
+        app.set_accels_for_action("win.composer.redo",                  {"<Ctrl><Shift>z"});
+        app.set_accels_for_action("win.composer.cut",                   {"<Ctrl>x"});

Using the SimpleActionGroup approach, you'd just use "compose.undo" here, and so on.

It's kind of annoying having to add composer actions to the top-level window like this though, for a few reasons:

 - They never get unset, so they're potentially hanging around and getting added multiple times
 - They may conflict with other app accelerators when the composer is embedded in the main window (e.g. Ctrl+A being used for both archive and select all.)

I think this was why ComposerContainer implementations had the focus-in/focus-out handlers that you removed. If there's no better way to do this then it might be worth restoring those and setting/unsetting the accels in the same way?

@@ +1610,3 @@
+        // We need the unprefixed name to send as a command to the editor
+        string[] prefixed_action_name = action.get_name().split(".");
+        string action_name = prefixed_action_name[prefixed_action_name.length - 1];

Would be much better to use Action.parse_detailed_name here if possible.

@@ +1764,3 @@
+        GLib.Menu compose_format_section = new GLib.Menu();
+        compose_format_section.append(_("_Rich Text"), "win.composer.compose_as_html");
+        menu.append_section(null, compose_format_section);

Is there a name clash? If not, don't bother using the GLib prefix here and elsewhere.

However, instead of constructing menus programmatically, using a UI file is probably a better way to go. Multiple menus can be packed into a single UI file and loaded when the widget is constructed. Again, see conversation-email.vala for an example of this. There's also a good read about these, too: https://developer.gnome.org/GMenu/

@@ +1775,3 @@
+        font_family_section.append(_("S_ans Serif"), "win.composer.font_family('sans')");
+        font_family_section.append(_("S_erif"), "win.composer.font_family('serif')");
+        font_family_section.append(_("_Fixed Width"), "win.composer.font_family('monospace')");

Ditto for this menu.

@@ +2001,3 @@
+                    if (get_action("composer.send").enabled)
+                        on_send(get_action("composer.send"), null);
+

This is a bit kludge - if both this method and the on_send handler need to execute the same code, move that that code out of on_send into a private method, and call the new method from both on_send and here.

If the on_send handler then becomes a one-liner, it would then be a good candidate to replace it altogether with a lambda connected to the action's signal handler, in the ctor.

@@ +2048,1 @@
         context_menu.append(undo);

Same comment about this menu as the ones above.

@@ +2169,3 @@
+    private SimpleAction get_action(string action_name) {
+        Gtk.ApplicationWindow window = get_toplevel() as Gtk.ApplicationWindow;
+        assert(window != null);

If using the SimpleActionGroup approach, it would be possible to just get the action directly from the group object.

@@ +2188,3 @@
+        get_action("composer.paste").set_enabled(editor.can_paste_clipboard());
+        get_action("composer.paste_with_formatting").set_enabled(editor.can_paste_clipboard() && compose_as_html);
+

Also, if using SimpleActionGroup and const action names, these and the other examples below can simply be retrieved from the group using the const names.

::: src/client/composer/email-entry.vala
@@ +63,2 @@
             empty = true;
             return;

Is this a behaviour change?
Comment 2 Niels De Graef 2016-09-05 18:50:39 UTC
Created attachment 334846 [details] [review]
Reworked patch (v2)

Reworked patch. Most of the remarks should be implemented.


>@@ +1610,3 @@
>+        // We need the unprefixed name to send as a command to the editor
>+        string[] prefixed_action_name = action.get_name().split(".");
>+        string action_name = prefixed_action_name[prefixed_action_name.length - >1];
>
>Would be much better to use Action.parse_detailed_name here if possible.
Action.parse_detailed_name() doesn't strip the prefix, it only separates the action name and the target value (which we don't need to do here).

>::: src/client/composer/email-entry.vala
>@@ +63,2 @@
>             empty = true;
>             return;
>
>Is this a behaviour change?
EmailEntry was only used in one place, in an if-condition that basically checked if each field was valid_or_empty() and not empty(), so I simplified it a bit, but the general behaviour should still be the same.
Comment 3 Michael Gratton 2016-09-06 01:00:21 UTC
Review of attachment 334846 [details] [review]:

I just took this for a spin. It seems to be working well, and it's great to see more of the UI code getting moved into UI files.

A few things to clean up that jumped straight out:

 * git am is reporting two trailing whitespace errors
 * POTFILES.in needs to be updated: composer.glade removed composer-widget.ui added
 * conversation-email.vala probably shouldn't be included

However, before doing a full review, the only composer keyboard accelerators that seem to be working are those for for bold & italic, for all three composer styles (windowed, embedded and full-hight). The other actions themselves seems to otherwise fine when activated by the mouse.
Comment 4 Niels De Graef 2016-09-08 10:21:54 UTC
Created attachment 335079 [details] [review]
Reworked patch (v3)

Hopefully the final changes.

FWIW: one of the problems was that the action naming convention uses dashes instead of underscores (see g_action_name_is_valid).
Comment 5 Michael Gratton 2016-09-09 13:44:50 UTC
Review of attachment 335079 [details] [review]:

Looking much better. Just a few thing that stick out with testing:

 - Ctrl+U doesn't invoke underline, but only when the composer is in the main window (maybe some collision?)
 - Strikethrough has some odd state bug - Ctrl+K will toggle the style but the action's button is not updated
 - Keyboard accels for indent quote (Ctrl+]) and outdent quote (Ctrl+[) are reversed (these may also need to be switched back when Geary is running in RTL mode in - you can test that out using the Gtk Inspector) 

The composer toolbar is now a plain Gtk.Box. One effect of this AFAIK is that when you click one of the formatting buttons, they stay focused, which is bad - focus should stay with the web view. IIRC if you use a Gtk.Toolbar instead this won't happen. Was there a reason why you used used a box instead?

The comments below for ComposerBox also apply to ComposerEmbed and ComposerWindow. It looks like there's a fair bit of common functionality between these classes, which would be worth moving to ComposerContainer and overriding only if needed.

::: src/client/composer/composer-box.vala
@@ +14,2 @@
     private Gee.Set<Geary.App.Conversation>? prev_selection = null;
+    private Gee.MultiMap<string, string> old_accelerators;

old_accellerators can be null, so the type needs a ? to make it nullable. Init it to be null here as well for clarity, like the line above.

@@ +17,3 @@
+    protected ComposerWidget composer {
+        get { return _composer; }
+    }

If the only reason why _composer is used here is to make it private, prefer this instead:

    protected ComposerWidget composer { get; set private; }

And you then don't need the _composer attr.

@@ +25,3 @@
     public ComposerBox(ComposerWidget composer) {
+        _composer = composer;
+

I'm trying to get into the habit of using "this" for accessing instance variables - it makes it immediately obvious in a method what is a local var and what is an instance attr/prop. So:

  this._composer = composer;

@@ +59,3 @@
+        top_window.present();
+    }
+

this.top_window, etc.

::: src/client/composer/composer-widget.vala
@@ +253,3 @@
+
+    public ComposerHeaderbar header = new ComposerHeaderbar();
+

This should be private set with default:

    ... { get; private set; default = ...; }

@@ +585,3 @@
+        insert_action_group("composer", actions);
+        header.insert_action_group("composer_header", actions);
+

I guess the two different action prefixes are needed for in-window composers?

The convention is to use three-char prefixes, which means using something like "cmp" and "chd" or whatever.
Comment 6 Niels De Graef 2016-09-15 22:58:59 UTC
Created attachment 335663 [details] [review]
Reworked patch (v4)

Latest patch, should solve most if not all of your previous remarks.

> - Keyboard accels for indent quote (Ctrl+]) and outdent quote (Ctrl+[) are 
> reversed (these may also need to be switched back when Geary is running in RTL
> mode in - you can test that out using the Gtk Inspector) 
As much as I would like to test this, when using RTL in my composer window, the text inside the webview still stays LTR, so I couldn't really make out whether this was the right thing.

> The composer toolbar is now a plain Gtk.Box. One effect of this AFAIK is that
> when you click one of the formatting buttons, they stay focused, which is bad
> focus should stay with the web view. IIRC if you use a Gtk.Toolbar instead this
> won't happen. Was there a reason why you used used a box instead?
Mainly because I have no idea how to add a label directly to a toolbar (if it is even possible), but also that it was hard to keep the current styling of the buttons (the "inline-toolbar"-class came close, but that gave some other issues). I solved the focusing problem by setting can_focus on the buttons to false, rendering the same effect.

> I'm trying to get into the habit of using "this" for accessing instance
> variables - it makes it immediately obvious in a method what is a local var 
> and what is an instance attr/prop.
I've tried to do this as much as possible now (even in the ComposerWidget), but since it was a lot of editing, some might've slipped by (it tends to be a  brain-numbing task after a while).
Comment 7 Michael Gratton 2016-09-17 02:05:52 UTC
Review of attachment 335663 [details] [review]:

Nearly there! Two feature bugs remain:

 * The context menu isn't getting invoked on right-click - we're getting the standard WebKitGTK one instead
 * Some weirdness between underline and strike-through remains:
    - Type some text, click Underline, then click Strike-through: The text remains underlined, but the Underline button becomes un-toggled.
    - Click Strike-through again, the Underline button becomes toggled again
    - The opposite seems to work fine (Strike-through then Underline)
Comment 8 Niels De Graef 2016-09-17 21:31:24 UTC
Created attachment 335780 [details] [review]
Fix for underline and strikethrough detection.

> * Some weirdness between underline and strike-through remains:
>    - Type some text, click Underline, then click Strike-through: The text 
>          remains underlined, but the Underline button becomes un-toggled.
>    - Click Strike-through again, the Underline button becomes toggled again
>    - The opposite seems to work fine (Strike-through then Underline)
The attached patch should fix that. It was quite trivial so I didn't want to force you to review the whole huge patch again.

> * The context menu isn't getting invoked on right-click - we're getting the 
> standard WebKitGTK one instead
Strange, for me it seems to work just fine...
Comment 9 Michael Gratton 2016-09-18 06:15:35 UTC
The second patch does indeed fix the uline/s-through issues. Ta.

So about the context menu, I'm not sure what I was thinking before, but it does indeed seem to be using the custom context menu. There are a few minor issues with it though which maybe was where the mis-understanding was arising.

 * Spelling suggestions aren't being added to the top: Ensure a spellcheck language is selected, mis-type a word, add a space to get the squiggly red line, right click on it, note that no correction suggestions are present at the top.

 * "Paste with Formatting" is present when not in Rich Text mode, which it shouldn't be. (Copy Link shouldn't be there either in plan text mode, although that wasn't introduced by this patch)

Another small follow-up patch to address this would be great as well.
Comment 10 Niels De Graef 2016-09-18 10:57:28 UTC
Created attachment 335798 [details] [review]
Fix for context menu

The patch should fix it.
Comment 11 Michael Gratton 2016-09-18 11:22:28 UTC
I think you accidentally re-attached the underline/strike-out patch. :)
Comment 12 Niels De Graef 2016-09-18 11:33:08 UTC
Created attachment 335800 [details] [review]
Fix for context menu

Ah crap. My bad!
Comment 13 Michael Gratton 2016-09-18 11:55:06 UTC
Err, yeah that's the same one again? :)
Comment 14 Niels De Graef 2016-09-18 11:58:57 UTC
Created attachment 335801 [details] [review]
Fix for context menu (for real this time)

Well derp, really really sorry 'bout that. Third time's a charm I guess.
Comment 15 Niels De Graef 2016-09-21 01:46:11 UTC
Created attachment 335959 [details] [review]
Context menu patched

The context menu should be working now. As discussed on IRC, the Menu is transformed into a Gtk.Menu in a way that we can easily change this to a WebkitContextMenu in WK2.

Documenting changed behaviour just in case:
We now (correctly) show the 'Ignore spelling' and 'Learn spelling' when right-clicking on a misspelled word. Previously, it was necessary to first select the word.
Comment 16 Niels De Graef 2016-09-21 02:53:55 UTC
Created attachment 335960 [details] [review]
Context menu patched (without double separators)

The same patch as earlier, but without the double borders.
Comment 17 Michael Gratton 2016-09-21 05:05:32 UTC
Landed on master as ebd7889, 3670211 and 56c10ac. Thanks for working through all the issues with this!