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 741050 - Migrate to GAction
Migrate to GAction
Status: RESOLVED FIXED
Product: eog
Classification: Core
Component: general
3.14.x
Other Linux
: Normal normal
: ---
Assigned To: EOG Maintainers
EOG Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-12-03 00:07 UTC by Jente Hidskes
Modified: 2015-02-02 19:44 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Port eog-properties-dialog to GAction (3.08 KB, patch)
2014-12-11 19:15 UTC, Jente Hidskes
rejected Details | Review
Port eog-window.c to GAction (82.41 KB, patch)
2014-12-11 19:20 UTC, Jente Hidskes
reviewed Details | Review
Updated patch 0002 as per Felix' suggestions. (81.09 KB, patch)
2014-12-15 22:27 UTC, Jente Hidskes
committed Details | Review
Remove all code related to GtkToolbr (22.72 KB, patch)
2014-12-15 23:50 UTC, Jente Hidskes
committed Details | Review
Remove all other references to the toolbar (130.23 KB, patch)
2014-12-15 23:53 UTC, Jente Hidskes
committed Details | Review
This patch removes the GtkMenuBar (11.14 KB, patch)
2014-12-16 20:21 UTC, Jente Hidskes
committed Details | Review
Disable currently unneeded code (8.07 KB, patch)
2014-12-18 18:45 UTC, Jente Hidskes
committed Details | Review
Remove the string matching from the settings callback (3.63 KB, patch)
2014-12-18 18:46 UTC, Jente Hidskes
committed Details | Review
Change all GAction names to short, lowercase names (23.14 KB, patch)
2014-12-18 18:49 UTC, Jente Hidskes
committed Details | Review
Add accelerators to GActions. (4.01 KB, patch)
2014-12-20 01:17 UTC, Jente Hidskes
none Details | Review
Add accelerators to GActions. (4.00 KB, patch)
2014-12-21 00:55 UTC, Jente Hidskes
committed Details | Review
Remove zoom-fit action. (1.58 KB, patch)
2014-12-22 23:03 UTC, Jente Hidskes
committed Details | Review
Migrate EogPropertiesDialog to GAction. (5.12 KB, patch)
2014-12-23 22:11 UTC, Jente Hidskes
committed Details | Review
Implement "groupwise" enabling/disabling of actions (4.69 KB, patch)
2014-12-26 19:49 UTC, Jente Hidskes
none Details | Review
Implement "groupwise" enabling/disabling of actions (4.46 KB, patch)
2014-12-29 19:44 UTC, Jente Hidskes
committed Details | Review
Make Recent Images submenu work (10.96 KB, patch)
2015-01-17 17:35 UTC, Jente Hidskes
none Details | Review
Make Recent Images submenu work (11.32 KB, patch)
2015-01-17 19:23 UTC, Jente Hidskes
none Details | Review
Make Open With submenu work (13.33 KB, patch)
2015-01-17 22:37 UTC, Jente Hidskes
none Details | Review
Remove the Recent Images submenu (10.49 KB, patch)
2015-01-19 23:02 UTC, Jente Hidskes
committed Details | Review
Make Open With submenu work (15.57 KB, patch)
2015-01-31 17:36 UTC, Jente Hidskes
committed Details | Review

Description Jente Hidskes 2014-12-03 00:07:06 UTC
The design update for Eye-of-Gnome will require the switch from GtkAction to GAction, as discussed in bug 740426. I have already started working on this and have made a bit of progress in the main code, but I have no idea how much work it will be to port the plugins. I think I can finish the main code in not too long, depending on how occupied university will keep me.

As mentioned by Felix in bug 740426, perhaps it is an idea to have this ticket block bug 740426.
Comment 1 Felix Riemann 2014-12-05 18:32:55 UTC
As this is going to be part of a larger effort, the plugins shouldn't block you.
We can try handling those in a second step if necessary.
Comment 2 Jente Hidskes 2014-12-11 19:15:54 UTC
Created attachment 292562 [details] [review]
Port eog-properties-dialog to GAction

This patch is the first of a series that will migrate Eye-of-Gnome to GAction
from GtkAction. It takes care of the migration inside the properties dialog.
Comment 3 Jente Hidskes 2014-12-11 19:20:14 UTC
Created attachment 292564 [details] [review]
Port eog-window.c to GAction

This patch is the second in a series that intends to port Eye of Gnome to
GAction. It replaces all the GtkAction related code in eog-window.c to their
GAction equivalents.

Some things to note:

    1) I only changed the lines required to change, to keep the diff as small
       as possible. I do think some parts in eog-window.c can do with some
       cleanup and a consistent style needs to be applied. (I tried to match up
       with whatever style was used in the function I was working in.)
    2) I renamed _some) functions and arguments to better indicate what they're
       for, but only those lines I already touched anyway.
    3) actions_window is now directly mapped onto the
       GtkApplicationWindow/EogWindow.
    4) We might want to change the action names to short, lowercase names as the
       documentation suggests.
    5) The UI is not working anymore, due to incompatibility: it requires
       GtkAction. This left me unable to test some/most of the changes, however
       I quickly added some accelerators to those actions I could and it all
       seems to be working so I have faith the other changes will to.
    6) The appmenu does not trigger changes in the UI, although the binding
       mapping between GSettings and the related Gaction does get triggered.

What's left to do:

    1) Fix the app-menu.
    2) Test all changes.
    3) Remove deprecated and not-working UI code.
    4) Add accelerators to all actions (maybe requires to re-think the splitting
       of actions into different GActionEntries - I'm not sure if we can add
       accelerators to the code as is).

Excuse me for the long time to submit these patches. I had quite a few deadlines at university this week. I also apologise for leaving the app-menu in a non-working state but I'm at a loss as to what's wrong here. As the patch is way too long already, I thought I'd submit it while I keep digging to give you some extra time for a review.

As for the UI, I'm not sure if I can rip out the elements that are not working anymore (menubar, toolbar, toolbar edit dialog). These are the only elements that spit erros during runtime due to my changes.

I look forward to your reply and feedback and again, apologies for the huge diff (but I couldn't find a way to properly split this and most changes are straightforward)
Comment 4 Felix Riemann 2014-12-14 17:46:45 UTC
Review of attachment 292562 [details] [review]:

I don't think it will work that way. While you correctly switch the dialog's properties from GtkAction to GAction, there's AFAIK no way to assign a GAction to a GtkButton directly. In fact if you look at the dialog's set_property function it will still try to assign a GtkAction. That's why the buttons in the dialog do nothing currently.

The dialog's interface most likely needs some changes for this to work. Looking at how GtkActionable works a first approach might be to not pass the exact actions but simply construct the dialog with a parent window parameter (which is also the GActionGroup) and hardcode the action names or add parameters for those.
Comment 5 Felix Riemann 2014-12-14 21:53:11 UTC
Review of attachment 292564 [details] [review]:

(In reply to comment #3)
> Created an attachment (id=292564) [details] [review]
> Port eog-window.c to GAction
> 
> This patch is the second in a series that intends to port Eye of Gnome to
> GAction. It replaces all the GtkAction related code in eog-window.c to their
> GAction equivalents.
> 
> Some things to note:
> 
>     1) I only changed the lines required to change, to keep the diff as small
>        as possible. I do think some parts in eog-window.c can do with some
>        cleanup and a consistent style needs to be applied. (I tried to match up
>        with whatever style was used in the function I was working in.)

Yes. EogWindow is a pretty huge piece of code, which is also because it grew in size over the years. There are several parts where eog needs some rearchitecturing. EogWindow's last rework was for eog 2.20 (which was in 2007)

>     2) I renamed _some) functions and arguments to better indicate what they're
>        for, but only those lines I already touched anyway.

Ok!

>     3) actions_window is now directly mapped onto the
>        GtkApplicationWindow/EogWindow.

Good!

>     4) We might want to change the action names to short, lowercase names as
> the
>        documentation suggests.

Yes. :)

>     5) The UI is not working anymore, due to incompatibility: it requires
>        GtkAction. This left me unable to test some/most of the changes, however
>        I quickly added some accelerators to those actions I could and it all
>        seems to be working so I have faith the other changes will to.

You can try using GtkInspector to trigger application- and window-wide actions.

>     6) The appmenu does not trigger changes in the UI, although the binding
>        mapping between GSettings and the related Gaction does get triggered.

Well, the menu does work. It's just the stateful actions that don't.

I took a look and apparently changing the state via the "state" property skips the state-changed handler.
Using a simple callback instead of the binding that uses g_simple_action_change_state worked.

> What's left to do:
> 
>     1) Fix the app-menu.
>     2) Test all changes.
>     3) Remove deprecated and not-working UI code.
>     4) Add accelerators to all actions (maybe requires to re-think the
> splitting
>        of actions into different GActionEntries - I'm not sure if we can add
>        accelerators to the code as is).
> 
> Excuse me for the long time to submit these patches. I had quite a few
> deadlines at university this week. I also apologise for leaving the app-menu in
> a non-working state but I'm at a loss as to what's wrong here. As the patch is
> way too long already, I thought I'd submit it while I keep digging to give you
> some extra time for a review.

No need for apologies. Nobody was setting you deadlines for the patches.

> 
> As for the UI, I'm not sure if I can rip out the elements that are not working
> anymore (menubar, toolbar, toolbar edit dialog). These are the only elements
> that spit erros during runtime due to my changes.

Since we plan to move to a GtkHeaderBar with menu if have no objections against removing the menu and the toolbar. Just put that in separate commits (1 for the menu and 1 or 2 for the editable toolbar).

> 
> I look forward to your reply and feedback and again, apologies for the huge
> diff (but I couldn't find a way to properly split this and most changes are
> straightforward)

Well, the diff's long but we're talking about EogWindow here and ripping out GtkAction, so it's okay in my opinion.

::: src/eog-window.c
@@ +677,2 @@
+		/*gtk_action_group_set_sensitive (window, TRUE);
+		gtk_action_group_set_sensitive (priv->actions_image,  TRUE);*/

Okay, I see we have to look for a way to handle these bulk enabling/disablings.
Or do you handle this already?

@@ +4202,1 @@
 };

Some of these image actions already have duplicate window actions and thus wouldn't be needed here.
Also actions like "ViewFullscreen" are more window actions than image actions.

We should probably merge the useful actions into the window actions as well and disable the others for now.

@@ +4208,3 @@
+	{ "GoFirst",    eog_window_action_go_first },
+	{ "GoLast",     eog_window_action_go_last },
+	{ "GoRandom",   eog_window_action_go_random },

These should also just be the ones from the window actions list.

@@ +4211,3 @@
+
+	/* Stateful actions. */
+	{ "ViewSlideshow", NULL, NULL, "false", eog_window_action_toggle_slideshow },

Can easily be merged into the window actions list.

@@ +4307,3 @@
+								  G_SETTINGS_BIND_GET,
+								  _settings_map_get_bool_variant,
+								  NULL, NULL, NULL);

Indentation drifting away.
Comment 6 Felix Riemann 2014-12-14 21:57:21 UTC
(In reply to comment #3)
> What's left to do:
> ...
>     4) Add accelerators to all actions (maybe requires to re-think the
> splitting
>        of actions into different GActionEntries - I'm not sure if we can add
>        accelerators to the code as is).

Well, as there seems to be no function to disable/enable a whole GActionGroup there's not much use in splitting the actions into multiple groups. If that makes it easier for you, I'd go that way.
Comment 7 Jente Hidskes 2014-12-15 22:27:27 UTC
Created attachment 292778 [details] [review]
Updated patch 0002 as per Felix' suggestions.

This new revision of patch 0002 fixes the app-menu as per Felix' suggestion. It also moves all GActions into one single GActionEntry that is mapped onto the GtkApplicationWindow as a whole, removing duplicate entries that were spread over the different GActionEntries that existed before.

To spare you from having to review the huge diff all over again, I made a diff of the diff that only holds the changes I just made. It can be found here: https://gist.github.com/Unia/0d04e93c44804b07df41

I will now continue the work, but those will be delivered in separate diffs. Thanks for the review!
Comment 8 Jente Hidskes 2014-12-15 23:50:15 UTC
Created attachment 292783 [details] [review]
Remove all code related to GtkToolbr

This patch removes all code for the GtkToolbar inside the src/ directory.
Comment 9 Jente Hidskes 2014-12-15 23:53:19 UTC
Created attachment 292784 [details] [review]
Remove all other references to the toolbar

This patch removes all further references to the toolbar. In hindsight, I think the two patches combined would've worked just fine.

I did not touch the files in help/ or po/, as I am unsure if those are to be touched by me at the moment. If so, I can edit this patch and take care of that.

By the way, is there a way to attach multiple attachments to one comment? It's getting kind of chaotic (or it could just be me working with Bugzilla for the first time).
Comment 10 Jente Hidskes 2014-12-16 20:21:03 UTC
Created attachment 292858 [details] [review]
This patch removes the GtkMenuBar

This patch removes the GtkMenuBar, as discussed.

I left the "open with" and "recently opened" menus in, because those are going to be used in the new design, aren't they? If not, I can update the patch to remove these as well.

I notice that some tests use the menu. I haven't changed those yet, since I'm unsure how they work exactly and I think it's easier to edit them after the UI has been updated as well. Again, I can easily update the patch to change this to whatever you see fit.

I will now wait with implementing the remaining todo's listed in comment 3, because otherwise there will be a tad too much work going on simultaneously.
Comment 11 Felix Riemann 2014-12-16 21:25:05 UTC
Review of attachment 292858 [details] [review]:

Allright, can't seem to spot anything bad on this one. :)

