-
Notifications
You must be signed in to change notification settings - Fork 149
improv(stack): optimize, improve styling, and fix keyboard focus #1568
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite the challenge to review, I would've really appreciated this to be split in a few more manageable commits - especially the larger refactorings like the Arc
and moving a lot of the logic into update
.
Refactored CosmicStack to queue window add/remove/swap messages for CosmicStackInternal's update method
Eliminates need for a Mutex when accessing windows attached to CosmicStackInternal
Lets CosmicStackInternal cache tab model data, and keep it in sync with window order changes
Allows CosmicStackInternal to redraw itself without needing to force a redraw
I am not sure, that this is a good idea for multiple reasons:
- We potentially call this in a completely different thread, which I am not sure is even valid for the wayland-objects involved here. The
Send
-impls here are not sound - just necessary for the render/main-logic split. Sadly iced doesn't allow us to split this as neatly resulting in this mess. - We potentially delay focus events even longer by doing this and only the next keyboard event afterwards actually dispatch the correct events, while e.g. modifiers/key events might still go to the previous window as long as we aren't done processing that queue. For that reason I believe, that this likely introduces even more corner-cases than it fixes.
A proper fix for the latter would be Smithay/smithay#1384, which would us allow to easily send these events, where they should happen.
Realistically we need to rework set_active
and the like to have access to the full State
to accomplish that and properly fix this.
Fixes keyboard focus sometimes being sent to the wrong window in the stack on focus changes
How does it fix that, btw? From what I can decipher, it doesn't look like we actually handle things differently, just at a different point of time.
Have you been able to debug where the code previous went wrong or are you simply not able to reproduce the issue after the refactor?
Sorry if that came over as a bit harsh.
These are all very welcome and I appreciate the design around the tab-model a lot. Given this is all bundled into a single commit, I just felt like I had to address the issues first, sorry! |
559b50d
to
d8995a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great apart from some minor issues!
src/shell/element/stack/tab_text.rs
Outdated
renderer.with_layer(bounds, |renderer| { | ||
if state.overflowed { | ||
let overlay = match (theme.cosmic().is_dark, self.selected) { | ||
(true, false) => Color { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it would be nice to move these into constants with proper names, but given they are only used once, this isn't important.
Rebased and removed the last commit. Still testing though |
Uh oh!
There was an error while loading. Please reload this page.