GNOME Bugzilla – Bug 740647
Use Gtk+'s Composite Widget Templates feature
Last modified: 2015-08-23 17:32:03 UTC
Now that Gtk+'s Composite Widget Templates feature is with us for a while and that Gjs implemented a good integration of it, it'd be nice to use this feature in Maps, so we can make the code to look even nicer and move a lot of JS code to GtkBuilder files. Actually, this ticket is intended to formalize the development of the branch wip/templates and to add new work to it, so we can review it and integrate the code to master from BZ. I'll try to submit in the following days a patch-set with the current wip/template work (rebased and with the fixups squashed, of course) and more work necessary to complete this refactorization.
Created attachment 300169 [details] [review] LayersPopover: Use templates
Created attachment 300170 [details] [review] ZoomControl: Use Templates
Created attachment 300171 [details] [review] InstructionRow: Use Templates
Created attachment 300172 [details] [review] Sidebar: Use Templates
Created attachment 300173 [details] [review] MainWindow: Use Templates Since we need to inherit from a GObject to be templated this patch also makes MainWindow extend Gtk.ApplicationWindow.
Created attachment 300174 [details] [review] MainWindow: Use params pattern for _init This makes MainWindow consistent with most other classes in Maps.
Created attachment 300175 [details] [review] MainWindow: Use getter for mapView This adds consistency to our coding style.
Created attachment 300176 [details] [review] ContextMenu: Use Templates
Created attachment 300177 [details] [review] ContextMenu: Use params pattern This makes ContextMenu consistent with most other classes in Maps.
The patches above moves Maps to use templates for all cases that I'm sure it works. Still not sure about how to use templates when inheriting from another template class, but let's take that on later. There's also some small cleanup patches sprinkled in.
(In reply to Mattias Bengtsson from comment #10) > The patches above moves Maps to use templates for all cases that I'm sure it > works. > Still not sure about how to use templates when inheriting from another > template class, but let's take that on later. > > There's also some small cleanup patches sprinkled in. Thanks for doing this!
Review of attachment 300169 [details] [review]: Thanks! ::: data/ui/layers-popover.ui @@ +63,3 @@ + <property name="width">1</property> + <property name="height">1</property> + </packing> Nit: Are these packing attributes (and the one above), needed? Would the result not be the same with them gone? ::: src/layersPopover.js @@ +37,1 @@ + this._aerialLayerButton.join_group(this._streetLayerButton); And this is still needed? : (
Review of attachment 300170 [details] [review]: Nice!
Review of attachment 300171 [details] [review]: (o/ ::: data/ui/sidebar.ui @@ -246,3 @@ - </packing> - </child> - <property name="xalign">0</property> Sweet!
Review of attachment 300172 [details] [review]: \o/
Review of attachment 300173 [details] [review]: Åsm! ::: src/application.js @@ +233,2 @@ }, Looks much better! ::: src/mainWindow.js @@ +92,3 @@ this._busySignalId = 0; + this._grid.attach(this._sidebar, 1, 0, 1, 1); Still not possible to do this in ui file I guess?
Review of attachment 300174 [details] [review]: Ack!
Review of attachment 300175 [details] [review]: Ok!
Review of attachment 300176 [details] [review]: Yes!
Review of attachment 300177 [details] [review]: (o/
I say "commit-now" but we should wait until we branch off gnome-3-16.
Review of attachment 300169 [details] [review]: ::: data/ui/layers-popover.ui @@ +63,3 @@ + <property name="width">1</property> + <property name="height">1</property> + </packing> I have no idea. I think Glade likes to insert them and that's why they're there. I'm not even sure what it does. I could really see a general cleanup patch of our ui files removing stuff like this, but that's for a later patch series. ::: src/layersPopover.js @@ +37,1 @@ + this._aerialLayerButton.join_group(this._streetLayerButton); Yeah I got rid of it in a separate patch that I'll attach soon.
Attachment 300170 [details] pushed as 74d3a12 - ZoomControl: Use Templates Attachment 300171 [details] pushed as fe270c5 - InstructionRow: Use Templates Attachment 300172 [details] pushed as 16da0c7 - Sidebar: Use Templates Attachment 300173 [details] pushed as 6650c30 - MainWindow: Use Templates Attachment 300174 [details] pushed as c7e6f39 - MainWindow: Use params pattern for _init Attachment 300175 [details] pushed as a09e957 - MainWindow: Use getter for mapView Attachment 300176 [details] pushed as 8ca738f - ContextMenu: Use Templates Attachment 300177 [details] pushed as 0ab663b - ContextMenu: Use params pattern