GNOME Bugzilla – Bug 613194
SwitcherList (altTab.js) container hierarchy / scrolling
Last modified: 2012-03-01 11:29:19 UTC
There are a few different bugs in here, but they're all intertwined. 1. The new switcher code was mostly written when scrolling was totally broken, so it doesn't use StScrollView. But that could be fixed now. This would also involve adding horizontal gradient support to StScrollView to match the existing vertical gradient support, and making SwitcherList use that instead of its current gradient code. 2. AppSwitcher._getPreferredHeight is being called with the wrong forWidth; this is because it is inside an St.Bin (_clipBin) inside an St.BoxLayout (this.actor), and all of the actors are set HEIGHT_FOR_WIDTH, so when AltTabPopup calls get_preferred_height() on this._appSwitcher.actor, _st_actor_get_preferred_width() ends up getting called on this._list.actor, which computes a bogus value, and then _getPreferredHeight() is called with that value rather than the originally-passed-in value. In the AppSwitcher case (but NOT the ThumbnailList case), this.actor, this._clipBin, and this._list should all have request_mode = Clutter.RequestMode.WIDTH_FOR_HEIGHT, and I think that should fix this. (Though clipBin will presumably be turning into an StScrollView anyway.) 3. That said, it's a bit weird that SwitcherList's this.actor is an St.BoxLayout anyway, since all of its children are fixed-positioned. It would make more sense to have it be a Shell.GenericContainer, and it would take care of positioning the scrollview/list and the arrows (the gradients will be part of the scrollview now). And it should position them by allocating them, not by just assigning x, y, width, height. That might mean making ShellGenericContainer support StScrollable, which is probably a good plan, though we'd need to think about exactly the right way of making it work...
Created attachment 161733 [details] [review] [appSwitcher] Remove unneeded workaround SwitcherList.actor is no longer a St.BoxLayout but a GenericContainer now, so the hack in AppSwitcher._getPreferredHeight is no longer needed (it is called with the correct width now).
Comment on attachment 161733 [details] [review] [appSwitcher] Remove unneeded workaround ok, great
Comment on attachment 161733 [details] [review] [appSwitcher] Remove unneeded workaround Attachment 161733 [details] pushed as a1bfaac - [appSwitcher] Remove unneeded workaround
Created attachment 163875 [details] [review] Revert "[appSwitcher] Remove unneeded workaround" This reverts commit a1bfaac5a21010843a4d7e4206b2fed92b2f6e59. Seems like I was wrong; there are still cases where we get the wrong width.
Comment on attachment 163875 [details] [review] Revert "[appSwitcher] Remove unneeded workaround" ok
Comment on attachment 163875 [details] [review] Revert "[appSwitcher] Remove unneeded workaround" Attachment 163875 [details] pushed as 362fc78 - Revert "[appSwitcher] Remove unneeded workaround"
seems to have been accidentally left open
(In reply to comment #7) > seems to have been accidentally left open No I still haven't addressed the stuff from comment 1. I have a "port to St.ScrollView" patch in my local tree but didn't get to implement horizontal fade in st-scroll-view-fade in time for 3.2. So will get back to that shortly after 3.2 is out.
Created attachment 206696 [details] [review] altTab: Port to St.ScrollView The appSwitcher has been using a custom scrolling implementation because St.ScrollView was buggy when it was written. The bugs have been fixed so remove the custom implementation and move to St.ScrollView. ----------- This kills the fade effect for now; will follow up with a patch that implements it using shaders in st-scroll-view-fade. (Just getting this of my local queue).
Created attachment 207129 [details] [review] st-scroll-view-fade: Add horizontal fade support St-scroll-view-fade only allowed adding a fade effect in the vertical direction, extend it so it can work horizontally too.
Created attachment 207130 [details] [review] altTab: Port to St.ScrollView The appSwitcher has been using a custom scrolling implementation because St.ScrollView was buggy when it was written. The bugs have been fixed so remove the custom implementation and move to St.ScrollView. ---- Make use of the new fade effect.
Review of attachment 207129 [details] [review]: Wrap commit message to 79 chars. ::: src/st/st-scroll-view-fade.c @@ +64,3 @@ " float ratio = 1.0;\n" " float fade_bottom_start = fade_area[1][1] - offset_bottom;\n" +" float fade_right_start = fade_area[1][0] - offset_right;\n" Not a big fan of this index madness in the shader, but I really don't think GLSL gives us another option like a struct or something. Fine as is.
Review of attachment 207130 [details] [review]: Looks fine.
Attachment 207129 [details] pushed as fd4d645 - st-scroll-view-fade: Add horizontal fade support Attachment 207130 [details] pushed as 714ffc5 - altTab: Port to St.ScrollView
Something broke here. Change: const iconSizes = [...]; to: const iconSizes = [256]; open a few apps, and scroll to the right.
Created attachment 208776 [details] [review] altTab: Fix scrolling This hack was part of the custom scroll view code that allowed for proper scrolling when the actor was near the screen edges. Since the port to St.ScrollView, it's unnecessary and downright wrong, causing portions of actors to be clipped. Remove it.
Review of attachment 208776 [details] [review]: Looks good.
Attachment 208776 [details] pushed as bea5c6f - altTab: Fix scrolling Should be fixed for good now.