Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions niri-ipc/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1291,6 +1291,12 @@ pub struct Window {
///
/// There can be either one focused window or zero (e.g. when a layer-shell surface has focus).
pub is_focused: bool,
/// The fullscreen state of this window.
///
/// - `0`: not fullscreen (tiled or floating)
/// - `1`: maximized
/// - `2`: fullscreen
pub fullscreen_state: u8,
/// Whether this window is currently floating.
///
/// If the window isn't floating then it is in the tiling layout.
Expand Down Expand Up @@ -1532,6 +1538,16 @@ pub enum Event {
/// Id of the newly focused window, or `None` if no window is now focused.
id: Option<u64>,
},
/// 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,
},
Comment on lines +1541 to +1550

Choose a reason for hiding this comment

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

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

Suggested change
/// 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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

/// Window focus timestamp changed.
///
/// This event is separate from [`Event::WindowFocusChanged`] because the focus timestamp only
Expand Down
11 changes: 11 additions & 0 deletions niri-ipc/src/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,17 @@ impl EventStreamStatePart for WindowsState {
win.layout = update;
}
}
Event::WindowFullscreenStateChanged {
id,
fullscreen_state,
} => {
for win in self.windows.values_mut() {
if win.id == id {
win.fullscreen_state = fullscreen_state;
break;
}
}
}
event => return Some(event),
}
None
Expand Down
8 changes: 8 additions & 0 deletions src/ipc/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,12 @@ pub fn handle_msg(mut msg: Msg, json: bool) -> anyhow::Result<()> {
Event::WindowFocusChanged { id } => {
println!("Window focus changed: {id:?}");
}
Event::WindowFullscreenStateChanged {
id,
fullscreen_state,
} => {
println!("Window {id}: fullscreen state changed to {fullscreen_state}");
}
Event::WindowFocusTimestampChanged {
id,
focus_timestamp,
Expand Down Expand Up @@ -659,6 +665,8 @@ fn print_window(window: &Window) {
if window.is_floating { "yes" } else { "no" }
);

println!(" Fullscreen state: {}", window.fullscreen_state);

if let Some(pid) = window.pid {
println!(" PID: {pid}");
} else {
Expand Down
13 changes: 12 additions & 1 deletion src/ipc/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ use smithay::wayland::shell::wlr_layer::{KeyboardInteractivity, Layer};

use crate::backend::IpcOutputMap;
use crate::input::pick_window_grab::PickWindowGrab;
use crate::layout::workspace::WorkspaceId;
use crate::layout::{workspace::WorkspaceId, LayoutElement};
use crate::niri::State;
use crate::utils::{version, with_toplevel_role};
use crate::window::Mapped;
Expand Down Expand Up @@ -504,6 +504,9 @@ fn make_ipc_window(
workspace_id: Option<WorkspaceId>,
layout: WindowLayout,
) -> niri_ipc::Window {
// 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();
Comment on lines +507 to +509

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

with_toplevel_role(mapped.toplevel(), |role| niri_ipc::Window {
id: mapped.id().get(),
title: role.title.clone(),
Expand All @@ -512,6 +515,7 @@ fn make_ipc_window(
workspace_id: workspace_id.map(|id| id.get()),
is_focused: mapped.is_focused(),
is_floating: mapped.is_floating(),
fullscreen_state,
is_urgent: mapped.is_urgent(),
layout,
focus_timestamp: mapped.get_focus_timestamp().map(Timestamp::from),
Expand Down Expand Up @@ -726,6 +730,13 @@ impl State {
events.push(Event::WindowFocusChanged { id: Some(id) });
}

if mapped.sizing_mode() != ipc_win.fullscreen_state.into() {
events.push(Event::WindowFullscreenStateChanged {
id,
fullscreen_state: mapped.sizing_mode().into(),
});
}

let focus_timestamp = mapped.get_focus_timestamp().map(Timestamp::from);
if focus_timestamp != ipc_win.focus_timestamp {
events.push(Event::WindowFocusTimestampChanged {
Expand Down
21 changes: 21 additions & 0 deletions src/layout/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -526,6 +526,27 @@ impl SizingMode {
}
}

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,
}
}
}

Comment on lines +529 to +549

Choose a reason for hiding this comment

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

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

impl<W: LayoutElement> InteractiveMoveState<W> {
fn moving(&self) -> Option<&InteractiveMoveData<W>> {
match self {
Expand Down