GNOME Bugzilla – Bug 651092
Avoid two-step animation when going to the overview with mouse over workspace selector
Last modified: 2021-07-05 14:06:22 UTC
If someone: - Moves the mouse to the right side of the screen - Hits the logo key They shouldn't have to wait for two separate animations before seeing the workspaces.
Or don't slide in the workspace switcher, perhaps?
@Allan Day Agreed: if the mouse is already on the right side, I would agree that the work space switcher should not slide but just appear, along with the overview. Though it's not all that much to cry about on higher end systems, its also just a waste of power/resources.
(In reply to comment #2) > @Allan Day > Agreed: if the mouse is already on the right side, I would agree that the work > space switcher should not slide but just appear, along with the overview. > Though it's not all that much to cry about on higher end systems, its also just > a waste of power/resources. I should have been more elaborate in my original comment. What I meant was that the switcher should remain in its retracted state if the pointer is at the right hand side of the screen when the overview is opened. To open the workspace switcher, you would have to move the pointer away from the right hand side and then back again. The pointer being at the right hand side will often have nothing to do with the workspace switcher, and the switcher opening will be unintentional. It's confusing to have the switcher open when you don't intend it to.
Created attachment 209293 [details] [review] This should fix it.
Review of attachment 209293 [details] [review]: Thanks for working on this! We try to add "useful" commit messages, see http://lists.cairographics.org/archives/cairo/2008-September/015092.html for more information on what that is. They also really shouldn't be more than around 74-75 characters. Try something like: overview: Don't show workspaces when entering the overview If the user has their mouse over the workspace thumbnails while entering the overview, it's more likely that it's a coincidence that their mouse pointer is in the area. Avoid expanding the thumbnails box in that case. ::: js/ui/overview.js @@ +622,3 @@ this.emit('showing'); + + let [mouse_x, mouse_y, mouse_mods] = global.get_pointer(); We try to use camelCase in JS, e.g. mouseX, mouseY. We're not entirely consistent, though. @@ +631,3 @@ + + if(mouse_x > ws_x && mouse_y > ws_y && mouse_y < ws_bottom) { + this._workspacesDisplay._skipHover = true; We try not to access "private" properties of other objects. ::: js/ui/workspacesView.js @@ +998,3 @@ _onControlsHoverChanged: function() { + if(!this._controls.hover) + this._skipHover = false; _skipHover was never initialized in _init. We try to initialize all variables that we use.
Hey Jasper, Thanks again for your review. I've fixed most of the issues you pointed out, however as I mentioned on irc, I haven't been able to do it without accessing workspacesview._control. Do you have any ideas about this? Regards Joost
(In reply to comment #6) > Hey Jasper, > > Thanks again for your review. I've fixed most of the issues you pointed out, > however as I mentioned on irc, I haven't been able to do it without accessing > workspacesview._control. Do you have any ideas about this? Make it into a public property in a separate commit.
Created attachment 209454 [details] [review] workspacesView: Refactored the private workspacesDisplay._control to public workspacesDisplay.control, to allow outside accessing. This is a patch refactoring the private workspacesDisplay._controls to be a public property.
Created attachment 209456 [details] [review] overview: Don't show workspaces when the overview is entered and the user has the pointer on the right-side of the screen. This usually doesn't mean the user wants to open the workspaces view. overview: Don't show workspaces when the overview is entered and the user has the pointer on the right-side of the screen. This usually doesn't mean the user wants to open the workspaces view, but probably just a coincidence. This patch has to be applied in combination with the previously submitted patch to refactor workspacesDisplay._controls to workspacesDisplay.controls.
Review of attachment 209454 [details] [review]: > Author: Joost Verdoorn <joost@Monster.(none)> You need to fix that. Besides that, your commit message is a bit lacking. Let's try: workspacesView: Publicly expose _controls In order to properly prevent the thumbnails box from expanding while going to the overview, we first need to make it public. Again, your first line should explain the basic mechanism of the changes, while the rest of the commit message should go into more detail and explain the motivation behind the changes. Additionally, we like to add bug URLs as the last line in the commit message so we know what bug the patch came from. The easiest way to do this is with git-bz.
Review of attachment 209456 [details] [review]: ::: js/ui/overview.js @@ +621,3 @@ this._coverPane.show(); this.emit('showing'); + Remove whitespace. @@ +625,3 @@ + let [wsX, wsY] = this._workspacesDisplay.controls.get_transformed_position(); + let [vsX, vsY] = this._viewSelector.actor.get_transformed_position(); + let [vsW, vsH] = this._viewSelector.actor.get_transformed_size(); Since you're checking the viewselector's position/size which includes everything, if you're in the correct horizontal position (like the search box), hit the windows key, and then move over the thumbnails box, it wouldn't expand until hover changed twice more, right? @@ +626,3 @@ + let [vsX, vsY] = this._viewSelector.actor.get_transformed_position(); + let [vsW, vsH] = this._viewSelector.actor.get_transformed_size(); + Remove whitespace. @@ +627,3 @@ + let [vsW, vsH] = this._viewSelector.actor.get_transformed_size(); + + if(mouseX > wsX && mouseY > wsY && mouseY < vsY + vsH) This will break if the user had another monitor to the right and their cursor was in the right position on that monitor. ::: js/ui/workspacesView.js @@ +508,3 @@ + this.skipHover = false; + Remove whitespace. @@ +1001,3 @@ + if(!this.controls.hover) + this.skipHover = false; + Remove whitespace.
> @@ +625,3 @@ > + let [wsX, wsY] = > this._workspacesDisplay.controls.get_transformed_position(); > + let [vsX, vsY] = this._viewSelector.actor.get_transformed_position(); > + let [vsW, vsH] = this._viewSelector.actor.get_transformed_size(); > > Since you're checking the viewselector's position/size which includes > everything, if you're in the correct horizontal position (like the search box), > hit the windows key, and then move over the thumbnails box, it wouldn't expand > until hover changed twice more, right? No, that shouldn't happen. As you can see I use the viewselector's vertical position, which aligns perfectly with the workspacesView's vertical position. > @@ +627,3 @@ > + let [vsW, vsH] = this._viewSelector.actor.get_transformed_size(); > + > + if(mouseX > wsX && mouseY > wsY && mouseY < vsY + vsH) > > This will break if the user had another monitor to the right and their cursor > was in the right position on that monitor. I've tested this, but I believe default behavior when the user is using more than one monitor is to always show the workspacesview.
Created attachment 209554 [details] [review] overview: Don't show workspaces when entering the overview If the user has their mouse over the workspace thumbnails while entering the overview, it's more likely that it's a coincidence that their mouse pointer is in the area. Avoid expanding the thumbnails box in that case.
Created attachment 209612 [details] [review] overview: Don't show workspaces when entering the overview If the user has their mouse over the workspace thumbnails while entering the overview, it's more likely that it's a coincidence that their mouse pointer is in the area. Avoid expanding the thumbnails box in that case.
Created attachment 209629 [details] [review] overview: Don't show workspaces when entering the overview If the user has their mouse over the workspace thumbnails while entering the overview, it's more likely that it's a coincidence that their mouse pointer is in the area. Avoid expanding the thumbnails box in that case.
Review of attachment 209629 [details] [review]: This is getting very close! The comments below are mostly style comments. Apart from those, there's a fair share of trailing whitespace in the patch (apparently Linus really hates that, so by default git is very verbose about it). On a higher level, I don't think it is quite clear enough what's going on. The obvious option would be to add a comment, and maybe something like _controlsInitiallyHovered instead of _skipZoom makes the general idea clearer? ::: js/ui/workspacesView.js @@ +549,3 @@ + let [x, y] = this._controls.get_transformed_position(); + let width = this._controls.get_theme_node().get_length('visible-width'); + let height = this.actor.allocation.y2 - this.actor.allocation.y1; Not sure why you use this.actor for the height and not this._controls - they match, but you really want to test that the controls are hovered. I'd probably write that as let visibleWidth = this._controls.get_theme_node().get_length('visible-width'); let [width, height] = this._controls.get_transformed_size(); @@ +552,3 @@ + let rtl = (Clutter.get_default_text_direction () == Clutter.TextDirection.RTL); + if(rtl) { + let [fullWidth, _] = this._controls.get_transformed_size(); Eeeks, don't overwrite _ (which we use for gettext); if you want to make clear that you are not interested in one of the return values, you can use let fullWidth = this._controls.get_transformed_size()[0]; But then I'd just use the height as well, see previous comment :) @@ +555,3 @@ + x = x + fullWidth - width; + } + if(mouseX > x && mouseX < x + width && mouseY > y && mouseY < y + height) Not quite - we do want to include edge cases like mouseX == x. Note that the clutter position/size functions return floating point numbers, so it is probably safer to do checks like mouseX > x - 0.5 && mouseX < x + width + 0.5 (rather than using >=)
Created attachment 209906 [details] [review] overview: Don't show workspaces when entering the overview If the user has their mouse over the workspace thumbnails while entering the overview, it's more likely that it's a coincidence that their mouse pointer is in the area. Avoid expanding the thumbnails box in that case.
Florian, thank you for you patch review. The above patch should fix all the issues you pointed out. :)
Comment on attachment 209906 [details] [review] overview: Don't show workspaces when entering the overview Thanks again, let's get this committed!
Attachment 209906 [details] pushed as 4f87e86 - overview: Don't show workspaces when entering the overview
Awesome work Joost!
Seems like this regressed - I can see the original behaviour in 3.18.
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/gnome-shell/-/issues/ Thank you for your understanding and your help.