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

Add More Monitor 'Auto' Positions. #5670

Merged
merged 13 commits into from Apr 23, 2024
Merged

Conversation

The-Briel-Deal
Copy link
Contributor

Describe your PR, what does it fix/add?

Resolves #5573.

The objective of this PR is to add more automatic monitor placement options. Currently, you can specify a monitors position as auto and it will automatically place the monitor to the right. It was requested however that we enable automatically placing monitor in other directions. So this PR adds the ability to place monitors up, down, left, and right with the keywords auto-up, auto-down, auto-left, and auto-right.

Is there anything you want to mention? (unchecked code, possible bugs, found problems, breaking compatibility, etc.)

It was also mentioned that having corner directions would be desirable as well. But adding this leads to significant ambiguity as to what would actually happen. For example with auto-up-right where would the monitors connect? On the right top or right side? What happens if this monitor overlaps with another monitor due to an odd resolution or aspect ratio?

So, in this PR I kept things simple which I believe will cover most general use cases without introducing many new edge cases.

That said, in the future I would like to propose further improvements to the monitor placement system. For example, I think it would be nice to make a visual tui curses tool to align your monitors with a set scale and resolution. I also think we should implement a way to correct or warn on poor monitor placement if a user accidentally overlaps monitors or makes it so that they aren't connected.

Is it ready for merging, or does it need work?

I have done basic testing but I would like to play with some edge cases tomorrow as I've only been able to test my two monitors (both 16:9 with one rotated). I think its fine to review now though.

@vaxerski
Copy link
Member

For example, I think it would be nice to make a visual tui curses tool to align your monitors with a set scale and resolution

https://github.com/artizirk/wdisplays

I also think we should implement a way to correct or warn on poor monitor placement if a user accidentally overlaps monitors or makes it so that they aren't connected.

sounds fair

Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

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

rest lgtm

src/helpers/Monitor.hpp Outdated Show resolved Hide resolved
src/helpers/Monitor.hpp Outdated Show resolved Hide resolved
src/config/ConfigManager.cpp Outdated Show resolved Hide resolved
@The-Briel-Deal
Copy link
Contributor Author

For example, I think it would be nice to make a visual tui curses tool to align your monitors with a set scale and resolution

https://github.com/artizirk/wdisplays

Woahhh, thanks for bringing that to my attention. Thanks for the quick feedback though. Fixing now.

@The-Briel-Deal
Copy link
Contributor Author

@vaxerski All your feedback has been addressed. Just testing a bunch of different layouts to see if I can break this but feel free to review it now.

@The-Briel-Deal
Copy link
Contributor Author

The-Briel-Deal commented Apr 21, 2024

Edge case found. If you declare your first monitor as auto-up and then your second monitor as auto left they will only connect at the bottom left and top right corner. The solution to this could be to always make users have their first declared monitor be auto or auto-right that way there is always a monitor in the center of others.

Alternatively we could just leave this behavior and note it in the docs + give users a warning if one of their monitors isn't connected.
Sad monitors

Thoughts Vaxry?

@vaxerski
Copy link
Member

We can make the first declared always at pos 0,0, sounds reasonable

@The-Briel-Deal
Copy link
Contributor Author

We can make the first declared always at pos 0,0, sounds reasonable

Implemented. Not really sure why why the Nix stuff is failing. It looks like it is in upstream too.

@vaxerski
Copy link
Member

ignore the nix stuff

vaxerski
vaxerski previously approved these changes Apr 22, 2024
Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

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

lgtm, just needs a wiki mr and a clang-format

@fufexan
Copy link
Member

fufexan commented Apr 22, 2024

You can rebase to fix the Nix CI. Well you need to rebase anyway due to conflicts.

@The-Briel-Deal
Copy link
Contributor Author

The-Briel-Deal commented Apr 23, 2024

@vaxerski clang-format has been run, feature branch has been rebased, and docs have been written! hyprwm/hyprland-wiki#610

Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

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

thanks :)

@vaxerski vaxerski merged commit 464441e into hyprwm:main Apr 23, 2024
9 checks passed
vaxerski pushed a commit that referenced this pull request Apr 23, 2024
* Reverse Window Positioning.

* Cleanup old comments and logs.

* Finish Splitting Left and Right offset.

* Forgot to add Auto Left to ConfigManager

* Fix problems with auto_left.

* Nearly finish up and down.

* Finish draft of all four dirs. Testing now.

* Change Y value in moveTo for up and down.

* Format, comment, and cleanup.

* Address Vaxry's feedback.

* Add check to see if auto position is first rule.

* Run clang-format.
vaxerski pushed a commit that referenced this pull request Apr 23, 2024
* Reverse Window Positioning.

* Cleanup old comments and logs.

* Finish Splitting Left and Right offset.

* Forgot to add Auto Left to ConfigManager

* Fix problems with auto_left.

* Nearly finish up and down.

* Finish draft of all four dirs. Testing now.

* Change Y value in moveTo for up and down.

* Format, comment, and cleanup.

* Address Vaxry's feedback.

* Add check to see if auto position is first rule.

* Run clang-format.
vaxerski pushed a commit that referenced this pull request Apr 23, 2024
* Reverse Window Positioning.

* Cleanup old comments and logs.

* Finish Splitting Left and Right offset.

* Forgot to add Auto Left to ConfigManager

* Fix problems with auto_left.

* Nearly finish up and down.

* Finish draft of all four dirs. Testing now.

* Change Y value in moveTo for up and down.

* Format, comment, and cleanup.

* Address Vaxry's feedback.

* Add check to see if auto position is first rule.

* Run clang-format.
@The-Briel-Deal
Copy link
Contributor Author

thanks :)

Np! You've been a great maintainer and help with me getting started contributing to open source! I appreciate that and all the work you do!

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.

Add More Monitor 'Auto' Positions
3 participants