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

feat: add focus-column-or-monitor-left, focus-column-or-monitor-right #456

Merged
merged 9 commits into from
Jun 28, 2024

Conversation

lpnh
Copy link
Contributor

@lpnh lpnh commented Jun 20, 2024

Partially addresses: #296

This is a preliminary sketch for implementing focus-column-or-monitor-left and focus-column-or-monitor-right, which expand the default left and and right focus behaviors. If a window is available in the target direction, it receives focus; otherwise, the focus shifts to the corresponding monitor.

Current state: it kinda works, but there are cases that still need to be fixed. I will try to provide a better description soon.

To be completely honest, I'm new to the Niri codebase, so there's a lot I'm still unsure about. I was hoping to provide a good starting point for this PR, but it seems like I still have a lot to learn.

Any feedback would be greatly appreciated.

@lpnh lpnh marked this pull request as ready for review June 21, 2024 22:02
@lpnh
Copy link
Contributor Author

lpnh commented Jun 21, 2024

Well, it seems both focus-window-or-monitor-left and focus-window-or-monitor-right are now working as expected.

A couple of things about the implementation:

maybe_focus_window_or_monitor_*

  • I'm not entirely sure if using a boolean here is the most idiomatic approach, but it seems like a reasonable solution to avoid adding further complexity.
  • The "maybe" prefix try to suggest that this function depends on the availability of the output and that it's not solely responsible for handling the focus.

FocusWindowOrMonitor*

  • Given the need to manage the output and handle three different possible cases, the structure ended up quite clear.
  • The main challenge seems to be handling both focus and cursor at the same time.

Now, about the test... I'm not quite sure how to write it properly. I attempted to provide some, but I am unsure if they make any sense.

This was very fun, so I wouldn't mind rewriting anything you think could be improved. Also, feel free to take it from here.

Copy link
Owner

@YaLTeR YaLTeR left a comment

Choose a reason for hiding this comment

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

Sorry for taking so long to review, just some minor changes needed.

I'm not entirely sure if using a boolean here is the most idiomatic approach

It's alright.

Now, about the test... I'm not quite sure how to write it properly. I attempted to provide some, but I am unsure if they make any sense.

Seems correct. Just random index should be from 1 because the output random indices are 1 to 5.

src/layout/mod.rs Outdated Show resolved Hide resolved
src/layout/mod.rs Outdated Show resolved Hide resolved
src/layout/mod.rs Outdated Show resolved Hide resolved
@lpnh lpnh requested a review from YaLTeR June 28, 2024 12:22
Copy link
Owner

@YaLTeR YaLTeR left a comment

Choose a reason for hiding this comment

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

Looks good.

Copy link
Owner

@YaLTeR YaLTeR left a comment

Choose a reason for hiding this comment

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

Wait, I just realized. Could you rename Window to Column throughout? Left-Right focus goes by columns, not by windows, and other actions are also focus-column-left, etc.

@lpnh
Copy link
Contributor Author

lpnh commented Jun 28, 2024

Wait, I just realized. Could you rename Window to Column throughout? Left-Right focus goes by columns, not by windows, and other actions are also focus-column-left, etc.

Sure! I had the same thought initially. We might have been influenced by the initial request. Thanks for catching this.

@lpnh
Copy link
Contributor Author

lpnh commented Jun 28, 2024

May I move them next to the other Column variables?

@YaLTeR
Copy link
Owner

YaLTeR commented Jun 28, 2024

Yeah

@lpnh lpnh changed the title feat: add focus-window-or-monitor-left, focus-window-or-monitor-right feat: add focus-column-or-monitor-left, focus-column-or-monitor-right Jun 28, 2024
@YaLTeR YaLTeR merged commit bdf9894 into YaLTeR:main Jun 28, 2024
9 checks passed
@YaLTeR
Copy link
Owner

YaLTeR commented Jun 28, 2024

Thanks!

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.

2 participants