This seems to have the others as prerequisite?
Comment 12 Jente Hidskes 2014-12-16 21:26:57 UTC
Yes, all patches are posted in the order they are created. Is this a problem? (I wouldn't know how to do it differently, without creating conflicts)
Comment 13 Felix Riemann 2014-12-16 21:47:00 UTC
(In reply to comment #10)
> I left the "open with" and "recently opened" menus in, because those are going
> to be used in the new design, aren't they? If not, I can update the patch to
> remove these as well.

For now they are part of Allan's designs. However, we will have to see how nicely they integrate with the new widgets/layout.

We can disable the code for now I guess, as they are written for GtkUIManager (and thus GtkAction). I wouldn't change your patches just for that though.
 
> I notice that some tests use the menu. I haven't changed those yet, since I'm
> unsure how they work exactly and I think it's easier to edit them after the UI
> has been updated as well. Again, I can easily update the patch to change this
> to whatever you see fit.

Thanks, for reminding me of those. They will indeed need to be adjusted. Since the UI design is not final yet, we should correct them when we are done. I just hope they won't cause too much noise in GNOME Continous until then.


(In reply to comment #9)
> Created an attachment (id=292784) [details] [review]
> Remove all other references to the toolbar
> 
> This patch removes all further references to the toolbar. In hindsight, I think
> the two patches combined would've worked just fine.
> 
> I did not touch the files in help/ or po/, as I am unsure if those are to be
> touched by me at the moment. If so, I can edit this patch and take care of
> that.

Regarding /po, correcting the Makefiles or the POTFILES.* is something you might have to do to keep the build working, so do it if you have to. Touching the *.po files is usually not necessary as intltool should take care of removing unused strings when the translations are updated the next time.
You might be able to see this on l10n.gnome.org once your patches are committed.

Updating the help files can be post-poned until later, since they will need some adaption to the new UI anyways.

> By the way, is there a way to attach multiple attachments to one comment? It's
> getting kind of chaotic (or it could just be me working with Bugzilla for the
> first time).

No, one file per comment in Bugzilla.
There's a script on the wiki that can push patches straight from your working copy: https://wiki.gnome.org/Git/WorkingWithPatches#How_to_attach_a_patch_to_a_bug_in_Bugzilla

Haven't used it myself though.
Comment 14 Felix Riemann 2014-12-16 22:19:38 UTC
Review of attachment 292784 [details] [review]:

::: data/org.gnome.eog.gschema.xml.in
@@ +116,1 @@
     <key name="external-editor" type="s">

This is just a reminder for myself to check whether this key is still needed once we're done.

::: help/Makefile.am
@@ -49,3 @@
 	shortcuts.page				\
 	slideshow.page				\
-	toolbar.page				\

Let's not remove that page yet, as it could produce some broken cross-references from other pages.
This can be fixed while committing your patch though.
Comment 15 Felix Riemann 2014-12-16 22:19:38 UTC
Review of attachment 292784 [details] [review]:

::: data/org.gnome.eog.gschema.xml.in
@@ +116,1 @@
     <key name="external-editor" type="s">

This is just a reminder for myself to check whether this key is still needed once we're done.

::: help/Makefile.am
@@ -49,3 @@
 	shortcuts.page				\
 	slideshow.page				\
-	toolbar.page				\

Let's not remove that page yet, as it could produce some broken cross-references from other pages.
This can be fixed while committing your patch though.
Comment 16 Felix Riemann 2014-12-16 22:27:41 UTC
Review of attachment 292783 [details] [review]:

Seems to be allright as well.
I'll take care of the one point when checking it in.

::: src/eog-window.c
@@ -4606,3 @@
 static void
-eog_window_open_editor (GAction *action,
-                        EogWindow *window)

I think we should keep this one, albeit disabled, until we know whether the external-editor feature is coming back in the new design.
Comment 17 Felix Riemann 2014-12-16 22:44:17 UTC
Review of attachment 292778 [details] [review]:

::: src/eog-window.c
@@ +4865,1 @@
 					     G_BINDING_SYNC_CREATE,

We'll have to check whether that works or has the same problem as the bound GSettings.

@@ +4966,3 @@
+	g_signal_connect (priv->ui_settings, "changed",
+					  G_CALLBACK (eog_window_ui_settings_changed_cb),
+					  window);

I think it should work to pass the necessary setting as signal detail to the signal and use the action as user_data. That way you wouldn't have to do string matching on the action name in the signal handler.
Comment 18 Felix Riemann 2014-12-16 23:07:52 UTC
Okay, I'll see that I can get it pushed to git tomorrow.
Not sure yet whether to go directly to master or put it in a wip-branch until we're done.
Comment 19 Jente Hidskes 2014-12-17 17:31:45 UTC
That's great to hear, thanks!

Here's a list of what is left to do now (made mostly because I lost oversight):

1) Fix the eog-properties-dialog.c (attachment 292562 [details] [review], comment 4).
2) Disable the "open-with" and "recently-used" menus (comment 13) (after or before the merge?).
3) Fix the UI tests once the UI is finished (comment 13).
4) Update the help files once the UI is finished (comment 13).
5) Fix the things pointed out in comment 17 (after or before the merge?).

And the leftover todos from attachment 292778 [details] [review]:

6) We might want to change the action names to short, lowercase names as the documentation suggests.
7) Test all changes.
8) Add accelerators to all actions.
Comment 20 Felix Riemann 2014-12-17 19:12:00 UTC
Pushed to branch wip/gaction-migration. :)
Comment 21 Felix Riemann 2014-12-17 21:06:54 UTC
Sorry, had to repush the branch as I accidentally pushed the changes to master as well. :x

Also, hooked up the right-click menu to GAction/GtkBuilder which allows testing a few actions already.
Comment 22 Jente Hidskes 2014-12-18 18:45:52 UTC
Created attachment 292996 [details] [review]
Disable currently unneeded code
Comment 23 Jente Hidskes 2014-12-18 18:46:35 UTC
Created attachment 292997 [details] [review]
Remove the string matching from the settings callback
Comment 24 Jente Hidskes 2014-12-18 18:49:50 UTC
Created attachment 292998 [details] [review]
Change all GAction names to short, lowercase names

I assume the naming is subject to opinion and stuff, so feel free to rename anything as you see fit.

Also, I noticed there was a name clash between two actions. There were to actions named "ViewZoomFit", both with a different callback:

{ "ViewZoomFit", eog_window_action_zoom_best_fit },
{ "ViewZoomFit", NULL, NULL, "true",  eog_window_action_toggle_zoom_fit },

As you can see in the patch, I sorted it out but I am unsure whether I renamed the usage of those ViewZoomFit actions correctly. You will probably want to look into this.
Comment 25 Jente Hidskes 2014-12-18 18:51:44 UTC
While testing some changes just now, I noticed two things that aren't working:

1) Setting the wallpaper. I don't see any errors in the terminal, but it just doesn't work. If you open the background settings after having changed the wallpaper via Eye of Gnome, you will see the default blue background. The key also does not change in dconf-editor.

