Add windows' fullscreen states to ipc #2836
Conversation
nerdwave-nick
left a comment
nerdwave-nick
left a comment
There was a problem hiding this comment.
Posting this with the disclaimer that I barely know rust and this is my first look ever at the niri code base, but I think the simplification I have proposed could be a general improvement for clarity and simplicity. Unless, of course, there are good reasons to do it the way it is.
Thank you for being my experiment/first attempt at reading the niri code 😄
| // Quick hack to fix the consistent halting if we tried to access the sizing mode | ||
| // inside the constructor | ||
| let fullscreen_state = mapped.sizing_mode().into(); |
There was a problem hiding this comment.
Will the "quick hack" stay? Usually a hack is not really a good thing for a project, although I don't have insight if this is avoidable or not
There was a problem hiding this comment.
well, it's not a hack per se. the mapped.sizing_mode().into(); was supposed to go inside the constructor below, but i believe mapped gets locked inside the with_toplevel_role method, so when i try and access it inside the same funciton a deadlock occurs. i could be totally wrong here, i'm not sure if this is intended of with_toplevel_role, but saving the fullscreen_state beforehand avoids any halts.
There was a problem hiding this comment.
Hmmm... It's not necessary for any of the other values. That looks weird to me, but perhaps I'm just not knowledgeable enough regarding the code
| /// A window's fullscreen state was changed changed. | ||
| WindowFullscreenStateChanged { | ||
| /// Id of the window. | ||
| id: u64, | ||
| /// Fullscreen state for the window | ||
| /// - `0`: not fullscreen (tiled or floating) | ||
| /// - `1`: maximized | ||
| /// - `2`: fullscreen | ||
| fullscreen_state: u8, | ||
| }, |
There was a problem hiding this comment.
Have you considered using sizing_mode: SizingMode instead? This could get rid of one translation (fullscreen_state -> sizing_mode), which can help with general reasoning about this code. Would also save on the comments above. Something like
| /// A window's fullscreen state was changed changed. | |
| WindowFullscreenStateChanged { | |
| /// Id of the window. | |
| id: u64, | |
| /// Fullscreen state for the window | |
| /// - `0`: not fullscreen (tiled or floating) | |
| /// - `1`: maximized | |
| /// - `2`: fullscreen | |
| fullscreen_state: u8, | |
| }, | |
| /// A window's sizing mode was changed changed. | |
| WindowSizingModeChanged { | |
| /// Id of the window. | |
| id: u64, | |
| sizing_mode: SizingMode, | |
| }, |
This would of course go with changes in the rest of the PR, but if there is no good reason to do the sizing_mode -> fullscreen_state translation, this would help with the mental indirection, and make things more obvious, which is always good in a large (or any) code base.
There was a problem hiding this comment.
Yeah, i actually considered this when creating the pr, but i instead decided to follow Hyprland's implementation.
this would help with the mental indirection, and make things more obvious
I agree, this would also eliminate some useless conversions between u8 and SizingMode that only gets used once.
There was a problem hiding this comment.
Small issue is that SizingMode is defined inside the niri crate and is therefore inaccessable inside niri_ipc. it's possible to move the definition and implementations from niri to niri_ipc, but this would be a breaking change and it's just stupid to refactor the entire project for a single featture.
There was a problem hiding this comment.
Flexing my missing rust knowledge - could you serialzie the SizingMode as is? Who cares if it comes out as a lil string or number. But idk how rust is about this stuff
| impl From<u8> for SizingMode { | ||
| fn from(value: u8) -> Self { | ||
| match value { | ||
| 0 => Self::Normal, | ||
| 1 => Self::Maximized, | ||
| 2 => Self::Fullscreen, | ||
| _ => Self::Normal, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl From<SizingMode> for u8 { | ||
| fn from(value: SizingMode) -> Self { | ||
| match value { | ||
| SizingMode::Normal => 0, | ||
| SizingMode::Maximized => 1, | ||
| SizingMode::Fullscreen => 2, | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Instead of this, the SizingMode could instead be a unit only enum, which saves on these functions, and goes hand in hand with the idea I proposed to change it to sizing_mode instead of fullscreen_state
2 participants
Solves #discussion:1843.
Adds a
fullscreen_stateto the Window struct, and aWindowFullscreenStateChangedevent.