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

Multiple keyboard-based focus/resize improvements #73

Merged
merged 12 commits into from
Mar 2, 2024

Conversation

github-usr-name
Copy link

  • Add hy3:resizenode dispatcher, which functions just like resizeactivewindow but applied to a whole node
  • Consider floating windows when handling hy3:movefocus
  • Consider other monitors when handling hy3:movefocus
  • Don't permit direction-based focusing of tiled windows which are completely obscured by a floating window

@outfoxxed
Copy link
Owner

outfoxxed commented Feb 18, 2024

I'm not currently available to do any code review, but

  • Consider floating windows when handling hy3:movefocus
  • Don't permit direction-based focusing of tiled windows which are completely obscured by a floating window

If these are not configurable they mist be to merge the PR without breaking peoples' workflows.

To clarify, considering floating windows when a floating window is actually selected is good behavior, but integrating them with the tiled layer is unexpected if you do. I don't mind an option or multiple for that existing but it must be configurable.

@github-usr-name
Copy link
Author

github-usr-name commented Feb 18, 2024

Thank you, I'll add those - had considered this already but chose not to predicated on avoiding exploding optionitis, hadn't considered principle of least surprise though. Proposal:

  • Add configuration field default_movefocus_layer with values "samelayer", "floating", "tiled", "all" ("same" meaning "operate at the same layer as the focused window). Default: "samelayer"
  • Add configuration field focus_obscured_windows <int>. Default: 1 (i.e. true - existing behaviour)
  • Add optional layer argument to hy3:movefocus Default: the value of the configuration field default_movefocus_layer

Rationale:

  • Behaviour is easily controllable at global level
  • Defaults should not be surprising
  • Fine-grained control is available for people with complex setups e.g. modal keymaps for window management

Please confirm that you're happy with this approach, I'll start implementing now working on basis that you are unless you say otherwise & obviously would prefer not to waste time if that proposal doesn't make sense to you.

@outfoxxed
Copy link
Owner

Sounds good to me, except I don't see the point of "floating"/"tiled" for default_movefocus_layer. It makes sense for the layer argument to movefocus though.

@github-usr-name
Copy link
Author

Done - I'm going to need to handle the hyprlang config change though

@github-usr-name
Copy link
Author

github-usr-name commented Feb 19, 2024

Righto, that's ready for review please. I changed the config slightly focus_obscured_windows -> focus_obscured_windows_policy with additional default option 2 which is effectively "smart"; if focus movement is on samelayer [default] then use current behaviour, if movement considers floating & tiled then don't focus obscured windows

@outfoxxed
Copy link
Owner

outfoxxed commented Feb 23, 2024

Ah sorry, missed this again, I'll review it today.
Also you do know your git identity is set to Pete Appleton right? Just wondering because its nowhere on your github account.

@github-usr-name
Copy link
Author

github-usr-name commented Feb 23, 2024 via email

@outfoxxed
Copy link
Owner

bump for likely missed edit since your email reply didn't include it

@github-usr-name
Copy link
Author

Git identity .... yep, a little bit of information leakage lol, thanks for pointing it out. I'm aware of it, have dropped a lot of paranoia since initially creating a GitHub account :).

Pete Appleton added 8 commits February 24, 2024 07:24
Move floating window if focused, even if tiled windows on same workspace

Navigate based on window middle

Feels unintuitive in use when floating overlaid on tiled

Fix: Set new monitor active when moving floating windows, remember previous workspace
Use last-focused-window when navigating to monitor by direction if it has same relative position as the logically closest window
Copy link
Owner

@outfoxxed outfoxxed left a comment

Choose a reason for hiding this comment

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

Have not tested functionality yet. May have more comments after that. Didn't see any actual bugs.

.gitignore Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
README.md Show resolved Hide resolved
src/BitFlag.hpp Outdated Show resolved Hide resolved
src/Hy3Layout.cpp Show resolved Hide resolved
src/Hy3Layout.cpp Outdated Show resolved Hide resolved
src/Hy3Layout.cpp Outdated Show resolved Hide resolved
@github-usr-name
Copy link
Author

Git identity .... yep, a little bit of information leakage lol, thanks for pointing it out.

Comments addressed & changes pushed, thank you for the review.

@outfoxxed
Copy link
Owner

Alright I gave it a test.

  • Some of the nodes focused, especially in "all" mode are kind of odd. Basing the target node off the nearest window edges instead of the center would likely help this quite a lot. Not a blocker for merging though.
  • Resizenode feels kind of bad to use and should have a way to deal with all sides and not just the bottom left. Also not going to block merging.
  • Multi-node navigation is broken (select a node larger than one window and try to move)
    Otherwise looks fine.

@outfoxxed outfoxxed merged commit f306ecc into outfoxxed:master Mar 2, 2024
@github-usr-name github-usr-name deleted the feature/focus-by-keyboard branch March 2, 2024 16:08
@outfoxxed
Copy link
Owner

@github-usr-name I had to revert this due to it breaking tab navigation. Feel free to re-open after thats fixed.

@github-usr-name
Copy link
Author

github-usr-name commented Mar 5, 2024 via email

@outfoxxed
Copy link
Owner

outfoxxed commented Mar 5, 2024 via email

@aacebedo
Copy link

@github-usr-name It was impossible to focus tabbed window. Do you plan to correct the bug? I was very interested by this functlionalities.

@github-usr-name
Copy link
Author

github-usr-name commented Mar 18, 2024 via email

@github-usr-name
Copy link
Author

github-usr-name commented Mar 23, 2024

Finally had a change to look. Despite seeing video of the issue I'm completely unable to replicate it. @aacebedo / @outfoxxed , it would be extremely helpful if you could please:

  • Provide a complete copy of your hyprland.conf (redacted as needed)
  • Confirm that you still observe the problem if you use my config (attached)

My code is on branch feature/focus-by-keyboard at https://github.com/github-usr-name/hy3/ - I don't want to reopen the PR until I've fixed the issue.

My hyprland.conf
Noddy test script
Test video

@outfoxxed
Copy link
Owner

@github-usr-name apologies, took me a while to get to this. anyway it's still reproducible for me.

Stripped down config used below
plugin = /home/admin/programming/outfoxxed/hyprland/hy3/build/libhy3.so

general {
    layout = hy3
}

debug {
    enable_stdout_logs = true
}

monitor = , preferred, auto, 1

$mod = ALT

bind = $mod+SHIFT, m, exit

bind = $mod, return, exec, alacritty
bind = $mod+SHIFT, q, hy3:killactive

bind = $mod, d, hy3:makegroup, h
bind = $mod, s, hy3:makegroup, v
bind = $mod, z, hy3:makegroup, tab
bind = $mod, a, hy3:changefocus, raise
bind = $mod+SHIFT, a, hy3:changefocus, lower

bindm = $mod, mouse:272, movewindow
bindm = $mod, mouse:273, resizewindow
bindn = , mouse:272, hy3:focustab, mouse
bindn = , mouse_down, hy3:focustab, l, require_hovered
bindn = , mouse_up, hy3:focustab, r, require_hovered

bind = $mod, h, hy3:movefocus, l
bind = $mod, j, hy3:movefocus, d
bind = $mod, k, hy3:movefocus, u
bind = $mod, l, hy3:movefocus, r
bind = $mod, left, hy3:movefocus, l
bind = $mod, down, hy3:movefocus, d
bind = $mod, up, hy3:movefocus, u
bind = $mod, right, hy3:movefocus, r

bind = $mod+CONTROL, h, hy3:movefocus, l, visible
bind = $mod+CONTROL, j, hy3:movefocus, d, visible
bind = $mod+CONTROL, k, hy3:movefocus, u, visible
bind = $mod+CONTROL, l, hy3:movefocus, r, visible
bind = $mod+CONTROL, left, hy3:movefocus, l, visible
bind = $mod+CONTROL, down, hy3:movefocus, d, visible
bind = $mod+CONTROL, up, hy3:movefocus, u, visible
bind = $mod+CONTROL, right, hy3:movefocus, r, visible

bind = $mod+SHIFT, h, hy3:movewindow, l, once
bind = $mod+SHIFT, j, hy3:movewindow, d, once
bind = $mod+SHIFT, k, hy3:movewindow, u, once
bind = $mod+SHIFT, l, hy3:movewindow, r, once
bind = $mod+SHIFT, left, hy3:movewindow, l, once
bind = $mod+SHIFT, down, hy3:movewindow, d, once
bind = $mod+SHIFT, up, hy3:movewindow, u, once
bind = $mod+SHIFT, right, hy3:movewindow, r, once

bind = $mod+CONTROL+SHIFT, h, hy3:movewindow, l, once, visible
bind = $mod+CONTROL+SHIFT, j, hy3:movewindow, d, once, visible
bind = $mod+CONTROL+SHIFT, k, hy3:movewindow, u, once, visible
bind = $mod+CONTROL+SHIFT, l, hy3:movewindow, r, once, visible
bind = $mod+CONTROL+SHIFT, left, hy3:movewindow, l, once, visible
bind = $mod+CONTROL+SHIFT, down, hy3:movewindow, d, once, visible
bind = $mod+CONTROL+SHIFT, up, hy3:movewindow, u, once, visible
bind = $mod+CONTROL+SHIFT, right, hy3:movewindow, r, once, visible

With this config you can observe that hy3:movefocus, [l|d|u|r] do not move between tabs, with or without visible.

recording.mp4

@github-usr-name
Copy link
Author

github-usr-name commented Apr 4, 2024

And yet, with minimal config changes (just plugin path & terminal) I have totally different results!!

OK, in the interests of eliminating things, then I am going to install alacritty; can you please supply your config on the tiniest little possible outside chance that it's something like an OSC sequence? Assuming that that doesn't show the problem then I'll prepare a branch with "insane logging" later on and ask if you could please supply the log output from that.

test.webm

test.log

(still not reproducible for me with stock alacritty install via pacman)

@postsolar
Copy link

@github-usr-name

I think there's a regression on this branch introduced by some recent Hyprland changes. I think it's since 0.38 or so, before that I've been using your branch just fine. When I use hy3:movefocus on a floating window Hyprland freezes and eventually crashes. I get this with a pretty much the default config. Did you experience this too recently?

@github-usr-name
Copy link
Author

github-usr-name commented Apr 9, 2024 via email

@postsolar
Copy link

I noticed I'm getting the same freeze+crash if I do:

  1. Open two windows, expand one of the windows with hy3:expand expand
  2. Try to use hy3:movefocus in the direction where the other window was

I checked and it doesn't happen on base hy3.

@github-usr-name
Copy link
Author

github-usr-name commented Apr 11, 2024 via email

@github-usr-name
Copy link
Author

github-usr-name commented Apr 11, 2024 via email

@postsolar
Copy link

@github-usr-name sorry I didn't notice this message — although I did update and was using the new version without knowing this issue is fixed :D Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants