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 703833 - Port from StTable to ClutterTableLayout
Port from StTable to ClutterTableLayout
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on: 703809 703905
Blocks:
 
 
Reported: 2013-07-08 23:35 UTC by Florian Müllner
Modified: 2015-05-05 14:38 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
st: Make Table use a Clutter layout manager internally (30.49 KB, patch)
2013-07-08 23:35 UTC, Florian Müllner
none Details | Review
st: Remove StTableChild getters/setters (13.12 KB, patch)
2013-07-08 23:35 UTC, Florian Müllner
none Details | Review
st: Translate StTableChild properties to ClutterTableChild (7.32 KB, patch)
2013-07-08 23:35 UTC, Florian Müllner
none Details | Review
Adjust to StTable changes (2.14 KB, patch)
2013-07-08 23:35 UTC, Florian Müllner
none Details | Review
calendar: Port calendar to ClutterTableLayout (4.19 KB, patch)
2013-07-09 18:18 UTC, Florian Müllner
reviewed Details | Review
calendar: Port EventsList to ClutterTableLayout (5.08 KB, patch)
2013-07-09 18:18 UTC, Florian Müllner
reviewed Details | Review
keyring: Port to ClutterTableLayout (5.50 KB, patch)
2013-07-09 18:18 UTC, Florian Müllner
reviewed Details | Review
networkAgent: Port to ClutterTableLayout (2.87 KB, patch)
2013-07-09 18:18 UTC, Florian Müllner
reviewed Details | Review
messageTray: Port to Clutter.TableLayout (13.38 KB, patch)
2013-07-09 18:18 UTC, Florian Müllner
accepted-commit_now Details | Review
st: Remove StTable (69.04 KB, patch)
2013-07-09 18:18 UTC, Florian Müllner
committed Details | Review
calendar: Port calendar to ClutterTableLayout (4.58 KB, patch)
2013-07-09 21:35 UTC, Florian Müllner
none Details | Review
calendar: Port EventsList to ClutterTableLayout (4.51 KB, patch)
2013-07-09 21:36 UTC, Florian Müllner
none Details | Review
keyring: Port to ClutterTableLayout (5.22 KB, patch)
2013-07-09 21:36 UTC, Florian Müllner
none Details | Review
networkAgent: Port to ClutterTableLayout (2.58 KB, patch)
2013-07-09 21:36 UTC, Florian Müllner
none Details | Review
messageTray: Port to Clutter.TableLayout (13.24 KB, patch)
2013-07-09 21:36 UTC, Florian Müllner
needs-work Details | Review
calendar: Port calendar to ClutterTableLayout (4.47 KB, patch)
2013-07-24 13:26 UTC, Florian Müllner
committed Details | Review
calendar: Port EventsList to ClutterTableLayout (4.22 KB, patch)
2013-07-24 13:27 UTC, Florian Müllner
committed Details | Review
keyring: Port to ClutterTableLayout (5.27 KB, patch)
2013-07-24 13:27 UTC, Florian Müllner
committed Details | Review
networkAgent: Port to ClutterTableLayout (2.11 KB, patch)
2013-07-24 13:27 UTC, Florian Müllner
committed Details | Review

