GNOME Bugzilla – Bug 694902
Fix some really bad thumbnail layout cases
Last modified: 2013-03-03 02:53:57 UTC
Steps: 1. Open two terminal windows, make sure they're fairly small 2. Enter overview Expected result: Windows are not centered Actual result: Windows are centered
Created attachment 237652 [details] [review] workspace: Remove some additional features used for GridLayoutStrategy While we won't tear down the entire strategy infrastructure, we want to rework some layout code in the future, so just tear this piece out for now.
Created attachment 237653 [details] [review] workspace: Remove unused variable This typo seems to have been here since the very beginning; no "state" variable appears anywhere.
Created attachment 237654 [details] [review] workspace: Separate out per-window scaling as a separate variable Multiplication is linear, so we can split this out as a separate component. This will make it easier to think of it as an additional per-window scaling factor, rather than tweaking the scale a bit, which is more correct to the model.
Created attachment 237655 [details] [review] workspace: Write a giant wall of text describing the algorithm I wrote this for me to find solutions to some problems we were having with the current situation, so why not check it in?
Created attachment 237656 [details] [review] workspace: Use the scaled width when calculating the total width This is a small inconsistency I found in the code after review.
Created attachment 237657 [details] [review] workspace: Fix bad window positions with small windows This can happen if you open two or three terminal windows, and then open the overview -- they're not centered. The issue is that because of the WINDOW_CLONE_MAXIMUM_SCALE clamping, the scale that is being laid out is different from the scale that the layout was calculated for. Implement and document a hack-ish solution which simply keeps the scale for the layout as originally calculated, but centers the windows inside the cell.
*** Bug 694901 has been marked as a duplicate of this bug. ***
Review of attachment 237652 [details] [review]: OK
Review of attachment 237653 [details] [review]: Sure.
Review of attachment 237654 [details] [review]: OK
Review of attachment 237655 [details] [review]: Nice! ::: js/ui/workspace.js @@ +613,3 @@ +// input area or the layout scale or rows or anything else, and right +// now just enlarges the window if it's too small. This constant factor +// until we've added too many windows to a row, and then make a new row, Did you mean computed instead of complicated?
Review of attachment 237656 [details] [review]: Looks OK
Review of attachment 237657 [details] [review]: ::: js/ui/workspace.js @@ +776,3 @@ s = Math.min(s, WINDOW_CLONE_MAXIMUM_SCALE); + let cloneWidth = window.actor.width * s; + let cloneHeight = window.actor.height * s; You don't seem to use cloneHeight, nor center the offset vertically...doesn't that contradict the above comment?
Review of attachment 237657 [details] [review]: ::: js/ui/workspace.js @@ +776,3 @@ s = Math.min(s, WINDOW_CLONE_MAXIMUM_SCALE); + let cloneWidth = window.actor.width * s; + let cloneHeight = window.actor.height * s; The offset isn't centered vertically -- to align it with the rest of the thumbnails, we vertically align it to the bottom of the row. cloneHeight is indeed unused.
Attachment 237652 [details] pushed as d58b715 - workspace: Remove some additional features used for GridLayoutStrategy Attachment 237653 [details] pushed as 6e15e2d - workspace: Remove unused variable Attachment 237654 [details] pushed as 1a33de9 - workspace: Separate out per-window scaling as a separate variable Attachment 237655 [details] pushed as e1ef14d - workspace: Write a giant wall of text describing the algorithm Attachment 237656 [details] pushed as b24a10a - workspace: Use the scaled width when calculating the total width Attachment 237657 [details] pushed as 083c37a - workspace: Fix bad window positions with small windows Pushed with a clearer comment and suggested changes.