2) When closing the sidebar from within Eye of Gnome, the toggle action in the app-menu doesn't change.
Comment 26 Felix Riemann 2014-12-18 20:26:46 UTC
Thanks Jente, I'll take a look at the new ones.

(In reply to comment #25)
> While testing some changes just now, I noticed two things that aren't working:
> 
> 1) Setting the wallpaper. I don't see any errors in the terminal, but it just
> doesn't work. If you open the background settings after having changed the
> wallpaper via Eye of Gnome, you will see the default blue background. The key
> also does not change in dconf-editor.

Yes, I think this could be related to the JHBuild sandbox. It works when I start eog from outside the sandbox. Let's have an eye on this.

> 2) When closing the sidebar from within Eye of Gnome, the toggle action in the
> app-menu doesn't change.

Got that one already. :)
You were setting the action state using set_state() which doesn't call you change-state handler. I know now that g_simple_action_set_state is usually not what you want outside of change-state handlers. You should use g_action_change_state instead. Pretty much the same problem as with the GSettings binding. I'll fix that on push.

(In reply to comment #24)
> Created an attachment (id=292998) [details] [review]
> Change all GAction names to short, lowercase names
> 
> I assume the naming is subject to opinion and stuff, so feel free to rename
> anything as you see fit.
> 
> Also, I noticed there was a name clash between two actions. There were to
> actions named "ViewZoomFit", both with a different callback:
> 
> { "ViewZoomFit", eog_window_action_zoom_best_fit },
> { "ViewZoomFit", NULL, NULL, "true",  eog_window_action_toggle_zoom_fit },
> 
> As you can see in the patch, I sorted it out but I am unsure whether I renamed
> the usage of those ViewZoomFit actions correctly. You will probably want to
> look into this.

Allright, I'll check what's going on. The duplicate action could be a leftover from when the best-fit zoom was converted to a toggle (best-fit <-> free zoom).
Comment 27 Felix Riemann 2014-12-18 20:40:11 UTC
Review of attachment 292996 [details] [review]:

Alright, I'll push that one with some adjustments to work with the changes I made in the meantime.

Two advises for the commit-message. If you want to show that you worked on a specific class don't prefix the path to the filename but use the class instead. In this case just do it this way: "EogWindow: disable ...".
And you don't have to tell that you include a "Bugzilla link", just putting the link at the end of the message is fine.
Comment 28 Jente Hidskes 2014-12-18 21:00:32 UTC
(In reply to comment #26)
> 
> > 2) When closing the sidebar from within Eye of Gnome, the toggle action in the
> > app-menu doesn't change.
> 
> Got that one already. :)
> You were setting the action state using set_state() which doesn't call you
> change-state handler. I know now that g_simple_action_set_state is usually not
> what you want outside of change-state handlers. You should use
> g_action_change_state instead. Pretty much the same problem as with the
> GSettings binding. I'll fix that on push.
> 

I figured it was something like this. Have you noticed any other places where we should change set_state to change_state? Or shall I go ahead and to them all to be sure?

