GNOME Bugzilla – Bug 679500
Clean up overview code, don't use Clutter.Clone, show OR windows in the overview
Last modified: 2021-07-05 14:19:09 UTC
Lots of fancy stuff in this patch stack. I'm including it as one bug and patch stack because that's the easiest way to go about it. I don't want to make the same mistake I did with the initial SweetTooth extension madness with 20 different bugs :) Anyway, so, this patch does a few things: * It cleans up a lot of code in the overview and removes dead code. * Removes old concepts like the overlay_group, which we can now remove. * It makes OR windows visible in the overview. We might want to make this triggered by a special X property instead of including all OR windows. Something that might use this would include applications like grommit, or a special OSD window. There's plenty of other use cases for this behaviour, too. * Removes ClutterClone and reparenting hacks, and swaps them out with new Meta.WindowActors created in the Shell itself. This patch stack relies on the patches in bug #678989.
Created attachment 218155 [details] [review] overview: Don't fade out the Nautilus desktop window Instead, let it just drop to the floor like the rest of the other windows. We do not support using Nautilus with gnome-shell.
Created attachment 218156 [details] [review] Stop using the overlay group If we want to display override-redirect windows in the overview, we need to ensure that they are able to be sorted correctly, so that the window group appears on top of the overview. To ensure this, stop adding the overview to an overlay group, and instead add it directly to the uiGroup. Additionally, fix up some other cases where we add things directly to the overlay group where we should be adding it to the uiGroup.
Created attachment 218157 [details] [review] overview: Only show/hide non-override-redirect windows in the overview
Created attachment 218158 [details] [review] windowManager: Remove unused function
Created attachment 218159 [details] [review] windowManager: Don't play awful reparenting hacks with actors Create a new WindowActor for the MetaWindow rather than doing this awful mess.
Created attachment 218160 [details] [review] workspace: Stop using Clutter.Clone to display an X window
Created attachment 218161 [details] [review] xdndHandler: Stop using Clutter.Clone to display an X window
Created attachment 218162 [details] [review] altTab: Stop using Cluttrer.Clone to display an X window
Review of attachment 218155 [details] [review]: No. The option is still present and I'm not aware of any plans to remove it. It is not default and we discourage using it, but that doesn't mean we should remove support in gnome-shell (if "non-default" and "discouraged" were enough reason to remove features, the extension system was first to go)
Review of attachment 218156 [details] [review]: Looks mostly good, except for some minor comments and some breakage. ::: js/ui/overview.js @@ +111,3 @@ + this.actor = new St.Widget(); + Main.uiGroup.add_actor(this.actor); + this.actor.lower(global.window_group); Unrelated to this patch ::: js/ui/workspacesView.js @@ +479,3 @@ this.actor.set_clip_to_allocation(true); + this._fakeGroup = new St.Widget(); What is a "fake group"? I think the code would be less confusing if you just used Main.overview.actor ... @@ +846,3 @@ + let fakeGroup = this._fakeGroup; + fakeGroup.opacity = opacity; + fakeGroup.visible = opacity != 0; This is neither what the comment says nor what the code is supposed to be doing (in other words: you are breaking stuff).
Review of attachment 218157 [details] [review]: I'm really not sure it is a good idea to allow random UI in the overview - IMO it is system space and not something applications should be able to mess with (more so as applications don't have any means to determine the location of system elements, so they could end up covering anything). The only valid use case I can think of are g-s-d's OSDs, which are system UI as well - but as such, I'd rather see the UI code move into the shell than opening this can of worms ... So let's get design input here first - marking needs-work, because the patch is broken anyway. ::: js/ui/overview.js @@ +572,3 @@ + global.get_window_actors().forEach(function(w) { + if (!w.is_override_redirect()) + w.hide(); It's not that simple. Hint: try switching workspaces while in the overview ...
Review of attachment 218158 [details] [review]: Sure.
Review of attachment 218159 [details] [review]: Patch breaks the wallpaper during the switch. Also a less "awefully messy" commit message would be nice.
Review of attachment 218160 [details] [review]: Nice cleanup. Patch breaks DND of window previews to workspace thumbnails though (if the drop happens over a window preview of the thumbnail)
Review of attachment 218161 [details] [review]: OK
Review of attachment 218162 [details] [review]: Beside the comment below, s/Cluttrer/Clutter/ in the commit message ::: js/ui/altTab.js @@ +1111,3 @@ + let clone = new Meta.WindowActor({ meta_window: metaWindow, + reactive: true, + scale_x: scale, scale_y: scale }); Using scale_x/scale_y instead of width/height messes up the positioning.
(In reply to comment #10) > Review of attachment 218156 [details] [review]: > > Looks mostly good, except for some minor comments and some breakage. > > ::: js/ui/overview.js > @@ +111,3 @@ > + this.actor = new St.Widget(); > + Main.uiGroup.add_actor(this.actor); > + this.actor.lower(global.window_group); > > Unrelated to this patch How so?
(In reply to comment #17) > (In reply to comment #10) > > + this.actor.lower(global.window_group); > > > > Unrelated to this patch > > How so? You don't need to lower the actor below global.window_group unless it's visible in the overview.
(In reply to comment #11) > Review of attachment 218157 [details] [review]: > > I'm really not sure it is a good idea to allow random UI in the overview - IMO > it is system space and not something applications should be able to mess with > (more so as applications don't have any means to determine the location of > system elements, so they could end up covering anything). The only valid use > case I can think of are g-s-d's OSDs, which are system UI as well - but as > such, I'd rather see the UI code move into the shell than opening this can of > worms ... I think moving all possible global UI in the shell is another can of worms. But that's neither here nor there. What about the grommit case, though? The thing is that we really shouldn't be managing OR windows at all -- it's only through composite that we have that ability. I'm also perfectly fine with adding an X property to ensure that yes, you really want to show this in the overview, if the concern is accidental overlap rather than abuse.
(In reply to comment #16) > Review of attachment 218162 [details] [review]: > > Beside the comment below, s/Cluttrer/Clutter/ in the commit message > > ::: js/ui/altTab.js > @@ +1111,3 @@ > + let clone = new Meta.WindowActor({ meta_window: metaWindow, > + reactive: true, > + scale_x: scale, scale_y: scale > }); > > Using scale_x/scale_y instead of width/height messes up the positioning. MetaWindowActor directly won't scale with a changed width/height -- it will clip, since there's no direct scaling logic. Given that we have code in mutter to special case the scale with a mipmap, should we change mutter to support that?
(In reply to comment #18) > (In reply to comment #17) > > (In reply to comment #10) > > > + this.actor.lower(global.window_group); > > > > > > Unrelated to this patch > > > > How so? > > You don't need to lower the actor below global.window_group unless it's visible > in the overview. Yes I do -- otherwise the overview will be above other things like the panel.
(In reply to comment #19) > I think moving all possible global UI in the shell is another can of worms. But > that's neither here nor there. What about the grommit case, though? I don't consider "all UI we consider 'system/OS'" the same as "all possible global UI". A couple quick searches on "grommit" didn't turn up anything relevant, so I can't say anything about the grommit case ... > The thing is that we really shouldn't be managing OR windows at all -- it's > only through composite that we have that ability. There are a couple of things we can only do through composite - that's neither an argument in favor of showing OR windows in the overview nor against it. On the other hand, without composite the overview would probably be a fullscreen OR window covering any other windows (OR or not) ;-) (and no, that's not a valid argument either, I know) > I'm also perfectly fine with adding an X property to ensure that yes, > you really want to show this in the overview, if the concern is accidental > overlap rather than abuse. Yes, you mentioned the possibility of a property, but I'm more worried about blurring the distinction between applications and system (e.g. the "abuse" case). We already offer a pretty powerful mechanism to modify/adjust/hack the system UI, I don't think adding another (inferior) one is a very good idea ...
> (In reply to comment #19) > > I think moving all possible global UI in the shell is another can of worms. But > > that's neither here nor there. What about the grommit case, though? > > I don't consider "all UI we consider 'system/OS'" the same as "all possible > global UI". A couple quick searches on "grommit" didn't turn up anything > relevant, so I can't say anything about the grommit case ... That's because I keep misspelling it: http://www.home.unix-ag.org/simon/gromit/ It's an old app. Monty is updating it to work in GNOME 3, and was having trouble. That's what I originally wrote the OR patch stack for. > Yes, you mentioned the possibility of a property, but I'm more worried about > blurring the distinction between applications and system (e.g. the "abuse" > case). We already offer a pretty powerful mechanism to modify/adjust/hack the > system UI, I don't think adding another (inferior) one is a very good idea ... It's a separation of concerns. I really don't want to see everything ever go into the compositor because we're too lazy to build proper protocols. I am very opposed to having random system UI like OSDs going into the Shell. The Shell already does way too much, it's growing to be super complicated, and we're not testing any part of it. There's also cases where an application may want to have an overlay without being written in JavaScript or having any GNOME-specific part at all.
Comment on attachment 218158 [details] [review] windowManager: Remove unused function Attachment 218158 [details] pushed as de65739 - windowManager: Remove unused function
As I told you on IRC a while ago ... you can't create multiple TFP textures from the same pixmap / window on some drivers (atleast NVIDIA does not allow that). That will cause cogl to fallback to using XGetImage which is *slow*. You'd be better off reusing the textures instead of creating new ones. (Adding that to the bug for reference).
(In reply to comment #25) > As I told you on IRC a while ago ... you can't create multiple TFP textures > from the same pixmap / window on some drivers (atleast NVIDIA does not allow > that). That will cause cogl to fallback to using XGetImage which is *slow*. > You'd be better off reusing the textures instead of creating new ones. > > (Adding that to the bug for reference). Oh actually I have already said that in the mutter bug (just saw this bug now while going through the patch queue; ignore that).
Review of attachment 218161 [details] [review]: Getting of the acn list.
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.