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

Allow moving the windows between displays depending on their index instead of position #23

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

barnas-michal
Copy link

This will allow to move windows when the displays are not set in "siblings" positions, where no coordinates (x and y) are equal and will allow to have windows-like control.
Also the default direction is changed to "next", so the new users are able to just execute program without getting the "no siblings" error.

index instead of position.

This will allow to move windows when the displays are not set in "siblings" positions,
where no coordinates (x and y) are equal and will allow to have windows-like control.
This change allows to use the program without parameters when there's no siblings
in the default configuration.
Copy link
Owner

@AlexisBRENON AlexisBRENON left a comment

Choose a reason for hiding this comment

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

Hey, thanks a lot for your contribution.

However, while reviewing it, I see too much case where NEXT and PREV are ambiguous, where you don’t exactly know how to handle them, etc.
One thing makes me realize that maybe it’s not the right abstraction. NEXT and PREV are not ordinal points.

Instead of using such kind of fuzzy words, may I propose you to define more ordinal points?
Today, we only handles N-S-E-W matching vertically or horizontally aligned screens. Maybe we can add more points:

  • SE: will match screens with x and y greater than current screen (not the vertically or horizontally aligned ones)
  • ESE: will match horizontally aligned screens to the east (E) and then the non-aligned ones (SE)
  • SSE: will match vertically aligned (S) and then the non-aligned ones (SE)
  • etc.

With such kind of generalization, I would be open to update the default --direction argument to a more permissive Ordinal.ESE value.

What do you thing of such idea. One remaining question is how to order the screens in these cases. For example, for ESE should we put all screens horizontally aligned (from the nearest to the furthest) and then the non-aligned ones ; or should we mix them all and just sort by the distance to the current screen ? Maybe adding still more points, such as "southeast by east" (between "east southeast" and "southeast") and "southeast by south" (between "southeast" and "south southeast") can be a way to provide different ordering methods.

Comment on lines 13 to 26
def get(cls, v: str):
if v.upper()[0] == 'E':
return Ordinal.EAST
if v.upper()[0] == 'W':
return Ordinal.WEST
if v.upper()[0] == 'N':
if v.upper()[0:2] == 'NO':
return Ordinal.NORTH
if v.upper()[0] == 'S':
return Ordinal.SOUTH
if v.upper()[0:2] == 'NE':
return Ordinal.NEXT
if v.upper()[0] == 'P':
return Ordinal.PREV
raise TypeError("No direction match with '{}'".format(v))
Copy link
Owner

Choose a reason for hiding this comment

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

I was thinking about rewriting this method with full name comparison (v.upper() == "EAST"), but why not use the __getitem__ method directly, instead of this custom get method ?

@@ -56,7 +56,7 @@ def main():
action="store",
type=Ordinal.get,
choices=list(Ordinal),
default=Ordinal.EAST.name.capitalize(),
default=Ordinal.NEXT.name.capitalize(),
Copy link
Owner

Choose a reason for hiding this comment

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

I don’t feel comfortable to change such default value. I would require anybody using the script without argument to update their configuration (in case the new value does not exhibit the same behavior).

Copy link
Author

Choose a reason for hiding this comment

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

You're right, I was thinking about this change, my main argument on this change is that after downloading your script, it's required to read help and decide which option should be used. But for current users, this change can be annoying, right.

@barnas-michal
Copy link
Author

Instead of using such kind of fuzzy words, may I propose you to define more ordinal points?

I know that these values are not directions, but that was the real reason for my change. I wanted this script to be able to iterate over all displays, negligible of their positions and relations between each other. On (MS) Windows, you have the shortcuts (meta+shift + L/R) that iterates your window over monitors and as far as I know, it iterates depending on the identifier of the current monitor. So it may be needed to press this shortcut twice to put the window on the above monitor, but it's (IMO) easier than setting more shortcuts and thinking about the relative positions of them, especially if you have two upper monitors and one below between them).

@AlexisBRENON
Copy link
Owner

Instead of using such kind of fuzzy words, may I propose you to define more ordinal points?

I know that these values are not directions, but that was the real reason for my change. I wanted this script to be able to iterate over all displays, negligible of their positions and relations between each other. On (MS) Windows, you have the shortcuts (meta+shift + L/R) that iterates your window over monitors and as far as I know, it iterates depending on the identifier of the current monitor. So it may be needed to press this shortcut twice to put the window on the above monitor, but it's (IMO) easier than setting more shortcuts and thinking about the relative positions of them, especially if you have two upper monitors and one below between them).

Thanks for explaining your use case. Is chaining the call a viable solution for you ? I recently merged a contribution which allows to chain call the script: move-to-monitor -d EAST || move-to-monitor -d SOUTH
If you have a setup with 2 upper monitors and one below, you can bind your keyboard shortcut to something like:
move-to-monitor -d EAST -W || move-to-monitor -d SOUTH -W || ( move-to-monitor -d EAST ; move-to-monitor -d SOUTH) (or something similar. Having more cardinal points would allow to make it easier).

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