GNOME Bugzilla – Bug 698146
Use a .ui file for the main window (+ menu button example)
Last modified: 2013-04-19 07:05:37 UTC
Use an .ui file for the main window structure The following patchset shows how to add menu button, as discussed on irc. Such goal can also be achieved without this refactoring but since I was at it I thought I'd propose what I think it is a good way to do it: 1) Refactor code to use a .ui file for the main window (it is tiny so it does not make much sense with the current structure, but it becomes useful when adding more ui elements, like the menu button). This part depends on the libgd patch of https://bugzilla.gnome.org/show_bug.cgi?id=698139 2) Extend the action util to create stateful action (like the menu toggle) 3) An example of a menu button with a fake test action
Created attachment 241663 [details] [review] patch 1
Created attachment 241664 [details] [review] patch 2
Created attachment 241665 [details] [review] patch 3
Review of attachment 241663 [details] [review]: Looks good. ::: src/mainWindow.js @@ +54,3 @@ title: _("Maps") }); + Gd.ensure_types(); this seems more like an init function for Gd. suggest it be named Gd.init().
Review of attachment 241664 [details] [review]: Looks good otherwise. ::: src/utils.js @@ +78,3 @@ simpleActionEntries.forEach(function(entry) { + let actionParams = { name: entry.name }; + if (entry.state) { No '{' for single statements please
Review of attachment 241665 [details] [review]: Example? whats gear menu?
> + Gd.ensure_types(); > > this seems more like an init function for Gd. suggest it be named Gd.init(). I reported this comment in the libgd bug. Let's see what Cosimo says. (In reply to comment #5) I'll wait on the libgd bug and then post an updated patch that includes the new button you added > simpleActionEntries.forEach(function(entry) { > + let actionParams = { name: entry.name }; > + if (entry.state) { > > No '{' for single statements please In js you *must* use { for single statements or the interpreter will imply a ";" at the end of the if line and execute the block inconditionally. > > Example? whats gear menu? It is an example of adding a menubutton to the headerbar as discussed on irc with moonlite
Created attachment 241858 [details] [review] updated patch 1 The libgd change was committed, so once we update the copy in libgd this patch can go in. I updated it to move the track-user button in the ui too.
Review of attachment 241858 [details] [review]: Looks good otherwise. I see that my suggestion to name it gd_init() didn't get any comment. :( ::: src/application.js @@ +28,2 @@ const GtkClutter = imports.gi.GtkClutter; +const GLib = imports.gi.GLib; Redundant change.
Review of attachment 241858 [details] [review]: well apart from that small issue, this can already go in.
(In reply to comment #9) > Review of attachment 241858 [details] [review]: > > Looks good otherwise. I see that my suggestion to name it gd_init() didn't get > any comment. :( > It was discussed on irc (I should have reported the comment, but I thought you saw them in #gnome-maps). The proposal was turned down because gd_init would look like a mandatory function. > ::: src/application.js > @@ +28,2 @@ > const GtkClutter = imports.gi.GtkClutter; > +const GLib = imports.gi.GLib; > > Redundant change. Yeah, I could not resist reordering the includes, I amended the patch to avoid this bit and pushed. I also pushed the second patch (after removing the curly brace for single lines as discussed on irc). I am closing this bug as fixed: the third patch was only an example for Mattias and should not be committed.
Comment on attachment 241664 [details] [review] patch 2 (after amending a style issue)
Comment on attachment 241665 [details] [review] patch 3 This was just a proof of concept, not intended for merging