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 smart window gaps feature #748

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JCapucho
Copy link
Contributor

This PR implements the smart gaps feature like what is present in i3 and sway https://i3wm.org/docs/userguide.html#gaps

Smart window gaps only add the external and internal gaps when it aids in visually separating the top levels (when using the tiling mode).

This is done by only adding the outer gap when more than one toplevel exists in the output and only adding the inner gap when an adjacent toplevel exists in the given direction.

I don't know if this is a wanted feature but it's absence was really bugging me (I previously used sway), so I implemented it and have been using it so I thought I might as well contribute it upstream :)

Screenshots

One window (no gaps)
One window

Two windows (outer and inner gaps)
Two windows

Notes

Requires a change in libcosmic, if the feature is wanted I will open the PR (Didn't want to spam two repositories for this feature), the changes are here https://github.com/JCapucho/libcosmic/tree/smart-window-gaps

I also think the inner gap calculations are wrong, currently they are:

if TilingLayout::has_adjacent_node(tree, &node_id, dir) {
    inner / 2
} else if is_smart_gaps {
    0
} else {
    inner
}

I have kept the previous version where if smart gaps isn't enabled and there are no adjacent windows a inner gap is added, but this seems wrong because it leads to windows having both an outer and inner gap when they are at the edge of the output. This should instead be:

if TilingLayout::has_adjacent_node(tree, &node_id, dir) {
    inner / 2
} else {
    0
}

I think this hasn't been a problem because by default the outer gap is 0 and there's no way to change it in the settings app, but if it's changed in the file directly it leads to a double border.

Further work

I have also a similar change to only add the active window hint when there's more than one window in the output, but that one still has some problems.

@Drakulix
Copy link
Member

Drakulix commented Aug 20, 2024

First of all, thanks for the contribution!

Second, this is a feature purely for cosmic-comp, I don't think this should be stored in the libcosmic-theme. Rather this should be inside cosmic-comp's config, just like the active-border width or gap_size. (Which you can take as inspiration on how to pass this down to the tiling layout code.)

Lastly, I don't know if we want to have that feature. Pinging @pop-os/ux on this one, specifically if we would want to include this inside cosmic-settings. I don't think it would hurt to merge this has a config-file only feature, but that might make it more prone to breakage as I don't know, if we would be testing this frequently. (Then again the tiling code is already complicated enough and rarely touched these days.)

@JCapucho
Copy link
Contributor Author

Second, this is a feature purely for cosmic-comp, I don't think this should be stored in the libcosmic-theme. Rather this should be inside cosmic-comp's config, just like the active-border width or gap_size. (Which you can take as inspiration on how to pass this down to the tiling layout code.)

That makes sense I looked at the gaps and those were on the theme but I should have looked if there were comp specific settings

@maria-komarova
Copy link

Thanks for the contribution.
I am not sure at this point if we want this feature. It seems a little too niche and not necessarily easy to understand unless one is coming from i3 and sway. But if one is new to tiling, then it might be confusing. We show hints for window groups when one is moving a window around because it seems that this is the most useful point when understanding window groups matters. But I'm not sure it is as valuable when one is just working.

@JCapucho
Copy link
Contributor Author

Thanks for the contribution. I am not sure at this point if we want this feature. It seems a little too niche and not necessarily easy to understand unless one is coming from i3 and sway. But if one is new to tiling, then it might be confusing. We show hints for window groups when one is moving a window around because it seems that this is the most useful point when understanding window groups matters. But I'm not sure it is as valuable when one is just working.

The hints are still there and unchanged I only disabled them by setting the width to 0 in my settings. The difference is in the gaps.

@Drakulix
Copy link
Member

I am not sure at this point if we want this feature. It seems a little too niche and not necessarily easy to understand unless one is coming from i3 and sway.

That is certainly the target group for this setting.

But if one is new to tiling, then it might be confusing. We show hints for window groups when one is moving a window around because it seems that this is the most useful point when understanding window groups matters. But I'm not sure it is as valuable when one is just working.

Adding to the previous comment, this only affects window positions at rest, it should do nothing to the overview when actively moving windows around or swapping them.

As such I think this is a useful feature for people wanting more screen estate and that feel at home with the tiling concepts already, though arguably a bit niche.

@maria-komarova
Copy link

maria-komarova commented Aug 22, 2024

Adding to the previous comment, this only affects window positions at rest, it should do nothing to the overview when actively moving windows around or swapping them.

I got it. Just trying to better understand why it is so helpful to see the groups easier when windows are at rest. I can see how it is useful in i3 or sway when you need to move windows and understanding the groups helps one figure out what will happen. But I'm still not sure how much value there would be in our case. Muscle memory that people bring with them is understandable but how exactly is it helpful to see the groups of windows at rest in COSMIC is not so clear to me.

@Drakulix
Copy link
Member

Muscle memory that people bring with them is understandable but how exactly is it helpful to see the groups of windows at rest in COSMIC is not so clear to me.

Got it. I tend to agree given that our tiling operations aren't based on rotating parts of the tree, which is what makes this information so crucial in i3/sway.

@JCapucho
Copy link
Contributor Author

I got it. Just trying to better understand why it is so helpful to see the groups easier when windows are at rest.

For me it isn't about seeing groups easier it's more because of personal preference, I like to use gaps however I find them quite distracting when I only have one window on an output and gaps around it, almost like a second bezel on the screen. However if I remove gaps completely and have more than one window the windows seem to blur together at the edges.

Smart gaps for me it's the ideal it only adds gaps when there's more than one window so outputs with a single window are almost fullscreen (minus any topbars) and outputs with multiple windows have clear separation between windows.

@JCapucho JCapucho force-pushed the smart-window-gaps branch 2 times, most recently from 26ff8bb to cd4d372 Compare August 29, 2024 15:00
@JCapucho
Copy link
Contributor Author

I rebased everything and moved the configuration to the cosmic-comp's config

@JCapucho
Copy link
Contributor Author

JCapucho commented Sep 6, 2024

@Drakulix sorry for the ping, but I've rebased again and wanted to know if there has been discussion around accepting this feature upstream.

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.

3 participants