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

Overhaul of workspace code #194

Merged
merged 27 commits into from Oct 26, 2023
Merged

Overhaul of workspace code #194

merged 27 commits into from Oct 26, 2023

Conversation

Drakulix
Copy link
Member

@Drakulix Drakulix commented Oct 10, 2023

This is a large refactor of handling of workspaces and their properties.

Most importantly this gets rid of layouts spanning multiple outputs in global-workspace mode, causing every internal logical workspace to only have one assigned output. This means windows spanning multiple outputs aren't possible any more by design, but simplifies a lot of the shell/workspace code.

Additionally this introduces two new typed Coordinate-spaces Local and Global, which referr to Logical coordinates, that are either Output/Workspace-local (which is now the same thing, we previously effectively had three spaces without any typing) or in global coordinates. This should fix a couple of X11 bugs regarding multiple outputs and frozen rendering / misplaced popups / broken input.

Lastly this PR changes the maximize/fullscreen logic once again. Instead of treating maximized as a special fullscreen-like state, maximized windows now temporarily belong to the floating layer, resulting in much more tranditional maximized behaviour. The fullscreen code is thus now separated and ditches the CosmicWindow-wrapper again, which gets rid of window decorations for fullscreen windows (as it should be).

Point of this PR is to enable testing. It is still not done. Remaining TODOs:

  • re-implement Shell::set_mode
  • Handle xdg_shell::set_fullscreen output hint properly
  • Change window opacity during drag to hint output placement
  • Fix moves resizing floating windows
  • Limit cursor to current output during resize
  • Fix crash moving fullscreen window to other monitor
  • Dragged Xwayland windows can appear on the wrong monitor
  • Cleanup/forge proper git history
  • More testing

Should address:

@ids1024
Copy link
Member

ids1024 commented Oct 10, 2023

Always using a Space per output should also fix the issue #165 currently has switching between global and output-bound workspaces (it didn't seem worth tracking down why that was broken if we were going to change this anyway). So that should be unblocked by this too.

@jacobgkau
Copy link
Member

This means windows spanning multiple outputs aren't possible any more by design, but simplifies a lot of the shell/workspace code.

I don't see internal discussion about this, so just wanted to confirm, has @pop-os/ux approved this design decision?

  • Testing right now, it looks like a window does show up on both displays while I'm dragging it from one to the other, but as soon as I let it go, it disappears off the display where the cursor's not located.
    • I find this to be jarring in cases where I've dragged a long window just off the edge of a display, and it disappears from the display where most of the window's located in order to follow the cursor onto the display where only a small sliver is located.
  • If I try to resize a window on one display to also be on the other, it doesn't show up on the other one even while I'm dragging its edge (the mouse cursor just displays on the other monitor holding nothing)-- this is different behavior from movement, since it's not following the cursor.

The only use case for displaying a window on multiple displays that I've personally used in the past (that was an actual reason it needed to be done, rather than just being nice to see on both) was resizing a video editor to span both just to take a larger screenshot of the timeline. However, I can also imagine this might impact some video games, and probably other situation-specific use cases. I'd expect we'd be okay with not having this feature at launch, but it's probably a limitation we're going to want to move past eventually.

@maria-komarova
Copy link

Limiting windows to only be on one display at a time was a design decision. We couldn't find a lot of cases that warranted a very complicated code to enable people to have a window span both displays. I can imagine those pretty rare and infrequent one off cases when someone would need it but it still seems more of an edge case.
We'll need to improve indication for moving a window from one display to another - make half of the window where it won't show after releasing the mouse transparent, and that should help with recognizing what is happening, I believe.

@Drakulix
Copy link
Member Author

Drakulix commented Oct 10, 2023

I don't see internal discussion about this, so just wanted to confirm, has https://github.com/orgs/pop-os/teams/ux approved this design decision?

Yes, this was approved by @maria-komarova and @WatchMkr.

I find this to be jarring in cases where I've dragged a long window just off the edge of a display, and it disappears from the display where most of the window's located in order to follow the cursor onto the display where only a small sliver is located.

That might be difficult in terms of focus, but regardless the current design isn't the final one. The window is supposed to be slightly transparent on the workspace, it wouldn't be placed, when dragging. I guess that hint alone would make this less jarring?

If I try to resize a window on one display to also be on the other, it doesn't show up on the other one even while I'm dragging its edge (the mouse cursor just displays on the other monitor holding nothing)-- this is different behavior from movement, since it's not following the cursor.

That is a very good question though, I guess the expected behaviour in that case was never defined.

However, I can also imagine this might impact some video games, and probably other situation-specific use cases.

Video games mostly create a window per output, if they support multi-monitor setups. Something like nVidia Surround / AMD Eyefinity needs deeper integration to synchronize outputs anyway.

I'd expect we'd be okay with not having this feature at launch, but it's probably a limitation we're going to want to move past eventually.

In that case this refactoring would not have happened. As to support the feature I basically have to revert 80% of this PR again, most of the code was already there and was causing problems frequently. If this is going to be re-introduced, either that will happen much later or this PR should be dropped.

But I honestly see us rather supporting specific use-cases (e.g. maximizing/fullscreen across multiple outputs), than arbitrary overlaps from the previous discussions on this feature.

@maria-komarova
Copy link

If I try to resize a window on one display to also be on the other, it doesn't show up on the other one even while I'm dragging its edge (the mouse cursor just displays on the other monitor holding nothing)-- this is different behavior from movement, since it's not following the cursor.

I need to look more into it since it was never designed in detail but quick thought is we might want to limit the movement of the cursor to the focused display where the window is resized.

@ids1024
Copy link
Member

ids1024 commented Oct 10, 2023

I do wonder though if the behavior could in some way be better when a window is dragged to overlap two monitors. The part on one window simply ceasing to appear when the drag ends seems sub-optimal.

I guess the window suddenly moving at end of drag to fit on one monitor wouldn't be the best experience either.

@jacobgkau
Copy link
Member

I guess the window suddenly moving at end of drag to fit on one monitor wouldn't be the best experience either.

I assumed it would work like this, but I do think the current system (even before the transparency is implemented) is slightly more seamless. The problem is that the current way (at least before the transparency) makes it feel like dropping it on multiple displays should be possible. The transparency sounds like it may help.

If the decision's already been made that this will not be supported in the foreseeable future, I personally disagree (especially having already encountered at least one of the "rare and infrequent" use-cases myself), but that's fine/acknowledged. I mainly just wanted to have it on the record since it's a big change and difference from other environments.

@jackpot51
Copy link
Member

This does appear to fix #161

@jackpot51
Copy link
Member

The window behavior is very strange. Moving a floating window across displays works while dragging but when dragging stops only the part on the display with the majority of the window shows. While I don't specifically size windows across multiple displays I don't think we can reasonably discard parts of a window that are on different displays.

@jackpot51
Copy link
Member

I'm still seeing issues with X11 games hanging on a black screen. In general, it seems intensive X11 programs can slow down all X11 programs running.

@jacobgkau jacobgkau linked an issue Oct 10, 2023 that may be closed by this pull request
@WatchMkr
Copy link

#177 is still present. Occurs with single and multi-display setups. Tested with Google Chrome menus that normally move to the left of the original menu if they'd go off-screen.

@WatchMkr
Copy link

#178 is still present.

@WatchMkr
Copy link

#181 may be fixed; however, floating windows won't stay resized when they're moved (resize window, move via header) so can't tell for certain.

@WatchMkr
Copy link

WatchMkr commented Oct 10, 2023

New Issue: Multi-display changing display position bug

  1. Laptop display left | External display right
  2. Use wdisplays to move the laptop display to the right
  3. Can grab windows and move to the display on the right
  4. Bug - Can't use shortcuts to move windows or change focus to the laptop display on the right

EDIT: logged out and back in and this went away.

@WatchMkr
Copy link

New issue: maximized windows show the accent but shouldn't

@Quackdoc
Copy link
Contributor

I think gaming would take a hit from this for at least a couple niche users myself included. most games are at least somewhat responsive to arbitrary resolutions now. This means if the game can see it as a supported resolution, it can output it.

one of the setups I have done in that past. that I am looking forwards to is using gamescope to play games with arbitrary resolutions that span across some, but not all monitors. one of the setups I have used is [ 1920x1080 ], [ 3840x2160 ], [ 2560x1080 ] with a monitor on top. not super conventional, but my system is primarily for work, and that is what worked out for me. when you do a 2x integer scale on 4k it works out quite nicely. this is really easy to setup on windows, and even easier on linux thanks to gamescope

gamescope -h 1080 -w 5760 -H 1080 -W 5760 -- %command%

this can then nicely span the three displays, as if it was 3x 16:9 screens, leaving part of the ultrawide availible for discord or whatever other things the user may need.

I realize this is for sure an edge case, but it works fine, I'm not the most perceptive to desync across monitors, but I haven't really noticed much on kwin or sway.