Description Florian Müllner 2013-07-08 23:35:25 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.
Comment 1 Florian Müllner 2013-07-08 23:35:30 UTC
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.
Comment 2 Florian Müllner 2013-07-08 23:35:35 UTC
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.
Comment 3 Florian Müllner 2013-07-08 23:35:40 UTC
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.
Comment 4 Florian Müllner 2013-07-08 23:35:44 UTC
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.
Comment 5 Jasper St. Pierre (not reading bugmail) 2013-07-08 23:50:58 UTC
(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.
Comment 6 Florian Müllner 2013-07-09 18:18:22 UTC
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.
Comment 7 Florian Müllner 2013-07-09 18:18:28 UTC
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.
Comment 8 Florian Müllner 2013-07-09 18:18:33 UTC
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.
Comment 9 Florian Müllner 2013-07-09 18:18:39 UTC
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.
Comment 10 Florian Müllner 2013-07-09 18:18:45 UTC
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.
Comment 11 Florian Müllner 2013-07-09 18:18:53 UTC
Created attachment 248766 [details] [review]
st: Remove StTable

With all users moved over to ClutterTableLayout, we can now remove
our copy of the widget.
Comment 12 Jasper St. Pierre (not reading bugmail) 2013-07-09 18:24:33 UTC
Review of attachment 248761 [details] [review]:

You should probably remove the spacing-rows / spacing-columns from the theme as well.
Comment 13 Jasper St. Pierre (not reading bugmail) 2013-07-09 18:28:34 UTC
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.
Comment 14 Jasper St. Pierre (not reading bugmail) 2013-07-09 18:29:28 UTC
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.
Comment 15 Jasper St. Pierre (not reading bugmail) 2013-07-09 18:29:47 UTC
Review of attachment 248763 [details] [review]:

ditto
Comment 16 Jasper St. Pierre (not reading bugmail) 2013-07-09 18:30:08 UTC
Review of attachment 248764 [details] [review]:

ditto
Comment 17 Jasper St. Pierre (not reading bugmail) 2013-07-09 18:32:07 UTC
Review of attachment 248765 [details] [review]:

The notification layout is a mess right now, but it works, so ACK.
Comment 18 Jasper St. Pierre (not reading bugmail) 2013-07-09 18:32:20 UTC
Review of attachment 248766 [details] [review]:

OK.
Comment 19 Florian Müllner 2013-07-09 21:35:23 UTC
Created attachment 248778 [details] [review]
calendar: Port calendar to ClutterTableLayout

Removed unused CSS properties
Comment 20 Florian Müllner 2013-07-09 21:36:11 UTC
Created attachment 248779 [details] [review]
calendar: Port EventsList to ClutterTableLayout

Switched to monkey-patched methods from bug 703905.
Comment 21 Florian Müllner 2013-07-09 21:36:23 UTC
Created attachment 248780 [details] [review]
keyring: Port to ClutterTableLayout

Switched to monkey-patched methods from bug 703905.
Comment 22 Florian Müllner 2013-07-09 21:36:33 UTC
Created attachment 248781 [details] [review]
networkAgent: Port to ClutterTableLayout

Switched to monkey-patched methods from bug 703905.
Comment 23 Florian Müllner 2013-07-09 21:36:42 UTC
Created attachment 248782 [details] [review]
messageTray: Port to Clutter.TableLayout

Switched to monkey-patched methods from bug 703905.
Comment 24 Florian Müllner 2013-07-24 13:26:58 UTC
Created attachment 250036 [details] [review]
calendar: Port calendar to ClutterTableLayout

Updated to not depend on Clutter changes.
Comment 25 Florian Müllner 2013-07-24 13:27:09 UTC
Created attachment 250037 [details] [review]
calendar: Port EventsList to ClutterTableLayout

Dto.
Comment 26 Florian Müllner 2013-07-24 13:27:19 UTC
Created attachment 250038 [details] [review]
keyring: Port to ClutterTableLayout

Dto.
Comment 27 Florian Müllner 2013-07-24 13:27:28 UTC
Created attachment 250039 [details] [review]
networkAgent: Port to ClutterTableLayout

Dto.
Comment 28 Florian Müllner 2013-07-24 13:29:32 UTC
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.
Comment 29 Jasper St. Pierre (not reading bugmail) 2013-09-18 19:52:35 UTC
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.
Comment 30 Florian Müllner 2013-09-18 19:59:16 UTC
(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)
Comment 31 Florian Müllner 2013-09-18 20:01:26 UTC
(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.
Comment 32 Jasper St. Pierre (not reading bugmail) 2013-09-18 20:24:11 UTC
Review of attachment 250038 [details] [review]:

Looks good.
Comment 33 Florian Müllner 2013-09-18 20:41:25 UTC
(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.
Comment 34 Jasper St. Pierre (not reading bugmail) 2013-09-18 21:37:17 UTC
Review of attachment 250039 [details] [review]:

OK.
Comment 35 Florian Müllner 2013-09-19 05:55:07 UTC
Attachment 250038 [details] pushed as fb52a93 - keyring: Port to ClutterTableLayout
Attachment 250039 [details] pushed as be1a7ba - networkAgent: Port to ClutterTableLayout
Comment 36 Florian Müllner 2014-05-28 20:13:13 UTC
Attachment 250036 [details] pushed as ec71486 - calendar: Port calendar to ClutterTableLayout
Attachment 250037 [details] pushed as 0d6c002 - calendar: Port EventsList to ClutterTableLayout
Comment 37 Florian Müllner 2015-05-05 14:38:17 UTC
Attachment 248766 [details] pushed as 86e0404 - st: Remove StTable