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: auto_group #7883

Merged
merged 23 commits into from
Oct 2, 2024
Merged

feat: auto_group #7883

merged 23 commits into from
Oct 2, 2024

Conversation

Aqa-Ib
Copy link
Contributor

@Aqa-Ib Aqa-Ib commented Sep 22, 2024

Describe your PR, what does it fix/add?

The behavior before this was to auto group only tiled groups when the tiling mode was enabled (allfloat=false on the workspace). This PR gives you the auto group feature for tiled and floating groups when the tiling mode is enabled and also when the floating mode is enabled (allfloat=true on the workspace).

Furthermore, this gives you the freedom to configure whether new windows will be automatically grouped into the focused unlocked group. Wiki

Fixes: #7840

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

It is ready.

@Edgars-Cirulis
Copy link

Clang format 🙏

@zakk4223
Copy link
Contributor

Wouldn't it be better to extend the grouping window rules to include the desired floating group behavior?

@Aqa-Ib
Copy link
Contributor Author

Aqa-Ib commented Sep 23, 2024

Yes, when we solve the bug, I will do that if possible.

@zakk4223
Copy link
Contributor

I'm saying that group:auto_group shouldn't exist if you can do it via window rules. Having two ways to do the exact same thing is confusing. On top of that you have to worry about how the two methods interact

@Aqa-Ib
Copy link
Contributor Author

Aqa-Ib commented Sep 23, 2024

I'm saying that group:auto_group shouldn't exist if you can do it via window rules.

This PR is for general behavior that target all new windows created when focusing a group. In contrast, window rules target a window following regex, it is used for a specific target, or fine tuning.

On top of that you have to worry about how the two methods interact

They don't conflict with each other.

@zakk4223
Copy link
Contributor

You can target all windows with a window rule

@Aqa-Ib
Copy link
Contributor Author

Aqa-Ib commented Sep 23, 2024

Yes. The behavior that you can not target with window rules and what this code does is the following: when you are focusing a tiled or floating group, new windows that you create will be part of it. If you are not focusing the group though, or if the group is locked, new windows will take a new surface. Part of this code already exists in main hyprland as can be seen in the diff for tiling groups, but it is completed with tiled and floated groups in tiling and floating mode, and with a user config allowing to enable or disable it.

@Aqa-Ib
Copy link
Contributor Author

Aqa-Ib commented Sep 24, 2024

Ok, the floating group bug is fixed. Now I will try to fix the bugs remaining.

@Edgars-Cirulis
Copy link

Could you rebase commits? So it has a commit with clang format so it doesn't have multiple times repead commit names as clang-format

@Aqa-Ib Aqa-Ib force-pushed the auto_group branch 3 times, most recently from 3c24673 to f7151e5 Compare September 24, 2024 13:27
@Aqa-Ib
Copy link
Contributor Author

Aqa-Ib commented Sep 24, 2024

Done rebasing for cleaning up the commit history.

@Aqa-Ib Aqa-Ib force-pushed the auto_group branch 3 times, most recently from 9b0fb4e to a978564 Compare September 24, 2024 14:05
@Aqa-Ib Aqa-Ib force-pushed the auto_group branch 2 times, most recently from 9a5a996 to 479effc Compare September 24, 2024 17:55
is no longer reproduceable
@Aqa-Ib
Copy link
Contributor Author

Aqa-Ib commented Sep 27, 2024

Nevermind, Xwayland's popups are broken...

@Aqa-Ib
Copy link
Contributor Author

Aqa-Ib commented Sep 28, 2024

Fixed. I'm gonna try to refactor the code now.

@Aqa-Ib
Copy link
Contributor Author

Aqa-Ib commented Sep 28, 2024

@vaxerski I have unified all the code into the new function:
bool IHyprLayout::onWindowCreatedAutoGroup(PHLWINDOW pWindow)
Please check it out, I truly believe that it is ready for merging now. Cheers!

src/layout/IHyprLayout.cpp Show resolved Hide resolved
src/layout/IHyprLayout.cpp Outdated Show resolved Hide resolved
src/layout/IHyprLayout.cpp Outdated Show resolved Hide resolved
@Aqa-Ib
Copy link
Contributor Author

Aqa-Ib commented Sep 30, 2024

I have made a new refactor to the code. No trace of recursion, it is a straight path now.
Please check it out.

src/layout/IHyprLayout.cpp Outdated Show resolved Hide resolved
src/layout/IHyprLayout.cpp Outdated Show resolved Hide resolved
src/layout/IHyprLayout.cpp Outdated Show resolved Hide resolved
@Aqa-Ib
Copy link
Contributor Author

Aqa-Ib commented Oct 1, 2024

Done. I have also simplified the code where possible. There is one conversation remaining though.

@Aqa-Ib Aqa-Ib requested a review from vaxerski October 1, 2024 17:04
…has 0 size when:

+ allfloat = true (new windows start as floating).
+ create a new window, tile it, and make it a group
+ create a new window autogrouped into the tiled group.
+ call togglefloat and the window disappears.
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.

outside of that cursed {} all good

@Aqa-Ib
Copy link
Contributor Author

Aqa-Ib commented Oct 2, 2024

Done. Thanks for your attention and patience. It was a pleasure working on this for Hyprland. Cheers !

@Aqa-Ib Aqa-Ib requested a review from vaxerski October 2, 2024 08:18
@vaxerski vaxerski merged commit e242694 into hyprwm:main Oct 2, 2024
12 checks passed
vaxerski pushed a commit to hyprwm/hyprland-wiki that referenced this pull request Oct 2, 2024
* auto_group

For hyprwm/Hyprland#7883

* grammar

* grammar

* grammar

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

Successfully merging this pull request may close these issues.

When a floating group is focused, add new windows to it
4 participants