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

X.L.LayoutHints stop windows from overlapping #193

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

Conversation

ankaan
Copy link
Contributor

@ankaan ankaan commented Jun 10, 2017

Description

This is an improvement on the pull request "Fix render order of
LayoutHints and MultiColumns" (#186) and addresses the actual underlying
problem.

It turned out that windows can sometimes overlap also. This happens when
a window is exactly in the center along an axis. There is a special
case in the code for this, but it was not handled properly.

I removed the code that places the focused window on top since it is no
longer required, but I still preserve the window order of the underlying
layout. This interferes even less with the underlying layout.

I also removed some code paths that were no longer necessary due to this
change and generalized some types so that I could debug the code more
easily.

Checklist

@mention-bot
Copy link

@ankaan, thanks for your PR! By analyzing the history of the files in this pull request, we identified @pjones, @aavogt and @liskin to be potential reviewers.

This is an improvement on the pull request "Fix render order of
LayoutHints and MultiColumns" (xmonad#186) and addresses the actual underlying
problem.

It turned out that windows can sometimes overlap also. This happens when
a window is exactly in the center along an axis. There is a special
case in the code for this, but it was not handled properly.

I removed the code that places the focused window on top since it is no
longer required, but I still preserve the window order of the underlying
layout. This interferes even less with the underlying layout.

I also removed some code paths that were no longer necessary due to this
change and generalized some types so that I could debug the code more
easily.
@ankaan ankaan force-pushed the layoutHintsToCenter-fix-overlap branch from 15f9e73 to 45ecd35 Compare June 11, 2017 09:46
@ankaan
Copy link
Contributor Author

ankaan commented Jun 11, 2017

I realized that the special case that I initially removed actually made things more visually pleasing, so I added it back but made it work properly.

I also removed a comment that was wrong (and contradicted what is written right before it.)

Copy link
Member

@slotThe slotThe left a comment

Choose a reason for hiding this comment

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

LGTM, but I don't really use layout hints, so I can't adequately review it in detail.

@liskin you were flagged by that bot in the first run; do you still use this functionality and can review the change?

@ankaan can you perhaps rebase on top of current master?

@liskin
Copy link
Member

liskin commented Jan 29, 2021

I do use LayoutHints but I don't use LayoutHintsToCenter so I'm probably unaffected, and it's been almost 10 years since I last touched this module, so I'm as qualified to review it as anyone else. Might be nice to add unit test for the problem being fixed, or something like that which will help potential reviewers to understand it.

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