GNOME Bugzilla – Bug 744883
Window thumbnails can overlap when scaled
Last modified: 2015-03-03 22:01:09 UTC
Created attachment 297477 [details] screenshot of overlapping thumbnails Whenever any but the first row of the window thumbnails have to be scaled to fit the screen, it can happen, that thumbnails overlap each other. This is due to a bug in the computation of the window slots: When calculating the y position of a row the scaling factor of that row is applied to the cumulative height of all previous rows rather than just to the current row. So if the previous rows are not scaled, this means the y position of the scaled row would be shifted up, resulting in overlapping thumbnails. I'm going to attach patches for two different approaches to fix this and let the designers decide which one they prefer. I'll also attach videos showing each version, but it might be better to test the patches locally with a different number of windows. (I prefer the one that keeps the row height, since having scaling, vertical and horizontal movement at the same time can seem a bit too much. Also the code is much simpler.)
Created attachment 297478 [details] [review] fix that keeps the row height the same
Created attachment 297479 [details] video of the keep row height variant
Created attachment 297480 [details] [review] fix that scales each row properly
Created attachment 297481 [details] video of the scaling variant
Created attachment 297646 [details] [review] fix that scales each row properly [v2] The designers seem to prefer the scaling variant and I also realized that the "keep row height" variant basically prevents vertical scaling from adjusting the height of the grid if that was required. So let's go with the "scaling" variant. There was an issue with the vertical scaling in the "scaling" variant as well: The centering based on the too large height (= the reason for vertical scaling) was still happening when the grid was vertically scaled. This was basically undoing what the scaling trying to achieve. I've updated the patch to account for that. That being said, I've never seen the vertical scaling case actually happening, even after trying this with >80 windows open. The patch is visually identical to the previous one in the horizontal scaling case.
Created attachment 298475 [details] [review] fix that scales each row properly [v3] This version now considers that the spacing does not get scaled when calculating the additional scaling factor to prevent the overview overlapping the workspace switcher in situations with lots of open windows.
Review of attachment 298475 [details] [review]: "Also the previous code was not considering that the spacing does not get scaled" Out of scope for this bug, but that's something we might want to reconsider in the future? Increasing the proportion of whitespace as thumbnails get smaller and harder to read doesn't sound like the best strategy. Anyway, I don't like how code that is already tricky and hard to read gets even trickier and harder to read, but I don't see any obvious way around this - the patch certainly looks correct. So just a style suggestion, up to you to change the code or push your original patch ... ::: js/ui/workspace.js @@ +937,3 @@ + + if (additionalHorizontalScale < additionalVerticalScale) { + row.additionalScale = additionalHorizontalScale; Is row.additionalScale = Math.min(additionalHorizontalScale, additionalVerticalScale); // Only consider excess vertical scaling for compensation compensation += Math.max(0, (additionalVerticalScale - additionalHorizontalScale) * row.height)); clearer than the if-else split?
Review of attachment 297478 [details] [review]: Alternative fix for the one we picked, so rejecting
(In reply to Florian Müllner from comment #7) > Review of attachment 298475 [details] [review] [review]: > > "Also the previous code was not considering that the spacing does not get > scaled" > > Out of scope for this bug, but that's something we might want to reconsider > in the future? Increasing the proportion of whitespace as thumbnails get > smaller and harder to read doesn't sound like the best strategy. It was the design choice at the time. I'd be fine to rework it, but the issue is that the thumbnails can get a little tight.
(In reply to Florian Müllner from comment #7) > Review of attachment 298475 [details] [review] [review]: > > "Also the previous code was not considering that the spacing does not get > scaled" > > Out of scope for this bug, but that's something we might want to reconsider > in the future? Increasing the proportion of whitespace as thumbnails get > smaller and harder to read doesn't sound like the best strategy. That might require a better separation between horizontal and vertical scaling because it would look bad if there was different vertical spacing for a single row because it was horizontally scaled. But yeah, out of scope of this bug and really just a corner case, because usually there are not *that* many windows open. I just wanted to handle it a bit more cleanly while I was already touching that code. > Anyway, I don't like how code that is already tricky and hard to read gets > even trickier and harder to read, but I don't see any obvious way around > this - the patch certainly looks correct. So just a style suggestion, up to > you to change the code or push your original patch ... Yeah, I already tried to be rather explicit with the variable names but I couldn't come up with something more readable either... I have no strong opinion on your style suggestion. The code I've written seems a bit more explicit and clearer to me, but that's probably just because I wrote it and know what it is supposed to do. Could you push the patch for me then? (Either with or without your style fix, whichever you prefer)
Ah, I was assuming you had git access - pushed your patch unmodified then ...