GNOME Bugzilla – Bug 597983
[AppSwitcher] Prevent thumbnail box from being partly offscreen
Last modified: 2010-03-20 16:06:48 UTC
Created attachment 145203 [details] [review] [alt-tab] Prevent thumbnail list from being partly offscreen on the left Currently the thumbnail list can be partly offscreen if there are lots off apps and/or windows open. Prevent that by maintaining a minimum space (2px) between the thumbnail list and the left screen edge.
Couldn't the thumbnail list be partly offscreen to the right too? Also, probably needs to be multihead aware - presumably the switcher should be confined to be within a single monitor and not span multiple monitors.
(In reply to comment #1) > Couldn't the thumbnail list be partly offscreen to the right too? Also, > probably needs to be multihead aware - presumably the switcher should be > confined to be within a single monitor and not span multiple monitors. Yeah currently it can the conses was that for the right side it has to scroll and for the left side it should never be off screen. I agree about the multihead part, it should be on either primary screen or better on the screen where the mouse currently is (ie the one where the user is interacting with right now).
(In reply to comment #2) > (In reply to comment #1) > > Couldn't the thumbnail list be partly offscreen to the right too? Also, > > probably needs to be multihead aware - presumably the switcher should be > > confined to be within a single monitor and not span multiple monitors. > > Yeah currently it can the conses was that for the right side it has to scroll > and for the left side it should never be off screen. Not quite; if it is less than the width of the screen, then it should be shifted left or right as needed to fit fully on the screen. Scrolling is only for when it extends off both sides of the screen.
Comment on attachment 145203 [details] [review] [alt-tab] Prevent thumbnail list from being partly offscreen on the left marking patch as needs-work as discussed
Created attachment 154790 [details] [review] [appSwitcher] Prevent thumbnail box from being partly offscreen Implement scrolling as discussed in the comments.
Created attachment 154823 [details] [review] [appSwitcher] Prevent thumbnail box from being partly offscreen Improve scroll behaviour
Created attachment 154835 [details] [review] [AppSwitcher] Prevent thumbnail box from being partly offscreen Positioning fixes.
(In reply to comment #1) > Couldn't the thumbnail list be partly offscreen to the right too? Also, > probably needs to be multihead aware - presumably the switcher should be > confined to be within a single monitor and not span multiple monitors. (In reply to comment #3) > (In reply to comment #2) > > (In reply to comment #1) > > > Couldn't the thumbnail list be partly offscreen to the right too? Also, > > > probably needs to be multihead aware - presumably the switcher should be > > > confined to be within a single monitor and not span multiple monitors. > > > > Yeah currently it can the conses was that for the right side it has to scroll > > and for the left side it should never be off screen. > > Not quite; if it is less than the width of the screen, then it should be > shifted left or right as needed to fit fully on the screen. Scrolling is only > for when it extends off both sides of the screen. The current patch does that now.
Created attachment 154851 [details] [review] [AppSwitcher] Prevent thumbnail box from being partly offscreen Some minor fixes and cleanups.
Comment on attachment 154851 [details] [review] [AppSwitcher] Prevent thumbnail box from being partly offscreen So first off, when I test this, it crashes with: Clutter:ERROR:./clutter-actor.c:2975:clutter_actor_dispose: assertion failed: (priv->parent_actor == NULL) when disposing the thumbnail list. >+ this._thumbnails.actor.x = Math.max(POPUP_LIST_SPACING / 2, Math.floor(thumbnailCenter - this._thumbnails.actor.width / 2)); I think it should be POPUP_LIST_SPACING, not /2 (so it has the same padding relative to the screen edge as it has relative to the app switcher). (I realize the existing code has the /2. It's wrong. :) >+ // We don't want to use GenericContainer here, we use a ScrollView >+ // to avoid being partly offscreen. >+ this.actor.destroy(); >+ this._list.destroy(); No. No. That's not how inheritance works. Either you make it work with what SwitcherList provides, or you make SwitcherList provide less stuff and have AppSwitcher and ThumbnailList each implement the parts that are different, or else you just remove SwitcherList, and implement AppSwitcher and ThumbnailList entirely independently. You probably want to take bug 598030 into account while doing that. >+ // We don't want to show the scrollbar >+ this.actor.get_hscroll_bar().opacity = 0; but isn't there still going to be space allocated for it vertically? >+ this.actor.get_hscroll_bar().adjustment.value = (itemSize + POPUP_LIST_SPACING) * index; >+ else if (posX < 0) >+ this.actor.get_hscroll_bar().adjustment.value = (itemSize + POPUP_LIST_SPACING) * index; this needs to be tweened. it's too abrupt to just change it. also, we probably want to fade out on the edges, rather than just cutting it off at the edge.
(In reply to comment #10) > (From update of attachment 154851 [details] [review]) > So first off, when I test this, it crashes with: > > Clutter:ERROR:./clutter-actor.c:2975:clutter_actor_dispose: assertion failed: > (priv->parent_actor == NULL) > > when disposing the thumbnail list. That is why I added a dependency on bug 611203 (it contains a fix for St.ScrollView that is needed to prevent that). > >+ this._thumbnails.actor.x = Math.max(POPUP_LIST_SPACING / 2, Math.floor(thumbnailCenter - this._thumbnails.actor.width / 2)); > > I think it should be POPUP_LIST_SPACING, not /2 (so it has the same padding > relative to the screen edge as it has relative to the app switcher). (I realize > the existing code has the /2. It's wrong. :) OK > >+ // We don't want to use GenericContainer here, we use a ScrollView > >+ // to avoid being partly offscreen. > >+ this.actor.destroy(); > >+ this._list.destroy(); > > No. No. That's not how inheritance works. Yeah it looks ugly to me too ... > Either you make it work with what > SwitcherList provides, or you make SwitcherList provide less stuff and have > AppSwitcher and ThumbnailList each implement the parts that are different, or > else you just remove SwitcherList, and implement AppSwitcher and ThumbnailList > entirely independently. You probably want to take bug 598030 into account while > doing that. OK, will look at that. > >+ // We don't want to show the scrollbar > >+ this.actor.get_hscroll_bar().opacity = 0; > > but isn't there still going to be space allocated for it vertically? Yes but .hide() does no longer work for scrollbars ( and setting a policy does not work either because when I say never the scrollbar is not only invisible but it simply does not exist). > >+ this.actor.get_hscroll_bar().adjustment.value = (itemSize + POPUP_LIST_SPACING) * index; > >+ else if (posX < 0) > >+ this.actor.get_hscroll_bar().adjustment.value = (itemSize + POPUP_LIST_SPACING) * index; > > this needs to be tweened. it's too abrupt to just change it. OK > also, we probably want to fade out on the edges, rather than just cutting it > off at the edge. Yeah makes sense.
Created attachment 155063 [details] [review] [appSwitcher] Prevent thumbnail box from being partly offscreen Fixed up patch: *) Changes SwitcherList now instead of adding a hack to Thumbnails *) Add tweening to scrolling (+ other scrolling fixes) *) Fix positioning of thumbnail box *) Does not do fading yet due to implementation issues (just added a comment about it)
Created attachment 155069 [details] [review] [appSwitcher] Prevent thumbnail box from being partly offscreen Fix mouse wheel interaction.
Created attachment 155080 [details] [review] [AppSwitcher] Prevent thumbnail box from being partly offscreen *) Avoid being off screen at the bottom.
Created attachment 155105 [details] [review] [AppSwitcher] Prevent thumbnail box from being partly offscreen No one said we don't want arrows .... so add them back!! ;) (fixed a bug that caused them not to be displayed)
Created attachment 155109 [details] [review] [AppSwitcher] Prevent thumbnail box from being partly offscreen *) Add arrows to indicate the possible scrolling directions
Created attachment 155128 [details] [review] [AppSwitcher] Prevent thumbnail box from being partly offscreen *) Upload correct version ...
Created attachment 155141 [details] [review] [AppSwitcher] Prevent thumbnail box from being partly offscreen *) Fix separator
Created attachment 155144 [details] [review] [AppSwitcher] Prevent thumbnail box from being partly offscreen An other small fix.
Created attachment 155145 [details] [review] [AppSwitcher] Prevent thumbnail box from being partly offscreen Fix regression introduced by arrow fix.
Comment on attachment 155145 [details] [review] [AppSwitcher] Prevent thumbnail box from being partly offscreen So... this patch (and the related appswitcher one) seems to be making the code messier and messier, with an increasingly complex mix of fixed positioning, GenericContainer, CSS, etc. I think we need to step back and rearchitect things before this file collapses under the weight of its hacks. We really shouldn't ever be creating something, measuring it, and then destroying it and creating it again at a different size. We ought to be able to compute what the size would be without actually allocating it. If we can't do that, we need to figure out why, and fix it. We could make AltTabPopup.actor a GenericContainer, and then have it allocate the AppSwitcher and ThumbnailList inside it. AppSwitcher would be height-for-width (so AltTabPopup would allocate it as much width as needed/possible, and then AppSwitcher would pick the right icon size to go with that width, and set its height accordingly), then there'd be padding between AppSwitcher and ThumbnailList, and then ThumbnailList would be width-for-height (AltTabPopup would allocate it all the height that was left below the AppSwitcher, and ThumbnailList would pick a thumbnail size based on that height.) > padding: 18px; >+ padding-bottom: 0px; >+ padding-top: 0px; you can specify that in a single line. I think either "padding: 18px 0px" or "0px 18px"... I forget which. >+ this.actor.add_actor(this._appSwitcher.leftArrow); this is very weird. The arrows are created by SwitcherList, but then added to the AltTabPopup? And this is done separately for the AppSwitcher arrows and the ThumbnailList arrows. Why isn't this handled entirely within SwitcherList? >+ // TODO: Implement edge fadding "fading" >+ Tweener.addTween(this.actor.get_hscroll_bar().adjustment, { >+ value: prevIndex == 0 ? itemSize * index : targetPos, fix the indentation to match other uses of addTween >+ } >+ else if (posX < 0) { should be one line >+ let padding = this.box.get_theme_node().get_padding(St.Side.LEFT); >+ let [sepMin, sepNat] = this._separator.get_preferred_width(childHeight - padding); this doesn't seem to make sense at all. Why is the LEFT padding relevant to a height? >- let arrowHeight = Math.floor(this.actor.get_theme_node().get_padding(St.Side.BOTTOM) / 3); >+ let arrowHeight = Math.floor(this.box.get_theme_node().get_padding(St.Side.LEFT) / 3); so... i guess this is because you're setting the vertical padding to 0 in the CSS now, even though you don't want 0 padding? This may tie into what I was saying on IRC the other day; it is still allocating vertical space for the scrollbar even though it's invisible, and now you're having to work around that in other places. >+ bin.set_style("height: " + boxHeight + "px"); you don't need to use CSS; just make a smaller thumbnail
(In reply to comment #21) > (From update of attachment 155145 [details] [review]) > So... this patch (and the related appswitcher one) seems to be making the code > messier and messier, with an increasingly complex mix of fixed positioning, > GenericContainer, CSS, etc. I think we need to step back and rearchitect things > before this file collapses under the weight of its hacks. > > We really shouldn't ever be creating something, measuring it, and then > destroying it and creating it again at a different size. We ought to be able to > compute what the size would be without actually allocating it. If we can't do > that, we need to figure out why, and fix it. Well why I am not disagreeing here, the reason for those hacks is that it seems rather impossible to know the size of the relevant actors beforehand (see the scale icons patch). The app icon box is forced to be a square while part of this square is the label (i.e the width is adjusted to match the height on allocate). But the height depends on the label. To know the size of the label one has to it has to be created and added to the stage first ... oh wait that would be the same hack again. So without a way to query this information I don't see how to do that in a less hacky way. The same goes for the thumbnail box (size depends on the label ... ). So my idea was "if I can't know the size beforehand I just try it out and measure it" ... yeah this sucks but again I am open for better suggestion other than "this is a hack it should be done different" (I'd happy do do it differently if I'd know how). > We could make AltTabPopup.actor a GenericContainer, and then have it allocate > the AppSwitcher and ThumbnailList inside it. AppSwitcher would be > height-for-width (so AltTabPopup would allocate it as much width as > needed/possible, and then AppSwitcher would pick the right icon size to go with > that width, and set its height accordingly), How is that different as having the screen width as the "limit" and pick the right icon size for that? We still depend on the text label here (if we want to be square, otherwise we could just ignore it). > then there'd be padding between > AppSwitcher and ThumbnailList, and then ThumbnailList would be width-for-height > (AltTabPopup would allocate it all the height that was left below the > AppSwitcher, and ThumbnailList would pick a thumbnail size based on that > height.) See above we need to know the height of the St.Label to do that (which depends on the font size + font) > > padding: 18px; > >+ padding-bottom: 0px; > >+ padding-top: 0px; > > you can specify that in a single line. I think either "padding: 18px 0px" or > "0px 18px"... I forget which. OK > >+ this.actor.add_actor(this._appSwitcher.leftArrow); > > this is very weird. The arrows are created by SwitcherList, but then added to > the AltTabPopup? And this is done separately for the AppSwitcher arrows and the > ThumbnailList arrows. Why isn't this handled entirely within SwitcherList? Because they are drawn on the "padding area" of a St.Boxlayout but yeah I should be able to do the absolute positioning from within SwitcherList > >+ // TODO: Implement edge fadding > > "fading" OK > >+ Tweener.addTween(this.actor.get_hscroll_bar().adjustment, { > >+ value: prevIndex == 0 ? itemSize * index : targetPos, > > fix the indentation to match other uses of addTween OK > >+ } > >+ else if (posX < 0) { > > should be one line OK > >+ let padding = this.box.get_theme_node().get_padding(St.Side.LEFT); > >+ let [sepMin, sepNat] = this._separator.get_preferred_width(childHeight - padding); > > this doesn't seem to make sense at all. Why is the LEFT padding relevant to a > height? This is yet another hack (ugh) ... see below > >- let arrowHeight = Math.floor(this.actor.get_theme_node().get_padding(St.Side.BOTTOM) / 3); > >+ let arrowHeight = Math.floor(this.box.get_theme_node().get_padding(St.Side.LEFT) / 3); > > so... i guess this is because you're setting the vertical padding to 0 in the > CSS now, even though you don't want 0 padding? Well no I don't want to change the padding it was a hack to get make the arrows visible they where hiding behind the "bottom padding area" for some weird reason. > This may tie into what I was saying on IRC the other day; it is still > allocating vertical space for the scrollbar even though it's invisible, and now > you're having to work around that in other places. No, as I said on IRC the scrollbar is outside of the box (the box with the style class "switcher-list" is inside the scrollview, not outside). > >+ bin.set_style("height: " + boxHeight + "px"); > > you don't need to use CSS; just make a smaller thumbnail OK So to sum it up, yeah the whole thing is becoming a big mess right now ... we have to find out how to do this in a less hacky (or non hacky) way but the current status isn't much better (the code is the UI isn't) ... So again I am open for ideas/suggestions ;)
Created attachment 155311 [details] [review] [altTab] Improve positioning and implement down scaling OK, here is the allocation system based implementation. It does not contain scrolling yet due to weirdness that I still have to debug, but it fixes the other offscreen cases and implements icon and thumbnail scaling.
Created attachment 155314 [details] [review] [altTab] Improve positioning and implement down scaling Fix small bug.
Created attachment 155318 [details] [review] [altTab] Improve positioning and implement down scaling Some cleanups.
Created attachment 155319 [details] [review] [altTab] Improve positioning and implement down scaling Fix copy & paste mistake.
Created attachment 155431 [details] [review] [AppSwitcher] Prevent from being partly offscreen *) Some cleanups & fixes *) Scrolling is back *) Arrows are back So should be at the state before the rewrite to use an allocation system started. (well icon scaling is done here to, it does not make much sense to try to do it as an extra patch).
Created attachment 155436 [details] [review] [AppSwitcher] Prevent from being partly offscreen *) Fix scrolling bug
Created attachment 155438 [details] [review] [AppSwitcher] Prevent from being partly offscreen Limit the AppSwitcher to the screen size by either downscaling or scrolling. We scale the icons down up from 96, 64, 48, 32 to 22 and start scrolling if we still fail to fit on screen. The thumbnail box is shifted to either left or right, when failing to fit we scroll here to. To prevent from being offscreen at the buttom we adjust the thumbnail height to fit. The old positioning logic is replaced with a ShellGenericContainer to implement a custom allocation system.
Created attachment 155471 [details] [review] [AppSwitcher] Prevent from being partly offscreen *) Some more cleanups *) Don't cut the thumbnails at the edge but add a (fake) fading effect in this case.
Created attachment 155477 [details] [review] [AppSwitcher] Prevent from being partly offscreen *) Improve fading effect
Created attachment 155482 [details] [review] [AppSwitcher] Prevent from being partly offscreen *) Yet more fading improvements
Created attachment 155507 [details] [review] [AppSwitcher] Prevent from being partly offscreen *) Fix resizing issues with the thumbnail box (size changes on "entering")
Created attachment 155535 [details] [review] [AppSwitcher] Prevent from being partly offscreen *) Fix bug and add some comments
*** Bug 598030 has been marked as a duplicate of this bug. ***
Comment on attachment 155535 [details] [review] [AppSwitcher] Prevent from being partly offscreen Note for design review: when there are lots of apps and the icons shrink, the labels become useless The patch is looking good, though it still needs some work. >We scale the icons down up from 96, 64, 48, 32 to 22 and start scrolling if we still fail to fit on screen. please wrap commit messages to 72 character lines >+.thumbnail-scroll-gradient-left { so, since my last review of this bug, I've discovered that StScrollView already has code to do shadows on the edges when scrolling vertically. It would be simple to extend this to work with horizontal shadows too. However, doing this now would conflict with Owen's outstanding scrolling patches, so we'll do that later. > .switcher-list .thumbnail { > width: 256px; >- height: 256px; > } don't we want to set both width and height from the code? >+const POPUP_SCROLL_TIME = 0.25; // seconds this seems pretty slow in my testing >+ for (let i = 0; i < children.length; i++) { >+ // Allocate the appSwitcher >+ // We select a size based on an icon size that does not overflow the screen >+ if (this._appSwitcher && children[i] == this._appSwitcher.actor) { You don't need to iterate over this.actor.get_children(). Just allocate this._appSwitcher.actor, and then allocate this._thumbnails.actor. >+ let focus = global.get_focus_monitor(); move that to the start of the function please >+ let icon = this._appIcons[this._currentApp].actor; unused (in the AppSwitcher part; the ThumbnailList part uses it) >+ let totalSpacing = this._appSwitcher.list.spacing * (this._appSwitcher.items.length - 1) + topPadding + bottomPadding; Why are you using the top and bottom padding here instead of left and right? If that's not a bug it needs a comment explaining it. >+ // When we have multiple items we want to use one with the style_class 'item-box' to avoid >+ // a resize when changing styles Why not if there's only a single item? And in fact, the single item case is broken; when there's only one app running, you just get a little dot rather than a highlighted icon. >+ let iconPadding = this._appSwitcher.items[j].get_theme_node().get_padding(St.Side.LEFT) * 2; Cheater! You should add the LEFT and RIGHT padding. (Or maybe StThemeNode should have get_horizontal_padding() and get_vertical_padding() methods that get LEFT+RIGHT and TOP+BOTTOM respectively?) >+ let iconSpacing = this._appSwitcher.icons[j].label.get_preferred_height(-1)[1] + iconPadding; Need a comment explaining this. Also, don't use "get_preferred_height()[1]". Do let [iconMinHeight, iconNaturalHeight] = this._appSwitcher.icons[j].label.get_preferred_height(-1); let iconSpacing = iconNaturalHeight + iconPadding; (I'm not sure "iconSpacing" is the best name for this value either.) >+ if (finalWidth < focus.width - POPUP_LIST_SPACING * 2) POPUP_LIST_SPACING should be replaced with something from the CSS. In this case it would be the padding on this.actor. (Since Shell.GenericContainer is an St.Widget now, you can give it a name or style_class and then add CSS for that.) Actually, you could take the monitor rectangle, convert it to a ClutterAllocationBox, and pass it to this.actor.get_theme_node.get_content_box() to have it subtract out the padding from all sides, and then use that rectangle rather than using "focus" directly, and that will take care of the padding. (This might or might not turn out to be easier than subtracting the padding manually in the right places.) >+ let childHeight = children[i].get_preferred_height(-1)[1]; you haven't allocated the icons yet, so get_preferred_height() is going to return the wrong value. You need to just compute this by hand, by taking the height you picked, and then adding whatever additional padding and border terms there are. (st_theme_node_adjust_preferred_height() might be useful there.) >+ childBox.x1 = Math.max(POPUP_LIST_SPACING, focus.x + Math.floor((focus.width - finalWidth) / 2)); >+ childBox.x2 = Math.min(childBox.x1 + finalWidth, childBox.x1 + focus.width - POPUP_LIST_SPACING * 2); as above, replace POPUP_LIST_SPACING >+ let childWidth = children[i].get_preferred_width(-1)[1]; as with the "let childHeight" above, you haven't allocated the thumbnails yet, so this is going to be wrong. >+ childBox.x1 = Math.max(POPUP_LIST_SPACING, Math.floor(thumbnailCenter - childWidth / 2)); Again, replace POPUP_LIST_SPACING throughout >+ // When we have multiple items we want to use one with the style_class 'item-box' to avoid >+ // a resize when changing styles same single-vs-multiple mystery as above >+ let innerPadding = this._thumbnails.items[j].get_theme_node().get_padding(St.Side.TOP) * 2; >+ innerPadding += this._thumbnails.items[j].child.get_theme_node().get_padding(St.Side.TOP) * 2; cheating on TOP vs BOTTOM again. Also, would be good to have a comment explaining exactly what we're adding up here. It might be possible to simplify this a bit by using st_theme_node_adjust_preferred_height() to do some of the math. >+ // + 4 is the spacing we should read that from CSS, which is currently not possible There's no direct accessor for it, but you can use st_theme_node_get_length() >+ let targetHeight = this._thumbnails.labels[j].get_preferred_height(-1)[1] + THUMBNAIL_SIZE + topPadding + bottomPadding + innerPadding + 4; fix [1] again. THUMBNAIL_SIZE should probably become THUMBNAIL_DEFAULT_SIZE. >+ childBox.y1 = posY + this._appSwitcher.actor.height - POPUP_LIST_SPACING; That math seems improbable. You want the top of the thumbnail list to be the bottom of the app switcher, plus appropriate spacing. So use the app switcher's childBox.y2 as a starting point, and add the "spacing" term from this.actor's CSS (which, like its padding, would default to POPUP_LIST_SPACING aka 8). >+ let offset = childBox.y1 + targetHeight - focus.height + POPUP_LIST_SPACING; >+ childBox.y2 = Math.min(childBox.y1 + targetHeight - offset, childBox.y1 + targetHeight); That seems bizarrely complicated. What exactly does "offset" represent? Anyway, if you cancel out the pluses and minuses, "childBox.y1 + targetHeight - offset" is just "focus.height - POPUP_LIST_SPACING", so why not just say that: childBox.y2 = Math.min(childBox.y1 + targetHeight, focus.height - POPUP_LIST_SPACING); except you want to do it without POPUP_LIST_SPACING of course >+ this.list = new Shell.GenericContainer(); >+ this.list.spacing = POPUP_LIST_SPACING; We want to get this from CSS too. WellGrid in appDisplay.js has an example of connecting to 'style-changed', reading the spacing from CSS, and setting it as a property on the JS object. (As above, this will require adding a name or style-class to this.list and then putting the right value in gnome-shell.css.) Or you could just read the spacing out of the CSS inside get_preferred/allocate. >+ this._leftArrow = new St.DrawingArea(); >+ this._leftArrow.connect('redraw', Lang.bind(this, >+ function (area, texture) { >+ Shell.draw_box_pointer(texture, Shell.PointerDirection.LEFT, TRANSPARENT_COLOR, POPUP_ARROW_COLOR); >+ })); could use CSS here too. st_theme_node_get_foreground/background_color() >+ this.actor.connect('destroy', Lang.bind(this, this.cleanStage)); >+ >+ global.stage.add_actor(this._leftArrow); >+ global.stage.add_actor(this._rightArrow); No, there's no reason to add these directly to the stage. They're part of the switcherlist, they should be children of it. Maybe that means making SwitcherList.actor be an St.BoxLayout instead of an St.Bin. Or maybe make it a Clutter.Group, which contains the two arrows and the St.Bin. In testing, when the machine is heavily loaded, it becomes very obvious that the gradients associated with the ThumbnailList are not actually part of the thumbnaillist; you see them appear *before* the list itself appears. If you set up the parent/child relationships between actors correctly then that won't happen. >- this._list = new Shell.GenericContainer(); >+ this.list = new Shell.GenericContainer(); ... >- this._items = []; >+ this.items = []; ... >- this._separator = null; >+ this.separator = null; This is bad. You're turning almost all of the private/protected members of SwitcherList into public ones, which AltTabPopup then fiddles around with when allocating. It doesn't have to work that way though. You can move most of the allocation logic out of AltTabPopup and into AppSwitcher and ThumbnailList: - when AltTabPopup gets allocated, it does: childBox.x1 = leftMargin; childBox.x2 = rightMargin; appSwitcherHeight = this._appSwitcher.actor.get_preferred_height(childBox.x2 - childBox.x1); childBox.y1 = vCenter - Math.floor(appSwitcherHeight / 2); childBox.y2 = childBox.y1 + appSwitcherHeight; this._appSwitcher.actor.allocate(childBox, flags); AppSwitcher._getPreferredHeight() would then have the code that's currently in AltTabPopup._allocate() that figures out the icon size to use based on the available width, and would return the corresponding height. AppSwitcher._allocate() would only have to change to deal with the fact that it's horizontal allocation will be the full screen width, so it needs to center itself inside that allocation. (It can cache the icon size when _getPreferredHeight() is called, and then use that again from _allocate().) - after allocating the appSwitcher, AltTabPopup._allocate() now knows how much vertical space is left, so it calls this._thumbnails.actor.get_preferred_width() with that height, and then allocates this._thumbnails.actor based on that. Again this mostly just means moving existing code from AltTabPopup._allocate() to ThumbnailList._getPreferredWidth(), and fixing ThumbnailList._allocate() to deal with the fact that the allocation may be taller than it needs. - it doesn't matter what AppSwitcher._getPreferredWidth() or ThumbnailList._getPreferredHeight() return, because they'll never get called. Likewise, the existing SwitcherList._getPreferredWidth() and _getPreferredHeight() implementations are now irrelevant. and then, AltTabPopup doesn't need to be fiddling around in AppSwitcher/ThumbnailList's private members, so you can revert them all back to being underscore-prefixed like they were before. I think this would also make the squareItems flag unnecessary, since the size logic would all be in AppSwitcher and ThumbnailList, not in SwitcherList. >+ if (posX + itemSize > monitor.width) >+ this._scrollToRight(); >+ else if (posX < 0) >+ this._scrollToLeft(); This should not be based on absolute position. (if nothing else, if the padding was large enough, an icon might be off the edge of the visible part of the box without being offscreen.) Just compare the item's allocation against the current position/width of this._list. >+ Tweener.addTween(this.list, >+ { anchor_x: x, ah, anchor_x. clever. Later on, after Owen's scrolling fixes land, we can think about whether it would make more sense to use an St.ScrollView. The indentation here is non-standard. The "{" should be lined up with "this". (Likewise in _scrollToRight.) >+ onComplete: Lang.bind(this, function () { >+ this.list.queue_relayout(); why is that needed? >+ // Clip the area for scrolling >+ let [posX, posY] = this.actor.get_transformed_position(); unused > _allocate: function (actor, box, flags) { >+ don't start a function with a blank line. (SwitcherList._allocate, AppSwitcher._allocate, and ThumbnailList._allocate all have this problem.) >+ let res = Math.max(22, (availWidth - totalSpacing - iconSpacing * this.items.length) / this.items.length); don't hardcode "22" in two places. (it's possible this function will completely change anyway, but if it doesn't, fix that) >+ for(let i = 0; i < this.icons.length; i++) { space between "for" and "(" >+ global.stage.add_actor(this._leftGradient); >+ global.stage.add_actor(this._rightGradient); as with the left/right arrows, this is wrong. also, the gradients probably belong at the SwitcherList level; we want them in the AppSwitcher if it overflows too.
(In reply to comment #36) > (From update of attachment 155535 [details] [review]) > Note for design review: when there are lots of apps and the icons shrink, the > labels become useless Yeah I was thinking about just not showing them for 22x22 or even <= 32x32 icons. > The patch is looking good, though it still needs some work. > > >We scale the icons down up from 96, 64, 48, 32 to 22 and start scrolling if we still fail to fit on screen. > > please wrap commit messages to 72 character lines OK > >+.thumbnail-scroll-gradient-left { > > so, since my last review of this bug, I've discovered that StScrollView already > has code to do shadows on the edges when scrolling vertically. It would be > simple to extend this to work with horizontal shadows too. However, doing this > now would conflict with Owen's outstanding scrolling patches, so we'll do that > later. It wouldn't be quite the same, I only show the gradient when the thumbnails would be cut off otherwise (i.e even when we scroll there is no gradient if the thumbnail is on screen). > > .switcher-list .thumbnail { > > width: 256px; > >- height: 256px; > > } > > don't we want to set both width and height from the code? Why would we want to set the width from the code? There is no reason to shrink it horizontally as we scroll anyway. > >+const POPUP_SCROLL_TIME = 0.25; // seconds > > this seems pretty slow in my testing OK, can change this, do you suggest another number or should I just try something like 0.15/0.1 ? > >+ for (let i = 0; i < children.length; i++) { > >+ // Allocate the appSwitcher > >+ // We select a size based on an icon size that does not overflow the screen > >+ if (this._appSwitcher && children[i] == this._appSwitcher.actor) { > > You don't need to iterate over this.actor.get_children(). Just allocate > this._appSwitcher.actor, and then allocate this._thumbnails.actor. OK, that makes sense. > >+ let focus = global.get_focus_monitor(); > > move that to the start of the function please OK > >+ let icon = this._appIcons[this._currentApp].actor; > > unused (in the AppSwitcher part; the ThumbnailList part uses it) OK > >+ let totalSpacing = this._appSwitcher.list.spacing * (this._appSwitcher.items.length - 1) + topPadding + bottomPadding; > > Why are you using the top and bottom padding here instead of left and right? If > that's not a bug it needs a comment explaining it. Copy and paste fail ... as both are 18 I did not really notice it. > >+ // When we have multiple items we want to use one with the style_class 'item-box' to avoid > >+ // a resize when changing styles > > Why not if there's only a single item? And in fact, the single item case is > broken; when there's only one app running, you just get a little dot rather > than a highlighted icon. Because otherwise we don't have one? > >+ let iconPadding = this._appSwitcher.items[j].get_theme_node().get_padding(St.Side.LEFT) * 2; > > Cheater! You should add the LEFT and RIGHT padding. oh ... damn! Leftover from when I was testing the math forgot to fix it. > (Or maybe StThemeNode > should have get_horizontal_padding() and get_vertical_padding() methods that > get LEFT+RIGHT and TOP+BOTTOM respectively?) Yeah that would indeed be useful. > >+ let iconSpacing = this._appSwitcher.icons[j].label.get_preferred_height(-1)[1] + iconPadding; > > Need a comment explaining this. Also, don't use "get_preferred_height()[1]". Do > > let [iconMinHeight, iconNaturalHeight] = > this._appSwitcher.icons[j].label.get_preferred_height(-1); > let iconSpacing = iconNaturalHeight + iconPadding; Well the [1] does not add an unused variable and extra code, but if it is easier to read that way I can change it. > (I'm not sure "iconSpacing" is the best name for this value either.) Well was about to call it magicSpacingAddedBytheLabel ... but iconSpacing sounded better ;) Suggestions? > >+ if (finalWidth < focus.width - POPUP_LIST_SPACING * 2) > > POPUP_LIST_SPACING should be replaced with something from the CSS. In this case > it would be the padding on this.actor. (Since Shell.GenericContainer is an > St.Widget now, you can give it a name or style_class and then add CSS for > that.) > > Actually, you could take the monitor rectangle, convert it to a > ClutterAllocationBox, and pass it to > this.actor.get_theme_node.get_content_box() to have it subtract out the padding > from all sides, and then use that rectangle rather than using "focus" directly, > and that will take care of the padding. (This might or might not turn out to be > easier than subtracting the padding manually in the right places.) OK, will try that. > >+ let childHeight = children[i].get_preferred_height(-1)[1]; > > you haven't allocated the icons yet, so get_preferred_height() is going to > return the wrong value. You need to just compute this by hand, by taking the > height you picked, and then adding whatever additional padding and border terms > there are. (st_theme_node_adjust_preferred_height() might be useful there.) OK (seems to magically work though) > >+ childBox.x1 = Math.max(POPUP_LIST_SPACING, focus.x + Math.floor((focus.width - finalWidth) / 2)); > >+ childBox.x2 = Math.min(childBox.x1 + finalWidth, childBox.x1 + focus.width - POPUP_LIST_SPACING * 2); > > as above, replace POPUP_LIST_SPACING OK > >+ let childWidth = children[i].get_preferred_width(-1)[1]; > > as with the "let childHeight" above, you haven't allocated the thumbnails yet, > so this is going to be wrong. OK > >+ childBox.x1 = Math.max(POPUP_LIST_SPACING, Math.floor(thumbnailCenter - childWidth / 2)); > > Again, replace POPUP_LIST_SPACING throughout OK > >+ let innerPadding = this._thumbnails.items[j].get_theme_node().get_padding(St.Side.TOP) * 2; > >+ innerPadding += this._thumbnails.items[j].child.get_theme_node().get_padding(St.Side.TOP) * 2; > > cheating on TOP vs BOTTOM again. Argh .. same as above ;) > Also, would be good to have a comment > explaining exactly what we're adding up here. It might be possible to simplify > this a bit by using st_theme_node_adjust_preferred_height() to do some of the > math. OK > > >+ // + 4 is the spacing we should read that from CSS, which is currently not possible > > There's no direct accessor for it, but you can use st_theme_node_get_length() OK > >+ let targetHeight = this._thumbnails.labels[j].get_preferred_height(-1)[1] + THUMBNAIL_SIZE + topPadding + bottomPadding + innerPadding + 4; > > fix [1] again. OK > THUMBNAIL_SIZE should probably become THUMBNAIL_DEFAULT_SIZE. OK > >+ childBox.y1 = posY + this._appSwitcher.actor.height - POPUP_LIST_SPACING; > > That math seems improbable. You want the top of the thumbnail list to be the > bottom of the app switcher, plus appropriate spacing. So use the app switcher's > childBox.y2 as a starting point, and add the "spacing" term from this.actor's > CSS (which, like its padding, would default to POPUP_LIST_SPACING aka 8). .... this is indeed easier, dunno why I went the other way. > >+ let offset = childBox.y1 + targetHeight - focus.height + POPUP_LIST_SPACING; > >+ childBox.y2 = Math.min(childBox.y1 + targetHeight - offset, childBox.y1 + targetHeight); > > That seems bizarrely complicated. What exactly does "offset" represent? Anyway, > if you cancel out the pluses and minuses, "childBox.y1 + targetHeight - offset" > is just "focus.height - POPUP_LIST_SPACING", so why not just say that: > > childBox.y2 = Math.min(childBox.y1 + targetHeight, focus.height - > POPUP_LIST_SPACING); > > except you want to do it without POPUP_LIST_SPACING of course OK ... > >+ this.list = new Shell.GenericContainer(); > >+ this.list.spacing = POPUP_LIST_SPACING; > > We want to get this from CSS too. WellGrid in appDisplay.js has an example of > connecting to 'style-changed', reading the spacing from CSS, and setting it as > a property on the JS object. (As above, this will require adding a name or > style-class to this.list and then putting the right value in gnome-shell.css.) OK > Or you could just read the spacing out of the CSS inside > get_preferred/allocate. > > >+ this._leftArrow = new St.DrawingArea(); > >+ this._leftArrow.connect('redraw', Lang.bind(this, > >+ function (area, texture) { > >+ Shell.draw_box_pointer(texture, Shell.PointerDirection.LEFT, TRANSPARENT_COLOR, POPUP_ARROW_COLOR); > >+ })); > > could use CSS here too. st_theme_node_get_foreground/background_color() OK > >+ this.actor.connect('destroy', Lang.bind(this, this.cleanStage)); > >+ > >+ global.stage.add_actor(this._leftArrow); > >+ global.stage.add_actor(this._rightArrow); > > No, there's no reason to add these directly to the stage. They're part of the > switcherlist, they should be children of it. Maybe that means making > SwitcherList.actor be an St.BoxLayout instead of an St.Bin. Or maybe make it a > Clutter.Group, which contains the two arrows and the St.Bin. Is there a way to absolutely position a child of St.BoyLayout? Setting x and y seems to just being ignored. > In testing, when the machine is heavily loaded, it becomes very obvious that > the gradients associated with the ThumbnailList are not actually part of the > thumbnaillist; you see them appear *before* the list itself appears. If you set > up the parent/child relationships between actors correctly then that won't > happen. Yeah because one fades in, while the other doesn't (but yeah it is broken). > > >- this._list = new Shell.GenericContainer(); > >+ this.list = new Shell.GenericContainer(); > ... > >- this._items = []; > >+ this.items = []; > ... > >- this._separator = null; > >+ this.separator = null; > > This is bad. You're turning almost all of the private/protected members of > SwitcherList into public ones, which AltTabPopup then fiddles around with when > allocating. Well I though accessing private values would be worse, adding accessors wasn't really worth it so I just made them public. > > It doesn't have to work that way though. You can move most of the allocation > logic out of AltTabPopup and into AppSwitcher and ThumbnailList: > > - when AltTabPopup gets allocated, it does: > > childBox.x1 = leftMargin; > childBox.x2 = rightMargin; > appSwitcherHeight = > this._appSwitcher.actor.get_preferred_height(childBox.x2 - childBox.x1); > childBox.y1 = vCenter - Math.floor(appSwitcherHeight / 2); > childBox.y2 = childBox.y1 + appSwitcherHeight; > this._appSwitcher.actor.allocate(childBox, flags); > > AppSwitcher._getPreferredHeight() would then have the code that's > currently in AltTabPopup._allocate() that figures out the icon size to > use based on the available width, and would return the corresponding > height. AppSwitcher._allocate() would only have to change to deal with > the fact that it's horizontal allocation will be the full screen width, > so it needs to center itself inside that allocation. (It can cache the > icon size when _getPreferredHeight() is called, and then use that > again from _allocate().) OK > - after allocating the appSwitcher, AltTabPopup._allocate() now knows how > much vertical space is left, so it calls > this._thumbnails.actor.get_preferred_width() with that height, and then > allocates this._thumbnails.actor based on that. Again this mostly just > means moving existing code from AltTabPopup._allocate() to > ThumbnailList._getPreferredWidth(), and fixing ThumbnailList._allocate() > to deal with the fact that the allocation may be taller than it needs. OK > - it doesn't matter what AppSwitcher._getPreferredWidth() or > ThumbnailList._getPreferredHeight() return, because they'll never get > called. Likewise, the existing SwitcherList._getPreferredWidth() and > _getPreferredHeight() implementations are now irrelevant. > > and then, AltTabPopup doesn't need to be fiddling around in > AppSwitcher/ThumbnailList's private members, so you can revert them all back to > being underscore-prefixed like they were before. > > I think this would also make the squareItems flag unnecessary, since the size > logic would all be in AppSwitcher and ThumbnailList, not in SwitcherList. OK > > >+ if (posX + itemSize > monitor.width) > >+ this._scrollToRight(); > >+ else if (posX < 0) > >+ this._scrollToLeft(); > > This should not be based on absolute position. (if nothing else, if the padding > was large enough, an icon might be off the edge of the visible part of the box > without being offscreen.) Just compare the item's allocation against the > current position/width of this._list. OK > >+ Tweener.addTween(this.list, > >+ { anchor_x: x, > > ah, anchor_x. clever. Later on, after Owen's scrolling fixes land, we can think > about whether it would make more sense to use an St.ScrollView. Well St.ScrollView was causing to many issues that I just went this route ... if the problems gets fixed we might reconsider this. > The indentation here is non-standard. The "{" should be lined up with "this". > (Likewise in _scrollToRight.) OK > >+ onComplete: Lang.bind(this, function () { > >+ this.list.queue_relayout(); > > why is that needed? ... it isn't. (dunno why it is still here) > >+ // Clip the area for scrolling > >+ let [posX, posY] = this.actor.get_transformed_position(); > > unused OK > > _allocate: function (actor, box, flags) { > >+ > > don't start a function with a blank line. (SwitcherList._allocate, > AppSwitcher._allocate, and ThumbnailList._allocate all have this problem.) OK > >+ let res = Math.max(22, (availWidth - totalSpacing - iconSpacing * this.items.length) / this.items.length); > > don't hardcode "22" in two places. (it's possible this function will completely > change anyway, but if it doesn't, fix that) Fix that by defining MIN_ICON size or what do you exactly mean by that? It is needed to not starting to request < 22x22 icons. > >+ for(let i = 0; i < this.icons.length; i++) { > > space between "for" and "(" OK > >+ global.stage.add_actor(this._leftGradient); > >+ global.stage.add_actor(this._rightGradient); > > as with the left/right arrows, this is wrong. OK, see above question about St.BoxLayout > also, the gradients probably belong at the SwitcherList level; we want them in > the AppSwitcher if it overflows too. Well they do calculations based on the window clone's size / position so AppSwitcher would need its own for its icons, unless we want to just always show them (when scrolling) no matter if the thumbnail/icon would be cut or not. Thanks for the review.
(In reply to comment #37) > It wouldn't be quite the same, I only show the gradient when the thumbnails > would be cut off otherwise (i.e even when we scroll there is no gradient if the > thumbnail is on screen). I'm not quite sure I understand you, but I think what you do is the same as what StScrollView does; if you're scrolled all the way to one side or the other, then you don't show the shadow on that side. But if you're somewhere in the middle, you show the shadow on both sides. At any rate, when I wrote that comment, I hadn't discovered that you weren't using a ScrollView any more, so it's not immediately relevant. > Why would we want to set the width from the code? > There is no reason to shrink it horizontally as we scroll anyway. ah, I assumed you were scaling down to availHeight x availHeight, not availHeight x 256. OK. > OK, can change this, do you suggest another number or should I just try > something like 0.15/0.1 ? I didn't try anything. We want it to be pretty quick, I just didn't like it when it was completely instantaneous. That felt wrong. > > >+ // When we have multiple items we want to use one with the style_class 'item-box' to avoid > > >+ // a resize when changing styles > > > > Why not if there's only a single item? And in fact, the single item case is > > broken; when there's only one app running, you just get a little dot rather > > than a highlighted icon. > > Because otherwise we don't have one? because otherwise we don't have one what? I guess I don't understand what the comment means. (And, as noted, the dialog actually fails to work in the only-one-app case.) > > (I'm not sure "iconSpacing" is the best name for this value either.) > > Well was about to call it magicSpacingAddedBytheLabel ... but iconSpacing > sounded better ;) > > Suggestions? "spacing" usually means blank pixels between items, which is not what this is. How about not adding iconPadding to it, and calling it labelHeight. Then later do "labelHeight + iconPadding + iconSizes[i]". > Is there a way to absolutely position a child of St.BoyLayout? > Setting x and y seems to just being ignored. That's supposed to work... you sure it didn't? We do that elsewhere. > > This is bad. You're turning almost all of the private/protected members of > > SwitcherList into public ones, which AltTabPopup then fiddles around with when > > allocating. > > Well I though accessing private values would be worse, adding accessors wasn't > really worth it so I just made them public. Either of those would also have been bad. The point is, those things belong to SwitcherList, AltTabPopup shouldn't need to act on them directly, it should let SwitcherList (or its subclasses) act on them. > > don't hardcode "22" in two places. (it's possible this function will completely > > change anyway, but if it doesn't, fix that) > > Fix that by defining MIN_ICON size or what do you exactly mean by that? well, like make the iconSizes list a global variable (well, file-level global anyway), and then just say iconSizes[0] instead of 22. > Well they do calculations based on the window clone's size / position so > AppSwitcher would need its own for its icons can't it just do the calculation generically in terms of this._list rather than using the clones specifically? Also, I don't think the gradient should have the height of the clone/icon anyway; it looks odd to have the thumbnail fade, but the label get chopped off abruptly. just noticed in emacs: lines missing semicolons: "this._destroyThumbnails()" in AltTabPopup.onDestroy and "this._clones.push(clone)" in ThumbnailList._allocate.
(In reply to comment #38) > (In reply to comment #37) > > It wouldn't be quite the same, I only show the gradient when the thumbnails > > would be cut off otherwise (i.e even when we scroll there is no gradient if the > > thumbnail is on screen). > > I'm not quite sure I understand you, but I think what you do is the same as > what StScrollView does; if you're scrolled all the way to one side or the > other, then you don't show the shadow on that side. But if you're somewhere in > the middle, you show the shadow on both sides. OK, will do that. What I was trying to do now is to only show the gradient for cut off thumbnails. i.e when the thumbnail at the edge is fits on screen there would be no gradient. But OK, removing this logic will make it simpler anyway. > > > OK, can change this, do you suggest another number or should I just try > > something like 0.15/0.1 ? > > I didn't try anything. We want it to be pretty quick, I just didn't like it > when it was completely instantaneous. That felt wrong. OK > > > >+ // When we have multiple items we want to use one with the style_class 'item-box' to avoid > > > >+ // a resize when changing styles > > > > > > Why not if there's only a single item? And in fact, the single item case is > > > broken; when there's only one app running, you just get a little dot rather > > > than a highlighted icon. > > > > Because otherwise we don't have one? > > because otherwise we don't have one what? One item with style_class item-box as it would be either outlined-item-box or selected-item-box. > I guess I don't understand what the comment means. (And, as noted, the dialog > actually fails to work in the only-one-app case.) Yeah saw that ... > > > (I'm not sure "iconSpacing" is the best name for this value either.) > > > > Well was about to call it magicSpacingAddedBytheLabel ... but iconSpacing > > sounded better ;) > > > > Suggestions? > > "spacing" usually means blank pixels between items, which is not what this is. > > How about not adding iconPadding to it, and calling it labelHeight. Then later > do "labelHeight + iconPadding + iconSizes[i]". OK > > Is there a way to absolutely position a child of St.BoyLayout? > > Setting x and y seems to just being ignored. > > That's supposed to work... you sure it didn't? We do that elsewhere. Yes I am pretty sure it didn't unless I am doing something wrong ... Where do we do that? > > > This is bad. You're turning almost all of the private/protected members of > > > SwitcherList into public ones, which AltTabPopup then fiddles around with when > > > allocating. > > > > Well I though accessing private values would be worse, adding accessors wasn't > > really worth it so I just made them public. > > Either of those would also have been bad. The point is, those things belong to > SwitcherList, AltTabPopup shouldn't need to act on them directly, it should let > SwitcherList (or its subclasses) act on them. Yeah got that just explained why I did it. > > > don't hardcode "22" in two places. (it's possible this function will completely > > > change anyway, but if it doesn't, fix that) > > > > Fix that by defining MIN_ICON size or what do you exactly mean by that? > > well, like make the iconSizes list a global variable (well, file-level global > anyway), and then just say iconSizes[0] instead of 22. OK > > Well they do calculations based on the window clone's size / position so > > AppSwitcher would need its own for its icons > > can't it just do the calculation generically in terms of this._list rather than > using the clones specifically? > > Also, I don't think the gradient should have the height of the clone/icon > anyway; it looks odd to have the thumbnail fade, but the label get chopped off > abruptly. Well I tried that it looks like crap (not sure how to explain that but the gradient does not fit visually when drawn ontop of the appSwitcher's / ThumbnailBox's background). So I ended up only doing it as height as the clones (looks much better that way). > > > > just noticed in emacs: lines missing semicolons: "this._destroyThumbnails()" in > AltTabPopup.onDestroy and "this._clones.push(clone)" in > ThumbnailList._allocate. OK
Created attachment 155866 [details] [review] Add get_horizontal/vertical_padding() methods Add get_horizontal_padding() and get_vertical_padding() methods, that return the total padding (LEFT+RIGHT or TOP+BOTTOM).
Comment on attachment 155866 [details] [review] Add get_horizontal/vertical_padding() methods code is good. two minor issues: >+ * Return value: (transfer none): the total horizonal padding >+ * Return value: (transfer none): the total vertical padding "transfer" isn't really meaningful for simple types, so it shouldn't be specified here. >+double st_theme_node_get_horizontal_padding (StThemeNode *node); >+double st_theme_node_get_vertical_padding (StThemeNode *node); add spaces before the "(" on the second line so it lines up with the first
Created attachment 156079 [details] [review] [AppSwitcher] Prevent from being partly offscreen New patch: Fixes: *) Style issues (unless I missed something) *) Gradients are now part of SwitcherList and the arrows are childs of it, and there are no longer depending on the clones. *) No longer messing with private values in AltTabPopup *) No more cheating ;) *) Reduced scroll timeout NOTES: *) Still using POPUP_LIST_SPACING ... it sucks but I am not sure it is worth replacing it in that many places ... *) Contains one hack to workaround clutter weirdness (see comment in the code, was the size issue we talked about on IRC) *) Arrows still not using CSS, should probably fix that, but wanted to get the others issues sorted out first. *) Unless I am missing something st_theme_node_get_length() cannot be used from JS right? *) I am not sure whether the gradients should have a fixed with or not, currently it is hardcoded 60 in the CSS, might be to much for 22x22 icons ... opinions?
(In reply to comment #36) > >+ if (posX + itemSize > monitor.width) > >+ this._scrollToRight(); > >+ else if (posX < 0) > >+ this._scrollToLeft(); > > This should not be based on absolute position. (if nothing else, if the padding > was large enough, an icon might be off the edge of the visible part of the box > without being offscreen.) Just compare the item's allocation against the > current position/width of this._list. That does not work because the width of this._list can be way larger than the screen, it is just clipped so only a screen sized portion is shown in this case.
Created attachment 156088 [details] [review] [AppSwitcher] Prevent from being partly offscreen Fix position of the right gradient.
(In reply to comment #42) > *) I am not sure whether the gradients should have a fixed with or not, > currently it is hardcoded 60 in the CSS, might be to much for 22x22 icons ... > opinions? After some testing ~180px seems to look good on the thumbnails but is way to much for the icons. We probably should set it from the code, not sure which heuristic to use here though. (180px on 1680x1050 is not the same as lets on 1280x800)
Created attachment 156114 [details] [review] [AppSwitcher] Prevent from being partly offscreen *) Fix icon scaling regression
Created attachment 156369 [details] [review] [AppSwitcher] Prevent from being partly offscreen *) Rebase to master
(In reply to comment #42) > *) Still using POPUP_LIST_SPACING ... it sucks but I am not sure it is worth > replacing it in that many places ... It is. But we can fix that later. > *) Arrows still not using CSS, should probably fix that, but wanted to get the > others issues sorted out first. Likewise > *) Unless I am missing something st_theme_node_get_length() cannot be used from > JS right? It can. There are other places that use it: let [found, length] = themeNode.get_length('spacing', false); if (!found) length = 0; > *) I am not sure whether the gradients should have a fixed with or not, > currently it is hardcoded 60 in the CSS, might be to much for 22x22 icons ... > opinions? The StScrollView gradients also have a fixed width (though it's only 30). Fine for now.
(In reply to comment #48) > (In reply to comment #42) > > *) Still using POPUP_LIST_SPACING ... it sucks but I am not sure it is worth > > replacing it in that many places ... > > It is. But we can fix that later. OK > > *) Arrows still not using CSS, should probably fix that, but wanted to get the > > others issues sorted out first. > > Likewise OK > > *) Unless I am missing something st_theme_node_get_length() cannot be used from > > JS right? > > It can. There are other places that use it: > > let [found, length] = themeNode.get_length('spacing', false); > if (!found) > length = 0; OK, thanks. > > *) I am not sure whether the gradients should have a fixed with or not, > > currently it is hardcoded 60 in the CSS, might be to much for 22x22 icons ... > > opinions? > > The StScrollView gradients also have a fixed width (though it's only 30). Fine > for now. OK
Created attachment 156382 [details] [review] [AppSwitcher] Prevent from being partly offscreen *) Read spacing value from CSS rather than hardcoding "4".
Created attachment 156388 [details] [review] [AppSwitcher] Prevent from being partly offscreen *) Really upload the new patch
Comment on attachment 156388 [details] [review] [AppSwitcher] Prevent from being partly offscreen ok, there are still some issues with this, but it's definitely better than what we've got now, so let's just commit this and I'll file new bugs about the things that still need fixing. (And reviewing small patches for individual fixes will be a lot easier than continually re-reviewing the one large patch...)
*** Bug 606770 has been marked as a duplicate of this bug. ***