GNOME Bugzilla – Bug 695310
Add a GResource and an application menu
Last modified: 2013-03-06 19:07:54 UTC
See attached patch.
Created attachment 238219 [details] [review] Add a GResource and an application menu The application menu right now only contains "About Weather" and "Quit".
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.
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).
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.
Attachment 238230 [details] pushed as da5ee06 - Add a GResource and an application menu
Thanks, I pushed a version of the patch that does so.