GNOME Bugzilla – Bug 703833
Port from StTable to ClutterTableLayout
Last modified: 2015-05-05 14:38:24 UTC
This is fairly similar to the StBoxLayout patches, but leaves StTable with so little functionality (automatically hooking up row/column-spacing to CSS properties) that we might just as well consider to port the JS code to ClutterTableLayout and kill the widget ... This is still less tested than the BoxLayout patch set, but at least the test cases in libnotify and rhythmbox notifications work as expected now.
Created attachment 248681 [details] [review] st: Make Table use a Clutter layout manager internally Just like we did for BoxLayout, we can rip out our own implementation and delegate allocation to Clutter. However in contrast to BoxLayout, Table is neither widely used, nor does it provide much over the plain Clutter implementation, so me might consider to just replace it entirely in the future.
Created attachment 248682 [details] [review] st: Remove StTableChild getters/setters We exclusively access those through GObject properties, so instead of porting the functions to translate to corresponding ClutterTableChild properties, just remove the code.
Created attachment 248683 [details] [review] st: Translate StTableChild properties to ClutterTableChild With StTable using the Clutter layout manager, it will now respect actors' expand/align properties. However for the change to be transparent, we need to support the existing child meta properties as well. Do this by simply translating them to the corresponding ClutterTableChild properties.
Created attachment 248684 [details] [review] Adjust to StTable changes The old size request code in StTable and the now used ClutterTableLayout interpret the expand property on children that span multiple cells differently: the former considers the property only for the first cell, the latter for all spanned cells. Neither interpretation is inherently wrong, so adapt our code to the Clutter behavior, as it's the less intrusive change.
(In reply to comment #0) > This is fairly similar to the StBoxLayout patches, but leaves StTable with so > little functionality (automatically hooking up row/column-spacing to CSS > properties) that we might just as well consider to port the JS code to > ClutterTableLayout and kill the widget ... Sounds good to me.
Created attachment 248761 [details] [review] calendar: Port calendar to ClutterTableLayout We don't make use of any functionality StTable provides over ClutterTableLayout, so port all users to the Clutter layout in order to remove our own copy of the code.
Created attachment 248762 [details] [review] calendar: Port EventsList to ClutterTableLayout We don't make use of any functionality StTable provides over ClutterTableLayout, so port all users to the Clutter layout in order to remove our own copy of the code.
Created attachment 248763 [details] [review] keyring: Port to ClutterTableLayout We don't make use of any functionality StTable provides over ClutterTableLayout, so port all users to the Clutter layout in order to remove our own copy of the code.
Created attachment 248764 [details] [review] networkAgent: Port to ClutterTableLayout We don't make use of any functionality StTable provides over ClutterTableLayout, so port all users to the Clutter layout in order to remove our own copy of the code.
Created attachment 248765 [details] [review] messageTray: Port to Clutter.TableLayout We don't make use of any functionality StTable provides over ClutterTableLayout, so port all users to the Clutter layout in order to remove our own copy of the code.
Created attachment 248766 [details] [review] st: Remove StTable With all users moved over to ClutterTableLayout, we can now remove our copy of the widget.
Review of attachment 248761 [details] [review]: You should probably remove the spacing-rows / spacing-columns from the theme as well.
Review of attachment 248762 [details] [review]: ::: js/ui/calendar.js @@ +687,3 @@ }, + _onStyleChanged: function() { I wonder if this should be a common utility in misc/util.js or something so we don't have to write the same three-line function everywhere.
Review of attachment 248763 [details] [review]: I'd love to see if we could eventually merge the password dialog popups to have a common base, but we probably don't have time. With the misc/util.js helper, this is fine.
Review of attachment 248763 [details] [review]: ditto
Review of attachment 248764 [details] [review]: ditto
Review of attachment 248765 [details] [review]: The notification layout is a mess right now, but it works, so ACK.
Review of attachment 248766 [details] [review]: OK.
Created attachment 248778 [details] [review] calendar: Port calendar to ClutterTableLayout Removed unused CSS properties
Created attachment 248779 [details] [review] calendar: Port EventsList to ClutterTableLayout Switched to monkey-patched methods from bug 703905.
Created attachment 248780 [details] [review] keyring: Port to ClutterTableLayout Switched to monkey-patched methods from bug 703905.
Created attachment 248781 [details] [review] networkAgent: Port to ClutterTableLayout Switched to monkey-patched methods from bug 703905.
Created attachment 248782 [details] [review] messageTray: Port to Clutter.TableLayout Switched to monkey-patched methods from bug 703905.
Created attachment 250036 [details] [review] calendar: Port calendar to ClutterTableLayout Updated to not depend on Clutter changes.
Created attachment 250037 [details] [review] calendar: Port EventsList to ClutterTableLayout Dto.
Created attachment 250038 [details] [review] keyring: Port to ClutterTableLayout Dto.
Created attachment 250039 [details] [review] networkAgent: Port to ClutterTableLayout Dto.
Comment on attachment 248782 [details] [review] messageTray: Port to Clutter.TableLayout I also updated the messageTray patch, but unfortunately I found some cases where it breaks; will have to investigate more.
Does this need rebasing? What patches in here do you want me to review? I thought this was sort of a work-in-progress because of the message tray breakage.
(In reply to comment #29) > What patches in here do you want me to review? I > thought this was sort of a work-in-progress because of the message tray > breakage. Yes, I haven't worked on the message tray patch in any way recently, so that's still broken. The other ports should be good and could land, though at this point I'd limit it to the keyring patch as it fixes a user-visible problem (and possibly the networkAgent patch, if it suffers from the same issue)
(In reply to comment #29) > Does this need rebasing? The StTable-removal does, and I didn't check the message tray one (as it doesn't work anyway); the other ones apply fine for me.
Review of attachment 250038 [details] [review]: Looks good.
(In reply to comment #30) > and possibly the networkAgent patch, if it suffers from the same issue) It does, so we should land that as well.
Review of attachment 250039 [details] [review]: OK.
Attachment 250038 [details] pushed as fb52a93 - keyring: Port to ClutterTableLayout Attachment 250039 [details] pushed as be1a7ba - networkAgent: Port to ClutterTableLayout
Attachment 250036 [details] pushed as ec71486 - calendar: Port calendar to ClutterTableLayout Attachment 250037 [details] pushed as 0d6c002 - calendar: Port EventsList to ClutterTableLayout
Attachment 248766 [details] pushed as 86e0404 - st: Remove StTable