GNOME Bugzilla – Bug 614905
Fix Clutter warnings
Last modified: 2010-05-06 08:20:03 UTC
Created attachment 157998 [details] [review] Fix Clutter warnings A lot of warnings appear after switch to Clutter 1.2.
Review of attachment 157998 [details] [review]: ::: js/ui/lightbox.js @@ +63,2 @@ _allocationChanged : function(container, box, flags) { + Meta.later_add(Meta.LaterType.BEFORE_REDRAW, Lang.bind(this, function() { A BEFORE_REDRAW idle will run before the redraw of the *next* frame, which means we'll draw one frame wrong, and then one frame right. Can we use a Shell.GenericContainer or would that be too annoying?
(In reply to comment #1) > Review of attachment 157998 [details] [review]: > > ::: js/ui/lightbox.js > @@ +63,2 @@ > _allocationChanged : function(container, box, flags) { > + Meta.later_add(Meta.LaterType.BEFORE_REDRAW, Lang.bind(this, > function() { > > A BEFORE_REDRAW idle will run before the redraw of the *next* frame, which > means we'll draw one frame wrong, and then one frame right. Can we use a > Shell.GenericContainer or would that be too annoying? In this place only with hack. (We cant query this._container.width/height in _getPreferredWidth/_getPreferredHeight)
(In reply to comment #2) > (In reply to comment #1) > > Review of attachment 157998 [details] [review] [details]: > > > > ::: js/ui/lightbox.js > > @@ +63,2 @@ > > _allocationChanged : function(container, box, flags) { > > + Meta.later_add(Meta.LaterType.BEFORE_REDRAW, Lang.bind(this, > > function() { > > > > A BEFORE_REDRAW idle will run before the redraw of the *next* frame, which > > means we'll draw one frame wrong, and then one frame right. Can we use a > > Shell.GenericContainer or would that be too annoying? > > In this place only with hack. (We cant query this._container.width/height in > _getPreferredWidth/_getPreferredHeight) Yeah, I can't see any other way of doing it then the later_add for the specific case of the lightbox. Shouldn't really matter since resizing will only occur when the screen geometry changes - we aren't using the notification for the initial case. (I don't think.)
Created attachment 158325 [details] [review] [workspaces] Split out workspace indicator into separate class By simply using a St.BoxLayout, we avoid the positioning/allocation hacks that the old code was doing which triggered a Clutter warning.
Comment on attachment 157998 [details] [review] Fix Clutter warnings Ok I split up this patch and committed the chunk just reviewed by Owen. The altTab part needs to be reviewed, and I rewrote the workspaces one.
Created attachment 158327 [details] [review] [altTab] Fix clutter warning
Review of attachment 158325 [details] [review]: ::: js/ui/workspacesView.js @@ +495,3 @@ + _init: function() { + this.actor = new St.Bin(); + this._container = new St.BoxLayout({ name: 'workspaceIndicatorArea' }); Size of _container will grow on low resolution and a lot of workspaces @@ +545,3 @@ + _onButtonClicked: function(actor, event) { + let index = this._container.get_children().indexOf(actor); + this.emit('activate', index); 1.need correct 'checked' property (In case click on active button) 2.Nobody listen for this event
Review of attachment 158327 [details] [review]: This seems to fix some warnings but at the cost of introducing a real bug see below. ::: js/ui/altTab.js @@ +721,3 @@ } + else if (this._scrollable){ + this._scrollable = false; I fail to see how this makes any sense ... in fact it breaks the handling of the gradients. (once you start scrolling they disappear). The point here was to hide them when no scrolling is needed.
Created attachment 159010 [details] [review] Fix Clutter warnings in altTab.js
Review of attachment 159010 [details] [review]: This fixes the warnings too, but the behavior isn't correct either. The arrows are now on top rather than centered like they are supposed to be / where before the patch. (which looks kind of weird).
(In reply to comment #10) > This fixes the warnings too, but the behavior isn't correct either. When warnings appear? (altTab work without warnings for me.)
(In reply to comment #11) > (In reply to comment #10) > > This fixes the warnings too, but the behavior isn't correct either. > > When warnings appear? (altTab work without warnings for me.) There are no warning ... I said it _fixes_ the warnings. But the scroll arrows are broken (position is wrong; they should be centered vertically).
Created attachment 159024 [details] [review] Fix Clutter warnings in altTab.js
Review of attachment 159024 [details] [review]: Looks better and the arrows are at the correct position now, one small regression is still present though. ::: js/ui/altTab.js @@ +512,3 @@ + childBox.y2 = this.actor.height; + this._leftGradient.allocate(childBox, flags); + this._leftGradient.opacity = (this._highlighted != 0 && scrollable) ? 255 : 0; This does not work like it should ... the reason this was done in scrollToLeft/Right before is that it should only be shown when scrolling in this direction is possible (one or more items are off screen at the left). @@ +519,3 @@ + childBox.y2 = this.actor.height; + this._rightGradient.allocate(childBox, flags); + this._rightGradient.opacity = (this._highlighted != this._items.length - 1 && scrollable) ? 255 : 0; Same here. @@ +593,3 @@ Tweener.addTween(this._list, { anchor_x: x, time: POPUP_SCROLL_TIME, + transition: 'easeOutQuad' Is there a reason why we can't leave the onComplete handler here? Changing the anchor point shouldn't be triggering a re-layout so it should be safe (i.e does not cause the warning).
(In reply to comment #15) > Review of attachment 159024 [details] [review]: > > Looks better and the arrows are at the correct position now, one small > regression is still present though. > > ::: js/ui/altTab.js > @@ +512,3 @@ > + childBox.y2 = this.actor.height; > + this._leftGradient.allocate(childBox, flags); > + this._leftGradient.opacity = (this._highlighted != 0 && scrollable) ? > 255 : 0; > > This does not work like it should ... the reason this was done in > scrollToLeft/Right before is that it should only be shown when scrolling in > this direction is possible (one or more items are off screen at the left). > > @@ +519,3 @@ > + childBox.y2 = this.actor.height; > + this._rightGradient.allocate(childBox, flags); > + this._rightGradient.opacity = (this._highlighted != this._items.length > - 1 && scrollable) ? 255 : 0; > > Same here. > > @@ +593,3 @@ > Tweener.addTween(this._list, { anchor_x: x, > time: POPUP_SCROLL_TIME, > + transition: 'easeOutQuad' > > Is there a reason why we can't leave the onComplete handler here? > Changing the anchor point shouldn't be triggering a re-layout so it should be > safe (i.e does not cause the warning). Ignore this; it seems to have been submitted twice (it is the same as the above comment).
Created attachment 159037 [details] [review] Fix Clutter warnings in altTab.js
Review of attachment 159037 [details] [review]: This one looks good, and does not seem to cause any regressions.
Created attachment 159381 [details] [review] [workspaces] Split out workspace indicator into separate class
Review of attachment 159381 [details] [review]: One quick comment, I'll need to take a closer look tomorrow. Thanks a lot for working on these patches though, getting rid of the clutter warnings will help debugging a lot. ::: js/ui/workspacesView.js @@ +517,3 @@ + return false; + })); + })); The code reorganization here is an improvement, but I still don't like using the reallocate -> idle -> reallocate cycle here. I guess to do it in one cycle we'd basically need to draw the rectangles internally. I won't say this is a blocker though.
Created attachment 159454 [details] [review] [workspaces] Split out workspace indicator into separate class do reallocate (after resize) in one cycle.
Review of attachment 159454 [details] [review]: Some small comments: ::: js/ui/workspacesView.js @@ +507,3 @@ +} + this._init(activateWorkspace, workspaceAcceptDrop, scrollEventCb); +function WorkspaceIndicator(activateWorkspace, workspaceAcceptDrop, scrollEventCb) { This isn't the right way to use Main.initializeDeferredWork. You should save the return string value, and then later when something changes, call Main.queueDeferredWork(this._workId); Look at AllAppDisplay in js/ui/appDisplay.js. @@ +540,3 @@ + + _allocate: function(actor, box, flags) { + actor.set_clip(box.x1, box.y1, box.x2, box.y2); You could accomplish this by setting the "clip-to-allocation" Clutter.Actor property too. @@ +559,3 @@ + for (let i = 0; i < children.length; i++) { + if (!children[i].visible) + continue; I'm not sure why we're testing for visibility here - as far as I see we never hide the actors, right?
Created attachment 159613 [details] [review] [workspaces] Split out workspace indicator into separate class
Review of attachment 159613 [details] [review]: Looks fine, thanks!