Cheers for the advice :)
Comment 29 Felix Riemann 2014-12-19 16:11:42 UTC
(In reply to comment #27)
> Review of attachment 292996 [details] [review]:
> 
> Alright, I'll push that one with some adjustments to work with the changes I
> made in the meantime.
> 
> Two advises for the commit-message. If you want to show that you worked on a
> specific class don't prefix the path to the filename but use the class instead.
> In this case just do it this way: "EogWindow: disable ...".
> And you don't have to tell that you include a "Bugzilla link", just putting the
> link at the end of the message is fine.

Hmm, just noticed it's not that simple. Now, that I have dconf working in my JHBuild closing the sidebar will throw eog into an endless gsettings<->gaction feedback loop that will make the stack overflow. :(
Comment 30 Felix Riemann 2014-12-19 18:11:44 UTC
Allright, found the problem.

(In reply to comment #24)
> Also, I noticed there was a name clash between two actions. There were to
> actions named "ViewZoomFit", both with a different callback:
> 
> { "ViewZoomFit", eog_window_action_zoom_best_fit },
> { "ViewZoomFit", NULL, NULL, "true",  eog_window_action_toggle_zoom_fit },
> 
> As you can see in the patch, I sorted it out but I am unsure whether I renamed
> the usage of those ViewZoomFit actions correctly. You will probably want to
> look into this.

The non-toggle variant was a part of the d-bus interface which allowed switching to best fit zoom. However the same is possible with the toggle action now, so the non-toggle one can probably be removed in favour of the toggle action.

(In reply to comment #28) 
> I figured it was something like this. Have you noticed any other places where
> we should change set_state to change_state? Or shall I go ahead and to them all
> to be sure?

Well, I am not so sure about the ones in the update_ui_visibility and update_action_groups_state at the moment. The logic is a bit odd there.
The others look okay to me.
Comment 31 Jente Hidskes 2014-12-19 23:38:30 UTC
(In reply to comment #26)
> Yes, I think this could be related to the JHBuild sandbox. It works when I
> start eog from outside the sandbox. Let's have an eye on this.

Ah, yes. When manually launching eog it indeed works. I guess all is well then.

(In reply to comment #30)
> The non-toggle variant was a part of the d-bus interface which allowed
> switching to best fit zoom. However the same is possible with the toggle action
> now, so the non-toggle one can probably be removed in favour of the toggle
> action.

I see. So do I remove or comment it? Does it require more changes elsewhere?

> Well, I am not so sure about the ones in the update_ui_visibility and
> update_action_groups_state at the moment. The logic is a bit odd there.
> The others look okay to me.

I wasn't sure about those in update_ui_visibility from the start, considering the original code took the actions from the GtkUIManager and used those solely to change the GtkToggle to the correct value (see here for an example: https://git.gnome.org/browse/eog/tree/src/eog-window.c#n1972 ). I guess that code was there to sync the toggles with the settings? If so, we can remove those lines altogether.

The code in update_action_groups_state needs work as well. We still need to disable the relevant actions manually. I'm not sure about the only g_simple_action_set_state call here.
Comment 32 Jente Hidskes 2014-12-20 01:17:23 UTC
Created attachment 293119 [details] [review]
Add accelerators to GActions.

One problem is that adding multiple accelerators to the same action, overrides the previously set accelerators. I'm not sure what the best way is to work around the issue, but I thought I would discuss having several accelerators for the same action before looking for a fix: I don't think it's required to, for example, have both "Home" and "<Alt>Home" for the "go-first" action.

I'll leave this decision up to you, though. If I do need to add multiple accelerators, what do you think is the best way to go about this?
Comment 33 Felix Riemann 2014-12-20 13:55:02 UTC
(In reply to comment #31)
> (In reply to comment #30)
> > The non-toggle variant was a part of the d-bus interface which allowed
> > switching to best fit zoom. However the same is possible with the toggle action
> > now, so the non-toggle one can probably be removed in favour of the toggle
> > action.
> 
> I see. So do I remove or comment it? Does it require more changes elsewhere?

Activating the toggle action has the same effect, except that it would switch back to free zooming if it already were set to best fit. But that's easily fixed by changing the state to the appropriate value or checking the state beforehand. Considering the presumably low number of users for that interface I'd say let's remove the non-toggle variant.

(In reply to comment #32)
> Created an attachment (id=293119) [details] [review]
> Add accelerators to GActions.
> 
> One problem is that adding multiple accelerators to the same action, overrides
> the previously set accelerators. I'm not sure what the best way is to work
> around the issue, but I thought I would discuss having several accelerators for
> the same action before looking for a fix: I don't think it's required to, for
> example, have both "Home" and "<Alt>Home" for the "go-first" action.
> 
> I'll leave this decision up to you, though. If I do need to add multiple
> accelerators, what do you think is the best way to go about this?

The documentation for set_accels_for_action (https://developer.gnome.org/gtk3/stable/GtkApplication.html#gtk-application-set-accels-for-action) indicates that you can pass more than one accelerator for each action. Maybe you can add a eog_application_add_accelerators (in addition to the add_accelerator you already have) function that builds the accelerator list via some vararg parameters.

Regarding the duplicates, IIRC most have their origin in the GNOME HIG Guidelines (https://developer.gnome.org/hig-book/3.12/input-keyboard.html#standard-shortcuts) and some have duplicates because the user cannot distinguish between the keys (like '+' and KP_Plus). So let's keep them for now. There's probably some more duplicates hidden in the widget's button_press handlers, that we should take a look at once things are working stable again.
Comment 34 Jente Hidskes 2014-12-21 00:55:31 UTC
Created attachment 293139 [details] [review]
Add accelerators to GActions.

This version does support multiple accelerators per action. I tried implementing what you suggested, but the best I could do was to generate a "gchar **" that held pointers to duplicated strings which I got from va_arg. However, gtk_application_set_accels_for_action expects to receive a "const gchar * const*" and so nothing was ever mapped correctly.

This new way might look ugly, but it works. I know I'm crossing the 80/100 column, but if I break the lines it starts looking like spaghetti even more.

Well, I look forward to your reply on this one... :p
Comment 35 Felix Riemann 2014-12-21 14:40:15 UTC
Review of attachment 293139 [details] [review]:

(In reply to comment #34)
> Created an attachment (id=293139) [details] [review]
> Add accelerators to GActions.
> 
> This version does support multiple accelerators per action. I tried
> implementing what you suggested, but the best I could do was to generate a
> "gchar **" that held pointers to duplicated strings which I got from va_arg.
> However, gtk_application_set_accels_for_action expects to receive a "const
> gchar * const*" and so nothing was ever mapped correctly.

Hmm, odd that it wouldn't work. AFAIK set_accels copies the values passed in, so it shouldn't have a problem being given a dynamic allocated list.

> This new way might look ugly, but it works. I know I'm crossing the 80/100
> column, but if I break the lines it starts looking like spaghetti even more.

Wow, so much casting. :)
I took a look around some other GNOME apps as to how they solved this. Well, assigning multiple accelerators doesn't seem to be so common.
In the end I found a pretty neat solution/hack in Evince that I will adapt and which makes the code (or actually the assignments) a bit easier to grasp.
Comment 36 Jente Hidskes 2014-12-22 23:03:51 UTC
Created attachment 293211 [details] [review]
Remove zoom-fit action.

This patch removes the zoom-fit action, as discussed here and pointed out in comment #33.
Comment 37 Jente Hidskes 2014-12-22 23:04:10 UTC
That Evince-derived solution is indeed way more elegant - and so simple, I feel stupid for not having thought of it myself!

> > Well, I am not so sure about the ones in the update_ui_visibility and
> > update_action_groups_state at the moment. The logic is a bit odd there.
> > The others look okay to me.
> 
> I wasn't sure about those in update_ui_visibility from the start, considering
> the original code took the actions from the GtkUIManager and used those solely
> to change the GtkToggle to the correct value (see here for an example:
> https://git.gnome.org/browse/eog/tree/src/eog-window.c#n1972 ). I guess that
> code was there to sync the toggles with the settings? If so, we can remove
> those lines altogether.
> 
> The code in update_action_groups_state needs work as well. We still need to
> disable the relevant actions manually. I'm not sure about the only
> g_simple_action_set_state call here.

Have you taken a look at these yet? I'm going to work on the properties dialog now and soon those will be the remaining issues, together with the G_BINDING_SYNC_CREATE inside eog_window_construct_ui (https://git.gnome.org/browse/eog/tree/src/eog-window.c?h=wip%2Fgaction-migration#n4503) we still need to test.
Comment 38 Felix Riemann 2014-12-23 16:42:49 UTC
Review of attachment 293211 [details] [review]:

(In reply to comment #37)
> Have you taken a look at these yet? I'm going to work on the properties dialog
> now and soon those will be the remaining issues, together with the
> G_BINDING_SYNC_CREATE inside eog_window_construct_ui
> (https://git.gnome.org/browse/eog/tree/src/eog-window.c?h=wip%2Fgaction-migration#n4503)
> we still need to test.

Yes, set_state is from what I can see required and correct there. update_ui_visibility is mainly responsible for hiding stuff like the statusbar and the sidebar when entering a fullscreen mode. However, on exiting this mode they should be restored to their previous state. change_state would break this.

The SYNC_CREATE binding is needed as well, as that makes the toggle action change its state when you zoom away from "best fit". You wouldn't be able to return to best fit otherwise. The SYNC_CREATE flag is there to get the state in sync with the widget at once and not when it changes for the first time.
Comment 39 Jente Hidskes 2014-12-23 22:11:52 UTC
Created attachment 293304 [details] [review]
Migrate EogPropertiesDialog to GAction.

Alright, here's a second try to migrate EogPropertiesDialog. It does not work yet, because as I believe there is no 'mapping' between the properties dialog and the window it is spawned from, which results in the correct GActions not being found. I don't know how to remedy this. I tried adding a "parent" parameter to "g_object_new", but this resulted in a runtime error saying EogWindow can not contain multiple children since it is a GtkBin subclass. I take it that is not what you meant then, in comment #4 ?
Comment 40 Felix Riemann 2014-12-25 15:42:00 UTC
Review of attachment 293304 [details] [review]:

(In reply to comment #39)
> Created an attachment (id=293304) [details] [review]
> Migrate EogPropertiesDialog to GAction.
> 
> Alright, here's a second try to migrate EogPropertiesDialog. It does not work
> yet, because as I believe there is no 'mapping' between the properties dialog
> and the window it is spawned from, which results in the correct GActions not
> being found. I don't know how to remedy this. I tried adding a "parent"
> parameter to "g_object_new", but this resulted in a runtime error saying
> EogWindow can not contain multiple children since it is a GtkBin subclass. I
> take it that is not what you meant then, in comment #4 ?

No, while there is a parent<->child link between the EogWindow and the dialog, the dialog is not a child in the widget hierarchy. So, to have the dialog use the window's actions they need to be "inserted" into the dialog. I fixed that already by making the dialog's constructor a little bit smarter.
Comment 41 Jente Hidskes 2014-12-25 16:00:08 UTC
Ah yes, I see. If I remember correctly, that only leaves us to enable/disable the relevant actions inside update_action_groups_state. I will do those tomorrow, it's time to celebrate Christmas now. Happy holidays to you! :)
Comment 42 Jente Hidskes 2014-12-26 19:49:45 UTC
Created attachment 293362 [details] [review]
Implement "groupwise" enabling/disabling of actions

This patch tries to replace gtk_action_group_set_sensitive. I'm not entirely happy having to have two versions of actions_image, but at the moment I don't see an other option. Perhaps we can make it global, but that is not a nice solution in my opinion either.

Another possible solution is to go back to having multiple GActionEntries and iterating over those. We can add them all on the window, anyway.

Let me know which way you want to go and I'll update the patch accordingly!
Comment 43 Felix Riemann 2014-12-27 17:36:32 UTC
Well, how about putting each list into its own helper function (like _eog_window_enable_window_actions). Another alternative I could think of would be a central function that selects the list via an enum (eog_window_enable_action_group (EOG_WINDOW_ACTION_GROUP_{WINDOW|IMAGE|VIEW}, {TRUE|FALSE}).

You probably also want the lists to be static.


The second solution wouldn't be much easier I think as AFAIK you cannot iterate over the actions in a group directly but would have to get the list of action names and look up each action from the group anyways.
Comment 44 Jente Hidskes 2014-12-29 19:44:34 UTC
Created attachment 293453 [details] [review]
Implement "groupwise" enabling/disabling of actions

I went with having the helper functions, as it seemed the cleanest way to resolve this. Let me know what you think!

Besides this, is there anything that needs to be done before this ticket is resolved? I'm having a few days off, so contributions will be a bit slow until I'm back (Jan 5) but I can do some work when I have the time.
Comment 45 Felix Riemann 2015-01-04 17:31:02 UTC
Review of attachment 293453 [details] [review]:

(In reply to comment #44)
> Created an attachment (id=293453) [details] [review]
> Implement "groupwise" enabling/disabling of actions
> 
> I went with having the helper functions, as it seemed the cleanest way to
> resolve this. Let me know what you think!
> 
> Besides this, is there anything that needs to be done before this ticket is
> resolved? I'm having a few days off, so contributions will be a bit slow until
> I'm back (Jan 5) but I can do some work when I have the time.

Happy new year!
No need to hurry. :)
I was also away until yesterday.

From the top of my head I remember, that we still need to find a way for the plugins to inject their own actions. I am currently looking at how Totem handles this and it looks like it will need some changes to the plugin interface and thus won't necessarily be a small effort.

Then there's still the Recent and OpenWith menus which currently don't work either.

However, both can wait until we have the actual menus or at least the headerbar and the current state appears to work nicely. In so far, I don't have any objections of merging the current state back to master as a base for further developments.
Comment 46 Jente Hidskes 2015-01-05 19:08:40 UTC
Alright, then let's continue this into bug 740426 and come back when we have something working :)
Comment 47 Felix Riemann 2015-01-07 18:54:14 UTC
commit f8051de56416e9a49184065c70c1e926a3ae8187
Merge: 631b66c 5cfdbd1
Author: Felix Riemann <>
Date:   Wed Jan 7 19:43:50 2015 +0100

    Merge branch 'wip/gaction-migration'
    
    Conflicts:
        src/eog.gresource.xml
    
    This is the merge of the current state as it is required
    for further developments. The "OpenWith" and "RecentFiles" menus
    are still todo as is plugin integration.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=741050
Comment 48 Jente Hidskes 2015-01-17 17:35:49 UTC
Created attachment 294746 [details] [review]
Make Recent Images submenu work

I wasn't sure to which ticket I should attach this, but I figured here would work since it's not related to the headerbar per sé. (Both tickets are linked to in the commit message, however).

This patch makes the Recent Images submenu work. There is one thing I noticed that might require extra work: the whole popover menu gets expanded to the width of the largest item, so when an image has a long name the popover gets very wide. Is there a way to not have the submenu impact the width of the "root" menu? If not, we can also truncate the displayed filename to a certain length.
Comment 49 Jente Hidskes 2015-01-17 19:23:08 UTC
Created attachment 294750 [details] [review]
Make Recent Images submenu work

Previous patch is wrong. When rebasing onto master, something went wrong and a g_clear_object(&builder) wasn't removed when it should.

This version fixes that. Sorry for the noise.
Comment 50 Jente Hidskes 2015-01-17 22:37:57 UTC
Created attachment 294762 [details] [review]
Make Open With submenu work

This patch makes the Open With submenu work again.

A few things to note:

1) Passing the EogImage and GAppInfo objects to the action's activate callback is really messy at the moment. What I do currently is to create a GVariant that holds a string array, with values the commandline of the application to execute and the uri of the image to open with it. This GVariant gets unpacked in the callback and from these strings, the proper GAppInfo and GFile objects get created. It would be way nicer if we could just pack the EogImage and the GAppInfo objects in the GVariant or pass them in some other way, but I don't think this is possible.

2) Should we use GAppLaunchContext when launching the application?

3) As shown in the commit message, editor_app has been removed from eog_window_update_open_with_menu, because it causes all entries in the Open With menu to be disabled when set. Also, the application that would be set with this property should be listed amongst the entries, anyway.

Also, this patch removes some trailing whitespace in the previous. Seems my git settings still don't detect those...
Comment 51 Felix Riemann 2015-01-18 12:05:34 UTC
(In reply to comment #48)
> Created an attachment (id=294746) [details] [review]
> Make Recent Images submenu work
> 
> I wasn't sure to which ticket I should attach this, but I figured here would
> work since it's not related to the headerbar per sé. (Both tickets are linked
> to in the commit message, however).

Yep, attaching it here is fine.

> This patch makes the Recent Images submenu work. There is one thing I noticed
> that might require extra work: the whole popover menu gets expanded to the
> width of the largest item, so when an image has a long name the popover gets
> very wide. Is there a way to not have the submenu impact the width of the
> "root" menu? If not, we can also truncate the displayed filename to a certain
> length.

Well, that was technically also a limitation of the old menus. However, with the popovers the whole menu seems to break if the main window is too small. Hopefully that doesn't cause some problems with the Open With menu as well.

There's another problem. If you have a file with an underscore in its name, it will be parsed as additional mnemonic accelerators by the menu label. To solve this we'd likely have to replace every underscore in the filename with a double underscore.

However, playing with the menu quickly shows another disadvantage of the new style. The old menu was able to show the path to the image in the statusbar, which helps a lot if you have images with similar sounding file names.

We should probably think about scrapping the recent menu and fallback to use the file open dialog's recent file view.
Comment 52 Jente Hidskes 2015-01-18 12:36:19 UTC
(In reply to comment #51)
> > This patch makes the Recent Images submenu work. There is one thing I noticed
> > that might require extra work: the whole popover menu gets expanded to the
> > width of the largest item, so when an image has a long name the popover gets
> > very wide. Is there a way to not have the submenu impact the width of the
> > "root" menu? If not, we can also truncate the displayed filename to a certain
> > length.
> 
> Well, that was technically also a limitation of the old menus. However, with
> the popovers the whole menu seems to break if the main window is too small.
> Hopefully that doesn't cause some problems with the Open With menu as well.

How does it break? It doesn't for me here. I can resize the main window until it has a certain dimension and when I open the popover menu it just draws itself over and a bit outside of this window. Nothing breaks.

> There's another problem. If you have a file with an underscore in its name, it
> will be parsed as additional mnemonic accelerators by the menu label. To solve
> this we'd likely have to replace every underscore in the filename with a double
> underscore.

Oops, I didn't notice this one. Shouldn't be too hard to fix, though.

> However, playing with the menu quickly shows another disadvantage of the new
> style. The old menu was able to show the path to the image in the statusbar,
> which helps a lot if you have images with similar sounding file names.

Tooltips come to mind here, but it doesn't seem possible to add those to a GMenuItem... However, isn't the statusbar going to be removed? It's not in the mockups, at least.
Comment 53 Felix Riemann 2015-01-18 13:26:36 UTC
(In reply to comment #52)
> (In reply to comment #51)
> > > This patch makes the Recent Images submenu work. There is one thing I noticed
> > > that might require extra work: the whole popover menu gets expanded to the
> > > width of the largest item, so when an image has a long name the popover gets
> > > very wide. Is there a way to not have the submenu impact the width of the
> > > "root" menu? If not, we can also truncate the displayed filename to a certain
> > > length.
> > 
> > Well, that was technically also a limitation of the old menus. However, with
> > the popovers the whole menu seems to break if the main window is too small.
> > Hopefully that doesn't cause some problems with the Open With menu as well.
> 
> How does it break? It doesn't for me here. I can resize the main window until
> it has a certain dimension and when I open the popover menu it just draws
> itself over and a bit outside of this window. Nothing breaks.

I tried the default window size which is used when eog is started without an image. When I have an image with a really long name in the recent files list, even the main menu will be so far offset to the left, that the text is outside of the menu. I can only see the submenu-arrows.

> > There's another problem. If you have a file with an underscore in its name, it
> > will be parsed as additional mnemonic accelerators by the menu label. To solve
> > this we'd likely have to replace every underscore in the filename with a double
> > underscore.
> 
> Oops, I didn't notice this one. Shouldn't be too hard to fix, though.
> 
> > However, playing with the menu quickly shows another disadvantage of the new
> > style. The old menu was able to show the path to the image in the statusbar,
> > which helps a lot if you have images with similar sounding file names.
> 
> Tooltips come to mind here, but it doesn't seem possible to add those to a
> GMenuItem... However, isn't the statusbar going to be removed? It's not in the
> mockups, at least.

Yes, that's why I am thinking about leaving the recent files menu out in favour of the file open dialog, which has more files listed and shows the URI as a tooltip. The only advantage the old menu had, was that you could open a recent image with 1 click. With the new menus I need two, pretty much the same as the file open dialog (depending on how you count a doubleclick). When the recent files menu got introduced the filechooser didn't have the option to show the recent files, so its usefulness was different back then.
Comment 54 Jente Hidskes 2015-01-18 13:36:46 UTC
(In reply to comment #53)
> Yes, that's why I am thinking about leaving the recent files menu out in favour
> of the file open dialog, which has more files listed and shows the URI as a
> tooltip. The only advantage the old menu had, was that you could open a recent
> image with 1 click. With the new menus I need two, pretty much the same as the
> file open dialog (depending on how you count a doubleclick). When the recent
> files menu got introduced the filechooser didn't have the option to show the
> recent files, so its usefulness was different back then.

I see. The Recent Images submenu does "feel" faster than opening the file open dialog and having to look for "Recent", but I think in the end it does not matter that much. I don't think we can resolve the issue with similar filenames, so I indeed vote for removing it.
Comment 55 Felix Riemann 2015-01-19 22:23:29 UTC
(In reply to comment #54)
> (In reply to comment #53)
> > Yes, that's why I am thinking about leaving the recent files menu out in favour
> > of the file open dialog, which has more files listed and shows the URI as a
> > tooltip. The only advantage the old menu had, was that you could open a recent
> > image with 1 click. With the new menus I need two, pretty much the same as the
> > file open dialog (depending on how you count a doubleclick). When the recent
> > files menu got introduced the filechooser didn't have the option to show the
> > recent files, so its usefulness was different back then.
> 
> I see. The Recent Images submenu does "feel" faster than opening the file open
> dialog and having to look for "Recent", but I think in the end it does not
> matter that much. I don't think we can resolve the issue with similar
> filenames, so I indeed vote for removing it.

Actually, the default view for the filechooser when set to open a file is the recent files list. It's just that eog tells it to show a different folder (the user's pictures folder).

I'll remove the recent menu tomorrow. It's not that they can't be restored if necessary.

(In reply to comment #50)
> Created an attachment (id=294762) [details] [review]
> Make Open With submenu work
> 
> This patch makes the Open With submenu work again.
> 
> A few things to note:
> 
> 1) Passing the EogImage and GAppInfo objects to the action's activate callback
> is really messy at the moment. What I do currently is to create a GVariant that
> holds a string array, with values the commandline of the application to execute
> and the uri of the image to open with it. This GVariant gets unpacked in the
> callback and from these strings, the proper GAppInfo and GFile objects get
> created. It would be way nicer if we could just pack the EogImage and the
> GAppInfo objects in the GVariant or pass them in some other way, but I don't
> think this is possible.

You should get the EogWindow in the action handler's user_data parameter, so you can obtain the image that way. I'd also have no problem if you put the GAppInfo's into a GPtrArray (or something like that) and keep it in the window's private structure and just forward the entry's index through the target parameter.

> 2) Should we use GAppLaunchContext when launching the application?

You don't have to. Things worked out well so far without it. Is there a special setting you'd like to set?

> 3) As shown in the commit message, editor_app has been removed from
> eog_window_update_open_with_menu, because it causes all entries in the Open
> With menu to be disabled when set. Also, the application that would be set with
> this property should be listed amongst the entries, anyway.

Odd, how did it do that? IIRC all the open_with menu had to do with it was to enable or disable the "open editor" button depending on whether the editor app supported the current image type.

Anyway, as it was seemingly unused and is thus disabled I see no problem with removing parts that don't work anymore.

> Also, this patch removes some trailing whitespace in the previous. Seems my git
> settings still don't detect those...

Btw, I think I know why your patch overidents from time to time. Check your tab settings. It seems your tab width is set to 4 while eog normally uses 8.

I couldn't look at the patch in detail yet, sorry. I'll try to do it tomorrow evening. One thing I noticed though was, that it doesn't affect the open with menu in the right click menu. Not sure if you can reuse your menu model for that one as well.
Comment 56 Jente Hidskes 2015-01-19 22:45:45 UTC
(In reply to comment #55)
> (In reply to comment #50)
> You should get the EogWindow in the action handler's user_data parameter, so
> you can obtain the image that way.

This is what I tried initially, but I figured that it could in some way be possible that the image can already have changed before the action is executed in which case the wrong image would be openend. Are you sure this won't happen? I'll look into the GPtrArray some time this week, as I'm currently studying for the upcoming exams.

> > 2) Should we use GAppLaunchContext when launching the application?
> 
> You don't have to. Things worked out well so far without it. Is there a special setting you'd like to set?

Perhaps opening the application on the same screen would be nice. It seems I should've been referring to GdkAppLaunchContext. Evince has a nice example here[1].
 

> Btw, I think I know why your patch overidents from time to time. Check your tab
> settings. It seems your tab width is set to 4 while eog normally uses 8.
> 
> I couldn't look at the patch in detail yet, sorry. I'll try to do it tomorrow
> evening. One thing I noticed though was, that it doesn't affect the open with
> menu in the right click menu. Not sure if you can reuse your menu model for
> that one as well.

I have just set them to 8, so we will see if that improves things :) Sorry for the inconvenience caused. I didn't notice there was an open-with menu in the right click, I'll take a quick look now and see if I can update the patch tonight.

[1]: https://git.gnome.org/browse/evince/tree/shell/ev-window.c#n2955
Comment 57 Jente Hidskes 2015-01-19 23:02:32 UTC
Created attachment 294933 [details] [review]
Remove the Recent Images submenu

I had to rebase my Open With patch anyway, so figured I might as well update this one so you have less work to do tomorrow :)

It completely removes the Recent Images submenu, as was decided here.
Comment 58 Felix Riemann 2015-01-20 19:12:54 UTC
(In reply to comment #56)
> (In reply to comment #55)
> > (In reply to comment #50)
> > You should get the EogWindow in the action handler's user_data parameter, so
> > you can obtain the image that way.
> 
> This is what I tried initially, but I figured that it could in some way be
> possible that the image can already have changed before the action is executed
> in which case the wrong image would be openend. Are you sure this won't happen?
> I'll look into the GPtrArray some time this week, as I'm currently studying for
> the upcoming exams.

Sure, take your time. Regarding that race condition you mention, I think that's a border case which could only happen when switching images. However, your timing would have to be really good.

> > > 2) Should we use GAppLaunchContext when launching the application?
> > 
> > You don't have to. Things worked out well so far without it. Is there a special setting you'd like to set?
> 
> Perhaps opening the application on the same screen would be nice. It seems I
> should've been referring to GdkAppLaunchContext. Evince has a nice example
> here[1].

Yes, I was guessing something like that. It's probably easier to just recycle the open_editor function for that, as it fills out an app launch context pretty complete. ;)

(In reply to comment #57)
> Created an attachment (id=294933) [details] [review]
> Remove the Recent Images submenu
> 
> I had to rebase my Open With patch anyway, so figured I might as well update
> this one so you have less work to do tomorrow :)
> 
> It completely removes the Recent Images submenu, as was decided here.

Hey, thanks again! :)
Comment 59 Felix Riemann 2015-01-30 22:34:43 UTC
Allright, plugins should be able to add menu items now, although it's a little more work as with GtkUIManager.

commit 341e8b6e688bba0654f30cd40cb8d1eefbf556d3
Author: Felix Riemann <>
Date:   Fri Jan 30 23:30:55 2015 +0100

    reload: Update to GAction
    
    Menu handling as shown in the GTK+ plugman example.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=741050

commit 8b74808890afdff86f01e4b2748f3e4733068937
Author: Felix Riemann <>
Date:   Fri Jan 30 22:58:18 2015 +0100

    EogWindow: Allow plugin entries in gear menu
    
    Based on the corresponding feature in Totem.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=741050
Comment 60 Jente Hidskes 2015-01-31 17:36:17 UTC
Created attachment 295859 [details] [review]
Make Open With submenu work

Here's an updated patch that implements the Open With submenu. I have implemented the previous feedback, in that I now use a GPtrArray in the window's private struct to point to the relevant GAppInfo. The index into this array gets passed via the GVariant. Finally, we retrieve the image via window->priv->image.

Both popup menus are now also linked to the Open With submenu. I have also merged eog_window_open_editor and eog_window_action_open_with.

Sorry for the delay in providing this update, but I had an exam week and have been sick meanwhile.
Comment 61 Felix Riemann 2015-02-02 19:39:13 UTC
Review of attachment 295859 [details] [review]:

::: src/eog-window.c
@@ +1042,3 @@
+			 * list below outside of the loop. If we should unref,
+			 * why don't we unref the others that do not pass the
+			 * above check? */

Yes, you want to unref it here, because you keep the reference of the others when doing g_pointer_array_add below and release them again when clearing the array.
Comment 62 Felix Riemann 2015-02-02 19:39:14 UTC
Review of attachment 295859 [details] [review]:

::: src/eog-window.c
@@ +1042,3 @@
+			 * list below outside of the loop. If we should unref,
+			 * why don't we unref the others that do not pass the
+			 * above check? */

Yes, you want to unref it here, because you keep the reference of the others when doing g_pointer_array_add below and release them again when clearing the array.
Comment 63 Felix Riemann 2015-02-02 19:44:30 UTC
(In reply to comment #60)
> Created an attachment (id=295859) [details] [review]
> Make Open With submenu work
> 
> Here's an updated patch that implements the Open With submenu. I have
> implemented the previous feedback, in that I now use a GPtrArray in the
> window's private struct to point to the relevant GAppInfo. The index into this
> array gets passed via the GVariant. Finally, we retrieve the image via
> window->priv->image.
> 
> Both popup menus are now also linked to the Open With submenu. I have also
> merged eog_window_open_editor and eog_window_action_open_with.
> 
> Sorry for the delay in providing this update, but I had an exam week and have
> been sick meanwhile.

No problem, we're still pretty good in time. :)

It seems we might have to test for a while how the menu works out. From a quick check on my box it seems to have the same width pitfall as the recent files menu. It's however a bit less likely to show, so let's give it a try first.

It's nice that the popup menu even kept showing the icons again. :)

As far as I can see this completes this (quite long) ticket for now. If something needs to be changed again we should handle it in a new ticket.

So, thanks again for the help. :)
---
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.