Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Render only layer shell surfaces when cosmic-workspaces is open, and add a SplitRenderElements #609

Merged
merged 6 commits into from
Jul 15, 2024

Conversation

ids1024
Copy link
Member

@ids1024 ids1024 commented Jul 10, 2024

This helps with issues like pop-os/cosmic-workspaces-epoch#59.

I'm not entirely happy with the growing complexity of workspace_elements. I was thinking of a couple ways to split it up, noticed the most redundant part was the repeated code extending p_elements and w_elements. And the cleanest way to de-duplicate that out is with a dedicated SplitRenderElements type, which could be good in general. I have a commit adding that.

This also adds the commit from #459, which makes more of a difference now. (If we wanted an animation between workspaces with this, we'd need to make sure cosmic-comp and cosmic-comp animate in sync.)

This seems to generally work with cosmic-workspaces modified to leave the background transparent. But I still need to do a bit more testing with it. (And I guess make this change on apply to the X11/winit backends.)

Copy link
Member

@Drakulix Drakulix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM on first glance. The SplitRenderElements are definitely an improvement as is the ElementFilter, even if we should definitely investigate minimizing allocations at some point.

Vec::new()
elements
.p_elements
.extend(overlay_elements.p_elements.into_iter().map(Into::into));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not overlay_elements.join or chain at least? (Which should probably still yield an ExactSizeIterator, which should be good for extend.

src/shell/mod.rs Show resolved Hide resolved
.map_err(|_| OutputNoMode)?,
offset.to_physical_precise_round(output_scale),
);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the workspace elements from the "previous" workspace also be ignored the same? Or does this rely on animations being disabled for the overview?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it doesn't make a difference with the animations disabled, though it wouldn't hurt to add another if condition for that.

@ids1024
Copy link
Member Author

ids1024 commented Jul 10, 2024

even if we should definitely investigate minimizing allocations at some point.

Yeah. Can't really use iterators currently, since something like background_layer_elements would error due to having to call split_layer_elements twice with an &mut R. And it would generally make things a mess... Maybe internal iteration where everything including AsRenderElements takes a callback to call with each element could work.

Need to come up with a good benchmark for this.

ids1024 added 5 commits July 11, 2024 18:15
`(w_elements, p_elements)` tuples are used in a bunch of places. A
struct with named fields is generally an improvement just due to the
fact the order is non-obvious.

But we can also add methods. In particular,
`extend_from_workspace_elements` abstracts out some of the more
redundant code in `workspace_elements`.

It would be nice to avoid allocation everywhere, but iterators would
complicate lifetimes, run into issues with needing multiple mutable
borrows to things like the `Renderer`, and be awkward in certain
functions without generator syntax. In any case, cosmic-comp already
relies on allocating vectors here.

If this abstraction is commonly useful in compositors, perhaps it could
be moved to Smithay.
Fixes pop-os/cosmic-workspaces-epoch#27.

We want this to apply to changes to workspace either through keybindings
or the cosmic-workspaces UI, so it adding a check here seems reasonable.
In principle it could be good to have some kind of privileged protocol
for setting things like this.

We may also want a configuration option to disable animations at some
point.
This allows `cosmic-workspaces` to rely on cosmic-comp for rendering the
background, and just have transparency. This should be a more reliable
and performant way of doing things, at least for now.

Instead of adding another opaque bool argument, this defines an
`ElementFilter` enum, which makes calls more readable.

Window surfaces are still included in screencopy, as needed for the
workspace previews.
This is increasingly not just related to screencopy, so it's weird to
add there. I don't see any other module that fits, so add one called
"quirks" (like the Linux kernel uses for device-specific handling in
generic drives).
This way the same behavior will apply in winit/x11 backends.
@ids1024 ids1024 force-pushed the cosmic-workspaces-bg branch from 935e400 to 24e5a15 Compare July 13, 2024 02:50
@ids1024 ids1024 marked this pull request as ready for review July 13, 2024 02:50
src/input/mod.rs Outdated Show resolved Hide resolved
Without animation between workspaces, the behavior is a bit jarring.
Disable for now until we have a better solution.
@ids1024 ids1024 force-pushed the cosmic-workspaces-bg branch from 24e5a15 to a996f64 Compare July 15, 2024 14:59
Copy link
Member

@Drakulix Drakulix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now :)

@Drakulix Drakulix merged commit 7acfa10 into master Jul 15, 2024
ids1024 added a commit to pop-os/cosmic-workspaces-epoch that referenced this pull request Jul 15, 2024
Requires pop-os/cosmic-comp#609, or this will
show all the open windows.
@Drakulix Drakulix deleted the cosmic-workspaces-bg branch September 18, 2024 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants