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 653860 - introduce GtkActions
introduce GtkActions
Status: RESOLVED FIXED
Product: frogr
Classification: Other
Component: UI
master
Other Linux
: Normal enhancement
: ---
Assigned To: frogr maintainers
frogr maintainers
Depends on:
Blocks:
 
 
Reported: 2011-07-02 15:09 UTC by Joaquim Rocha
Modified: 2011-07-09 21:26 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch that adds this enhancement (59.20 KB, patch)
2011-07-04 07:36 UTC, Joaquim Rocha
none Details | Review
Patch that adds this enhancement (59.62 KB, patch)
2011-07-07 13:33 UTC, Joaquim Rocha
committed Details | Review

Description Joaquim Rocha 2011-07-02 15:09:36 UTC
Frogr's main view is "manually" creating its menu and processing of its actions.

This was done in order to simplify the code and make it easier to introduce UI changes.

Also, to use UI builder's capabilities I've raised the dependency of GTK+ to 2.16.

I did my best to tweak the menu items for the Mac OS integration but you should test this because I'm not able to.


I know it is a big patch but please consider it as it improves the code base (also remember to test it because everything seems to work for me but you never know...).
Comment 1 Joaquim Rocha 2011-07-04 07:36:28 UTC
Created attachment 191213 [details] [review]
Patch that adds this enhancement

Forgot the patch...
Comment 2 Mario Sánchez Prada 2011-07-05 08:30:46 UTC
This is one of the things I've always wanted to see in frogr but never found time to devote to. Thanks for taking care of it.

I can't try out the patch now, but I'll do it soon and will get back to you right after that.

Thanks!
Comment 3 Joaquim Rocha 2011-07-07 13:33:31 UTC
Created attachment 191448 [details] [review]
Patch that adds this enhancement

New patch with menu items' names corrected.
Comment 4 Mario Sánchez Prada 2011-07-07 14:10:56 UTC
Review of attachment 191448 [details] [review]:

Thank you! I have already committed your patch to the repo, along with a very small follow up patch to fix three translation strings.

This was a very needed thing in frogr that I was delaying ad infinitum for some time already, so thank you a lot for taking care of it.

It will be out with the next release.
Comment 5 Mario Sánchez Prada 2011-07-07 14:10:57 UTC
Review of attachment 191448 [details] [review]:

Thank you! I have already committed your patch to the repo, along with a very small follow up patch to fix three translation strings.

This was a very needed thing in frogr that I was delaying ad infinitum for some time already, so thank you a lot for taking care of it.

It will be out with the next release.
Comment 6 Mario Sánchez Prada 2011-07-09 09:28:47 UTC
Hi again, just a quick question that came to my mind this morning about this patch, related to the following excerpts of it:

diff --git a/configure.ac b/configure.ac
index 5dde704..10965e4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -116,7 +116,7 @@ else
 fi
 
 case "$with_gtk" in
-     2.0) GTK_MIN_VERSION=2.14
+     2.0) GTK_MIN_VERSION=2.16
           GTK_API_VERSION=2.0
           ;;
      3.0) GTK_MIN_VERSION=3.0


diff --git a/data/gtkbuilder/frogr-main-view.xml b/data/gtkbuilder/frogr-main-view.xml
index 144cad1..0c561eb 100644
--- a/data/gtkbuilder/frogr-main-view.xml
+++ b/data/gtkbuilder/frogr-main-view.xml
@@ -1,23 +1,188 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <interface>
-  <!-- interface-requires gtk+ 2.14 -->
-  <!-- interface-naming-policy toplevel-contextual -->
+  <requires lib="gtk+" version="2.16"/>
+  <object class="GtkAction" id="about_action">
[...]


Why is it needed to raise GTK+ version from 2.14 up to 2.16?

As far as I can see from your patch, you're not using anything specific from GTK > 2.16 (function, signals or properties), so everything should work fine without raising the version of the dependency, which is something I do not like very much to do without any specific reason.

For the time being, as I can't see any specific reason for raising the version, I have downgraded it back to 2.14, as you can see here:

http://git.gnome.org/browse/frogr/commit/?id=ab4a8b4142032d79512f364ff58ca1c093041532

But please point out any concern you have, if any, with such a change... it's more than likely that I could be missing something :)

Thanks again
Comment 7 Joaquim Rocha 2011-07-09 09:34:05 UTC
Well, just change the UI Builder's XML back to 2.14, open it with Glade and you'll see :D

It was the usage of the GtkMenuBar from UI Builder.
Comment 8 Mario Sánchez Prada 2011-07-09 21:26:08 UTC
(In reply to comment #7)
> Well, just change the UI Builder's XML back to 2.14, open it with Glade and
> you'll see :D
> 
> It was the usage of the GtkMenuBar from UI Builder.

Ahh, I see now the reason for that... already changed it back in the reposiroty, thanks for the explanation.

However, if possible in the future include a brief explanation in the commit message with the reasons behind raising a dependency, instead a simple "It also raises the dependency of GTK+ to 2.16." :), that would help people (like me) better understanding the situation.

As examples of what I mean, you can check this two commits, belonging to the moments I raised the version of GTK+ (2.12 > 2.14) and libsoup (2.24 > 2.26).

http://git.gnome.org/browse/frogr/commit/?id=938c0e67c56151580f2b4363e678d95ffe32f60b

http://git.gnome.org/browse/frogr/commit/?id=362a899b6626a32883e4ca1f2a847ed2dcc3e0dc

Again, thanks for the explanation, and for the patch, don't let this nitpicky stuff blur the awesomeness of your contribution :)