vswitch: remove "ghost window" on target workspace.

[lcolitti]

I don't understand how the scenegraph APIs work, but by tinkering with the code it looks like adding the overlay as a child of the scene::output_node_t results in both the overlay and the original window being displayed, even though the original window is hidden with wf::scene::set_node_enabled on its transfomed node.

This fix works on one output, but if a window is split across multiple outputs, then the overlay appears on the wrong output as well. That said, windows split across multiple outputs don't seem to work at the moment - only one output displays its part of the window, the others don't - so I don't know if that's a problem with this commit or a deeper problem.

Fixes #2041

[ammen99]

That said, windows split across multiple outputs don't seem to work at the moment - only one output displays its part of the window, the others don't - so I don't know if that's a problem with this commit or a deeper problem.

Windows are never meant to sit between outputs, this is by design in Wayfire.

Anyway.
I was baffled by the original bug at first but now seeing your fix I can see what is going on. The problem is that when we add the overlay inside an output_node_t, then the workspace-wall (the thing which is used to animate the transitions between workspaces) picks up the overlay as an actual view visible on the workspaces, because it blindly traverses the whole scenegraph and collects all output_node_t instances which belong to the current output. IOW:

• We disable the original view
• We add a new output_node_t which is rendered as an overlay
• The very same output_node_t is picked up by workspace-wall as part of the regular views, so it is drawn twice.

So your approach is actually on the right track! However we still need an intermediate node to translate the view to the correct output coordinates in multi-monitor scenarios. Does everything work properly if you use a translation node from the wayfire/unstable/translation-node.hpp and set its offset to be the output layout geometry .xy ? At worst, the view will be visible on multiple outputs during the transition (if you had placed it in a position like that) but at least it ought to be in the correct position. If we want to fix that as well, you'd need to extend the translation node or implement a new type of node which limits its children to a particular region, exactly like output_node_t does - however it needs to be a separate class so that dynamic_cast<output_node_t*> fails.

Maybe it would be worth refactoring our node code so that:

• Translation node can do both translation and clipping to a region
• Output nodes actually are subclasses of the translation+clipping node and simply set the translation and clip region properties according to the output they are part on.

This way we don't have duplicate functionality, plugins can do translation+clipping in a generic way (it would be useful for some other plugins as well).

If this sounds way too complicated, simply using a translation node without any clipping will still be an improvement and should be almost trivial to implement, and the rest can be left for later.

[lcolitti]

Not sure I understood exactly what you are suggesting, but... I changed overlay_view_node from an output_node_t to a translation_node_t. With a small but nonzero offset, like in this commit which uses an offset of (1, 1), things look almost correct. In that commit, the window moves down and to the right by 1 pixel while the animation is happening.

But with an offset of (0, 0), the window isn't displayed at all. While the switch is happening, the window disappears, and then when the animation ends, the window reappears. The translated node isn't drawn at all. I think this is because translation_node_instance_t::schedule_instructions doesn't do anything if the bounding box is the same as the children's bounding box.

[ammen99]

Your commit looks ok but I am not sure why offset 0,0 doesn't work. What do you mean exactly by "doesn't do anything if ..."? I don't see a line which matches your description https://github.com/WayfireWM/wayfire/blob/master/src/view/translation-node.cpp

[lcolitti]

What I'm talking about is this if statement. As far as I can tell, what is happening is that the bounding box is always 0,0,0,0 and our_damage.empty() is always false, so no instructions are scheduled.

Poking at the code some more looks like there might be a missing return statement in vswitch_overlay_node_t::get_bounding_box. If the view is locked, the code calculates the bounding box of the transformed node, but doesn't return it, instead falling out of the if and returning 0, 0, 0, 0. Fixing that seems to fix the problem. Patch coming up.

[ammen99]

This may look correct but is slightly misleading. The translation node has a boolean parameter (which in this particular case can be either true or false, it won't make a difference), and since output is a pointer, it is auto-converted to a bool.

You need to set the node's offset manually to origin(o->get_layout_geometry())

[lcolitti]

This may look correct but is slightly misleading. The translation node has a boolean parameter (which in this particular case can be either true or false, it won't make a difference), and since output is a pointer, it is auto-converted to a bool.

Oops! Fixed.

You need to set the node's offset manually to origin(o->get_layout_geometry())

Attempted.

[ammen99]

What I'm talking about is this if statement. As far as I can tell, what is happening is that the bounding box is always 0,0,0,0 and our_damage.empty() is always false, so no instructions are scheduled.

Poking at the code some more looks like there might be a missing return statement in vswitch_overlay_node_t::get_bounding_box. If the view is locked, the code calculates the bounding box of the transformed node, but doesn't return it, instead falling out of the if and returning 0, 0, 0, 0. Fixing that seems to fix the problem. Patch coming up.

Ah, nice catch!

[ammen99]

LGTM, thanks!

I'll test when I get back home & merge.

[marcusbritanicus]

Works great!! Thanks. :)