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 695310 - Add a GResource and an application menu
Add a GResource and an application menu
Status: RESOLVED FIXED
Product: gnome-weather
Classification: Applications
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: GNOME Weather Maintainer(s)
GNOME Weather Maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2013-03-06 18:06 UTC by Cosimo Cecchi
Modified: 2013-03-06 19:07 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add a GResource and an application menu (6.54 KB, patch)
2013-03-06 18:06 UTC, Cosimo Cecchi
reviewed Details | Review
Add a GResource and an application menu (6.41 KB, patch)
2013-03-06 18:41 UTC, Cosimo Cecchi
committed Details | Review

Description Cosimo Cecchi 2013-03-06 18:06:30 UTC
See attached patch.
Comment 1 Cosimo Cecchi 2013-03-06 18:06:32 UTC
Created attachment 238219 [details] [review]
Add a GResource and an application menu

The application menu right now only contains "About Weather" and "Quit".
Comment 2 Giovanni Campagna 2013-03-06 18:22:15 UTC
Review of attachment 238219 [details] [review]:

::: src/main.js
@@ +48,3 @@
 
+    _onQuit: function() {
+        this._mainWindow.destroy();

this.quit() or this.get_windows().forEach(...)

@@ +52,3 @@
+
+    _onAbout: function() {
+        this._mainWindow.showAbout();

Peek the last used window and show the dialog there.

@@ +64,3 @@
+        ];
+
+    _initActions: function() {

This seems generally useful, do you mind adding to Util?

I'm trying to collect useful stuff there, in the spirit of convenience.js, which was copied in nearly every gnome-shell extension out there.
(Ah, util.js is 3-clause BSD, in case you care)

@@ +71,3 @@
+
+            if (entry.accel)
+              accel: '<Primary>q',

Shouldn't it be handled with accel attributes in the menu file?

@@ +79,3 @@
+    _initAppMenu: function() {
+        let builder = new Gtk.Builder();
+        ];

What's the advantage of using GResource over loading the XML directly?

::: src/window.js
@@ +299,3 @@
+
+    showAbout: function() {
+        let aboutDialog = new Gtk.AboutDialog();

I know that for GtkWindow properties are not effective until you realize(), but in general I prefer to set them at construction whenever possible, even if it breaks the 80 columns limit.

@@ +302,3 @@
+
+        aboutDialog.artists = [ 'Jakub Steiner <jimmac@gmail.com>' ];
+        aboutDialog.authors = [ 'Giovanni Campagna <gcampagna@src.gnome.org>' ];

You can add yourself too, if you want.
Comment 3 Cosimo Cecchi 2013-03-06 18:41:36 UTC
Created attachment 238230 [details] [review]
Add a GResource and an application menu

--

This should address the review comments; even if we don't get all the benefits of resources in C, using a GResource here is still better IMO, especially if you have more than one file, because the files will be loaded only once at startup and mapped in memory. It also allows easier integration in the build system, since we now generate the list of extra dist files automatically from the resource (i.e. you don't have to edit Makefile.am when you add a new file in the resource, which makes it less error prone).
Comment 4 Giovanni Campagna 2013-03-06 18:49:01 UTC
Review of attachment 238230 [details] [review]:

Well, I see your point, but in that case, it should handle application.css too.
Good to commit with that sorted.
Comment 5 Cosimo Cecchi 2013-03-06 19:06:55 UTC
Attachment 238230 [details] pushed as da5ee06 - Add a GResource and an application menu
Comment 6 Cosimo Cecchi 2013-03-06 19:07:54 UTC
Thanks, I pushed a version of the patch that does so.