@Vixea
Copy link

Vixea commented Oct 10, 2023

#152 is not fixed
#182 is fixed
Also just noticed they both end with a 2

@Drakulix
Copy link
Member Author

Drakulix commented Oct 11, 2023

I think gaming would take a hit from this for at least a couple niche users myself included.

Spanning multiple outputs for fullscreen application is a known use-case (as mentioned above), that will be supported at some point. But that alone doesn't warrant arbitrary overlaps, which have a bunch of additional challenges and problems.

Even partially overlapping fullscreen windows like you are describing, are easier to support on their own.

@Drakulix Drakulix linked an issue Oct 11, 2023 that may be closed by this pull request
@jacobgkau jacobgkau linked an issue Oct 11, 2023 that may be closed by this pull request
@ids1024
Copy link
Member

ids1024 commented Oct 12, 2023

A crash I saw running this branch:

Oct 11 17:04:23 pop-os cosmic-comp[120116]: thread 'main' panicked at 'assertion failed: was_floating != was_tiled.is_some()': src/shell/workspace.rs:673
                                               0: <backtrace::capture::Backtrace as core::default::Default>::default
                                               1: log_panics::Config::install_panic_hook::{{closure}}
                                               2: <alloc::boxed::Box<F,A> as core::ops::function::Fn<Args>>::call
                                                         at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/alloc/src/boxed.rs:1987:9
                                                  std::panicking::rust_panic_with_hook
                                                         at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/std/src/panicking.rs:695:13
                                               3: std::panicking::begin_panic_handler::{{closure}}
                                                         at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/std/src/panicking.rs:580:13
                                               4: std::sys_common::backtrace::__rust_end_short_backtrace
                                                         at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/std/src/sys_common/backtrace.rs:150:18
                                               5: rust_begin_unwind
                                                         at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/std/src/panicking.rs:578:5
                                               6: core::panicking::panic_fmt
                                                         at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/core/src/panicking.rs:67:14
                                               7: core::panicking::panic
                                                         at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/core/src/panicking.rs:117:5
                                               8: cosmic_comp::shell::workspace::Workspace::move_request
                                               9: cosmic_comp::shell::Shell::move_request
                                              10: smithay::wayland::shell::xdg::handlers::surface::toplevel::<impl wayland_server::dispatch::Dispatch<wayland_protocols::xdg::shell::generated::server::xdg_toplevel::XdgToplevel,smithay::wayland::shell::xdg::handlers::surface::XdgShellSurfaceUserData,D> for smithay::wayland::shell::xdg::XdgShellState>::request
                                              11: <wayland_server::dispatch::ResourceData<I,U> as wayland_backend::sys::server::ObjectData<D>>::request
                                              12: wayland_backend::sys::server_impl::resource_dispatcher
                                              13: <unknown>
                                              14: wl_event_loop_dispatch
                                              15: <core::cell::RefCell<calloop::sources::DispatcherInner<S,F>> as calloop::sources::EventDispatcher<Data>>::process_events
                                              16: calloop::loop_logic::EventLoop<Data>::run
                                              17: cosmic_comp::main
                                              18: std::sys_common::backtrace::__rust_begin_short_backtrace
                                              19: main
                                              20: __libc_start_call_main
                                                         at ./csu/../sysdeps/nptl/libc_start_call_main.h:58:16
                                              21: __libc_start_main_impl
                                                         at ./csu/../csu/libc-start.c:392:3
                                              22: _start

@Drakulix
Copy link
Member Author

Drakulix commented Oct 12, 2023

A crash I saw running this branch:

Oct 11 17:04:23 pop-os cosmic-comp[120116]: thread 'main' panicked at 'assertion failed: was_floating != was_tiled.is_some()': src/shell/workspace.rs:673

So this happened trying to move a window. And since the both was_floating and was_tiled were true the window was likely maximized (which is the only condition, that can trigger this). Though no matter how I try to replicate this setup it doesn't crash for me.

There is code in place to first unmaximize the window in this case, but potentially this isn't catching the state correctly at the same frame, so I added this commit as a potential fix for good measure: b205a54

Though I don't believe this is the cause, probably a more convoluted interaction caused this state mismatch. Though without steps to reproduce this will be difficult to track down.

@Drakulix Drakulix merged commit 7300e23 into master_jammy Oct 26, 2023
4 checks passed
@Drakulix Drakulix deleted the ws-refactor_jammy branch November 9, 2023 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
10 participants