Skip to content

Commit

Permalink
feat(cli): add cycle-stack-index cmd
Browse files Browse the repository at this point in the history
This commit adds a new komorebi command "cycle-stack-index" which allows
the user to manipulate the index position of the focused window in the
focused stack by swapping it with either the previous or the next window
until the desired index position has been found.
  • Loading branch information
LGUG2Z committed Nov 12, 2024
1 parent 7f0b54c commit cc196db
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 0 deletions.
1 change: 1 addition & 0 deletions komorebi/src/core/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ pub enum SocketMessage {
StackWindow(OperationDirection),
UnstackWindow,
CycleStack(CycleDirection),
CycleStackIndex(CycleDirection),
FocusStackWindow(usize),
StackAll,
UnstackAll,
Expand Down
4 changes: 4 additions & 0 deletions komorebi/src/process_command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,10 @@ impl WindowManager {
self.cycle_container_window_in_direction(direction)?;
self.focused_window()?.focus(self.mouse_follows_focus)?;
}
SocketMessage::CycleStackIndex(direction) => {
self.cycle_container_window_index_in_direction(direction)?;
self.focused_window()?.focus(self.mouse_follows_focus)?;
}
SocketMessage::FocusStackWindow(idx) => {
// In case you are using this command on a bar on a monitor
// different from the currently focused one, you'd want that
Expand Down
33 changes: 33 additions & 0 deletions komorebi/src/window_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2022,6 +2022,39 @@ impl WindowManager {
self.update_focused_workspace(self.mouse_follows_focus, true)
}

#[tracing::instrument(skip(self))]
pub fn cycle_container_window_index_in_direction(
&mut self,
direction: CycleDirection,
) -> Result<()> {
self.handle_unmanaged_window_behaviour()?;

tracing::info!("cycling container window index");

let container =
if let Some(container) = self.focused_workspace_mut()?.monocle_container_mut() {
container
} else {
self.focused_container_mut()?
};

let len = NonZeroUsize::new(container.windows().len())
.ok_or_else(|| anyhow!("there must be at least one window in a container"))?;

if len.get() == 1 {
bail!("there is only one window in this container");
}

let current_idx = container.focused_window_idx();
let next_idx = direction.next_idx(current_idx, len);
container.windows_mut().swap(current_idx, next_idx);

container.focus_window(next_idx);
container.load_focused_window();

self.update_focused_workspace(self.mouse_follows_focus, true)

This comment has been minimized.

Copy link
@alex-ds13

alex-ds13 Nov 12, 2024

Contributor

@LGUG2Z It seems to me that there is a misunderstanding about the parameters of the function update_focused_workspace. From looking at the function itself it seems to me that the first parameter follow_focus should be true if we want the "actual" focus, as in the foreground window, to be "triggered" (if the second parameter trigger_focus is true) on the currently focused window (which looks in priority at maximized -> monocle -> containers -> desktop window, ignoring floating windows). If follow_focus is false then it only triggers the focus on desktop window if the workspace is empty (no matter what trigger_focus is), or it "triggers" focus (dependent on second parameter) on a stack container if it has no window that should show be above (like maximized -> monocle -> floating_windows), lastly it will also "trigger" focus (dependent on second parameter) on a maximized window if it exists.

This function is already somewhat confusing to me, it seems that there were some bugs that people tried to fix on this function even though it had nothing to do with it. To me this function was meant to be used to update the focused workspace (as in retile it correctly in case a container was added/removed for instace) and to update the foreground focus of the focused workspace. (maybe this second use shouldn't be here, since most times this "focus" is done after this function runs anyway...)

For situations like moving a container we would want follow_focus and trigger_focus to be true and in the case of sending a container we would want follow_focus to be false, I guess!
Even though that also doesn't make much sense since this function always applies to the focused workspace, so it is probably the other way around, when sending a container we want follow_focus and trigger_focus to be true to focus the window which should now be the focused one (since we moved the previous focused one away) and when moving a container we shouldn't need that since the container we moved had the focus and should still have it, however we would probably want to do it any way in case some other operation in between had changed the foreground focus.

TLDR: In summary, what I meant with this is there is nothing on this function that has to do with the mouse_follows_focus so that should not be set on the first parameter, however this is done in more than one place throughout the base code, I believe this might be the reason for some of the frustrations with the mouse_follows_focus not working that well... Also on this specific case of this commit, it doesn't matter what you set it to since there will be a stacked container, which means that it will always "trigger" focus (if the second parameter is true) on the stack container... However, after all this function runs, you still end up calling the window.focus() any way again on the line 252 of process_command.

TLDR of the TLDR: The update_focused_workspace function is currently confusing and might be the cause and also the fix of some weird issues. I might go and look for all its use cases to inspect if it is being used correctly or not since I suspect that sometimes it is not, but for that it should be better clarified what its use really is meant to be! And for that I need your help first!

PS.: Sorry for the long, long text! 😅

This comment has been minimized.

Copy link
@LGUG2Z

LGUG2Z Nov 12, 2024

Author Owner

The trigger_focus param was added by another contributor and honestly I don't understand it well myself at this point 😅

To me this function was meant to be used to update the focused workspace

This is true and it would be nice if we could find a way back to this ^

This comment has been minimized.

Copy link
@LGUG2Z

LGUG2Z Nov 12, 2024

Author Owner

b476bee#diff-f666a3179695ac6bb6f86a145553d66b1cc6aa2fcd2f687cbc633785e479b205

It was actually introduced in 2024, not that long ago 👀

This comment has been minimized.

Copy link
@LGUG2Z

LGUG2Z Nov 12, 2024

Author Owner

In fact, I believe that since moving to a message-based stackbar_manager module, the initial issue that this commit was targeting might not even exist anymore 🤔

This comment has been minimized.

Copy link
@alex-ds13

alex-ds13 Nov 12, 2024

Contributor

I can have a look to see if we can get rid of the focus (follow and trigger) part and make this just an update workspace function and maybe the focus of desktop when the workspace is empty. If needed we can create another function like trigger_focus to make sure we focus the current focused window or the desktop window. But I think that won't even be needed since in most cases that is being done on the function that calls this one anyway and in the cases where it's not we can add it. This way we can separate things, like a command can do its operations, call update_focused_workspace to make sure the tiles are correct and then depending on the command it may or may not need to focus some window, like a sending command needs to update focus but a moving command shouldn't need to... And that makes sense to be on the command function itself.

}

#[tracing::instrument(skip(self))]
pub fn focus_container_window(&mut self, idx: usize) -> Result<()> {
self.handle_unmanaged_window_behaviour()?;
Expand Down
7 changes: 7 additions & 0 deletions komorebic/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ gen_enum_subcommand_args! {
CycleMoveWorkspaceToMonitor: CycleDirection,
Stack: OperationDirection,
CycleStack: CycleDirection,
CycleStackIndex: CycleDirection,
FlipLayout: Axis,
ChangeLayout: DefaultLayout,
CycleLayout: CycleDirection,
Expand Down Expand Up @@ -985,6 +986,9 @@ enum SubCommand {
/// Cycle the focused stack in the specified cycle direction
#[clap(arg_required_else_help = true)]
CycleStack(CycleStack),
/// Cycle the index of the focused window in the focused stack in the specified cycle direction
#[clap(arg_required_else_help = true)]
CycleStackIndex(CycleStackIndex),
/// Focus the specified window index in the focused stack
#[clap(arg_required_else_help = true)]
FocusStackWindow(FocusStackWindow),
Expand Down Expand Up @@ -2305,6 +2309,9 @@ Stop-Process -Name:komorebi -ErrorAction SilentlyContinue
SubCommand::CycleStack(arg) => {
send_message(&SocketMessage::CycleStack(arg.cycle_direction))?;
}
SubCommand::CycleStackIndex(arg) => {
send_message(&SocketMessage::CycleStackIndex(arg.cycle_direction))?;
}
SubCommand::ChangeLayout(arg) => {
send_message(&SocketMessage::ChangeLayout(arg.default_layout))?;
}
Expand Down

0 comments on commit cc196db

Please sign in to comment.