GNOME Bugzilla – Bug 610049
workspaceswitcher popup should have same proportions as actual screen
Last modified: 2010-02-22 19:31:38 UTC
Created attachment 153879 [details] [review] Make the workspace switcher use a scaled representation of the actual screen When changing desktops the workspaceswitcher popup should use the actual screen proportions for its small representations of desktops. Attached patch tries to solve that issue while also making the popup a fixed scale of the actual screen so that it will behave sensibly on both low and high resolution screens.
This makes sense to me, we need input from the designers though.
Review of attachment 153879 [details] [review]: + this._indicatorWidth = Math.round(global.screen_width * INDICATOR_SCALE); + this._indicatorHeight = Math.round(global.screen_height * INDICATOR_SCALE); Would make sense to have the width/height ratio the same as the current screen, but changing the size depending on the screen resolution breaks the idea of having it using the same layout / position as the alt-tab switcher. So keep the height fixed and just adjust the width. + + indicator.set_style("width: " + this._indicatorWidth + "px;" + + "height: " + this._indicatorHeight + "px;"); I'd just use set_width instead. + spacer.set_style("height: " + this._indicatorHeight + "px;"); This seems unnecessary, even if we want to change the height, the height of the spacer shouldn't really matter.
(In reply to comment #2) > Review of attachment 153879 [details] [review]: > > + this._indicatorWidth = Math.round(global.screen_width * > INDICATOR_SCALE); > + this._indicatorHeight = Math.round(global.screen_height * > INDICATOR_SCALE); > > Would make sense to have the width/height ratio the same as the current screen, > but changing the size depending on the screen resolution breaks the idea of > having it using the same layout / position as the alt-tab switcher. > > So keep the height fixed and just adjust the width. > > + > + indicator.set_style("width: " + this._indicatorWidth + "px;" + > + "height: " + this._indicatorHeight + "px;"); > > I'd just use set_width instead. > > + spacer.set_style("height: " + this._indicatorHeight + "px;"); > > This seems unnecessary, even if we want to change the height, the height of the > spacer shouldn't really matter. One more note: commit message is missing, just commit it locally and use git format-patch or git-bz to get a patch with the commit message included.
Created attachment 153967 [details] [review] Make the width of the workspace switcher popup dynamic in order to make it have the same proportions as the actual screen. This time only the width of each switcher box is adjusted. I have also removed the hard-coded widths from the css as they are longer needed.
Review of attachment 153967 [details] [review]: The subject line is way to long; use a shorter subject line and put the detailed description in the body of the message. Besides that it looks good. Design input still needed. ::: js/ui/workspaceSwitcherPopup.js @@ +28,3 @@ global.stage.add_actor(this.actor); + this._scaleWidth = global.screen_width / global.screen_height Missing ; at the end (should be causing a warning)
Created attachment 153972 [details] [review] Make width of workspace switcher popup dynamic Sorry. This is my first time actually using git. I have fixed the missing semi-colon (couldn't find out where the warning went, though) and rebased the patch.
Review of attachment 153972 [details] [review]: Looks good, thanks.
(In reply to comment #6) > Sorry. This is my first time actually using git. I just was telling you how to do it not "your an idiot and doing it wrong" ;)
Comment on attachment 153972 [details] [review] Make width of workspace switcher popup dynamic this produces lots of: (mutter:25077): St-WARNING **: st_widget_get_theme_node called on a widget not in a stage because you're doing: >+ indicator.set_width(Math.round(indicator.get_height() * this._scaleWidth)); and in order to figure out how tall the indicator is, it needs to know what CSS rules are going to be applied to it, but it can only figure that out after adding it to the stage. So you either need to: a) redo it to not need to call get_height() b) change it to to all the width/height adjustments during the size allocation rather than ahead of time c) change the code so that the switcher has already been added to the stage (but hidden) at that point
Created attachment 154089 [details] [review] Make width of workspace switcher popup dynamic I have tried using the style-changed signal to calculate the new size. It works and the warning is gone, but I am not entirely sure that style-changed is an elegant way of doing this. Comments?
Just ignore the attempt to use style-changed - it messes up the initial placement of the workspace switcher. Sorry for the noise.
Created attachment 154163 [details] [review] Make width of workspace switcher popup dynamic This patch should not give any warnings or strange visual effects - at least it doesn't on my system. I moved the call to redraw down after the call to add, and that appears to have fixed the warning.
Comment on attachment 154163 [details] [review] Make width of workspace switcher popup dynamic ok
Comment on attachment 154163 [details] [review] Make width of workspace switcher popup dynamic Pushed as b8a9eec.