Screen position based switching. #82

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
@ScatteredRay
Contributor

ScatteredRay commented Dec 15, 2012

Especially since Mac OSX likes to randomly rearrange screens, it was hard to predict which way this would swap, making it based on a constant direction improves behavior a bunch.

There are a few edge cases this won't handle, but would like to start a dialog on how to implement this properly.

@eczarny

This comment has been minimized.

Show comment
Hide comment
@eczarny

eczarny Jan 24, 2013

Owner

I'm not sure I understand what this change does. Could you explain the original issue in a bit more detail?

Owner

eczarny commented Jan 24, 2013

I'm not sure I understand what this change does. Could you explain the original issue in a bit more detail?

@ScatteredRay

This comment has been minimized.

Show comment
Hide comment
@ScatteredRay

ScatteredRay Feb 5, 2013

Contributor

@eczarny, when you only have two monitors, this doesn't matter since next screen and prev screen both cycle the same way. But if you have three or more, it's very difficult to predict which display the window will cycle to, because the screen order is pretty arbitrarily defined, and may change between reboots and the like. The intent of this change is to take screen position into account while moving a window between screens.

Contributor

ScatteredRay commented Feb 5, 2013

@eczarny, when you only have two monitors, this doesn't matter since next screen and prev screen both cycle the same way. But if you have three or more, it's very difficult to predict which display the window will cycle to, because the screen order is pretty arbitrarily defined, and may change between reboots and the like. The intent of this change is to take screen position into account while moving a window between screens.

@eczarny

This comment has been minimized.

Show comment
Hide comment
@eczarny

eczarny Feb 5, 2013

Owner

Got it. Could you rebase this commit? I'll try and fit it into the upcoming 0.8 release.

Owner

eczarny commented Feb 5, 2013

Got it. Could you rebase this commit? I'll try and fit it into the upcoming 0.8 release.

@ScatteredRay

This comment has been minimized.

Show comment
Hide comment
@ScatteredRay

ScatteredRay Feb 5, 2013

Contributor

So, I updated it to also take into account screen Y position, so that it works if you have stacked displays... And I rebased this. I'm not in the office so I haven't gotten a chance to fully test it however.

Contributor

ScatteredRay commented Feb 5, 2013

So, I updated it to also take into account screen Y position, so that it works if you have stacked displays... And I rebased this. I'm not in the office so I haven't gotten a chance to fully test it however.

@eczarny

This comment has been minimized.

Show comment
Hide comment
@eczarny

eczarny Feb 5, 2013

Owner

Once you test I'll merge. After the merge would you mind testing the change again from my master branch? I don't have the required setup to verify.

Owner

eczarny commented Feb 5, 2013

Once you test I'll merge. After the merge would you mind testing the change again from my master branch? I don't have the required setup to verify.

@ScatteredRay

This comment has been minimized.

Show comment
Hide comment
@ScatteredRay

ScatteredRay Feb 9, 2013

Contributor

Apologies, I've found a bug in the implementation and haven't had enough time to debug it yet.

Contributor

ScatteredRay commented Feb 9, 2013

Apologies, I've found a bug in the implementation and haven't had enough time to debug it yet.

@jasonm jasonm referenced this pull request Feb 13, 2013

Closed

Directional change-focus? #97

@eczarny

This comment has been minimized.

Show comment
Hide comment
@eczarny

eczarny Apr 25, 2013

Owner

I believe this would address issue #101.

Owner

eczarny commented Apr 25, 2013

I believe this would address issue #101.

@eczarny

This comment has been minimized.

Show comment
Hide comment
@eczarny

eczarny Sep 1, 2013

Owner

@Arelius, have you had a chance to debug the issue you discovered? I would love to get this change into an upcoming release.

Owner

eczarny commented Sep 1, 2013

@Arelius, have you had a chance to debug the issue you discovered? I would love to get this change into an upcoming release.

@ScatteredRay

This comment has been minimized.

Show comment
Hide comment
@ScatteredRay

ScatteredRay Sep 20, 2013

Contributor

@eczarny Sadly, I don't have a working Mac right now, so am not capable of fixing this.

Contributor

ScatteredRay commented Sep 20, 2013

@eczarny Sadly, I don't have a working Mac right now, so am not capable of fixing this.

@eczarny

This comment has been minimized.

Show comment
Hide comment
@eczarny

eczarny Sep 23, 2013

Owner

Unfortunately, without the required hardware I can't test this change. If somebody else is able to do so I would love to be able to merge it in.

Owner

eczarny commented Sep 23, 2013

Unfortunately, without the required hardware I can't test this change. If somebody else is able to do so I would love to be able to merge it in.

@eczarny

This comment has been minimized.

Show comment
Hide comment
@eczarny

eczarny Jan 23, 2014

Owner

I would love to test this change out somehow. Is anybody able to try it out? If not I'm going to have to close this out.

Owner

eczarny commented Jan 23, 2014

I would love to test this change out somehow. Is anybody able to try it out? If not I'm going to have to close this out.

@eczarny eczarny closed this Jun 12, 2014

@liam-m

This comment has been minimized.

Show comment
Hide comment
@liam-m

liam-m Jun 18, 2014

@eczarny I'd be interested in testing this, as I'm experiencing the issue. Could you give me some instructions on how to build?

liam-m commented Jun 18, 2014

@eczarny I'd be interested in testing this, as I'm experiencing the issue. Could you give me some instructions on how to build?

ScatteredRay added some commits Dec 15, 2012

Screen moving works spatially.
* Next and previous screen now take spatial relationships into account.
@ScatteredRay

This comment has been minimized.

Show comment
Hide comment
@ScatteredRay

ScatteredRay Jun 18, 2014

Contributor

@liam-m I just did a rebase on current master, I don't have a machine I can test the build on, but feel free to sync up to my updated branch at https://github.com/Arelius/spectacle and try a build. IIRC, you can just open the xcodeproj in xcode and hit build.

Contributor

ScatteredRay commented Jun 18, 2014

@liam-m I just did a rebase on current master, I don't have a machine I can test the build on, but feel free to sync up to my updated branch at https://github.com/Arelius/spectacle and try a build. IIRC, you can just open the xcodeproj in xcode and hit build.

@eczarny

This comment has been minimized.

Show comment
Hide comment
@eczarny

eczarny Jun 18, 2014

Owner

Since adding Cocoapods you should open the xcworkspace instead of the xcodeproj.

Owner

eczarny commented Jun 18, 2014

Since adding Cocoapods you should open the xcworkspace instead of the xcodeproj.

@liam-m

This comment has been minimized.

Show comment
Hide comment
@liam-m

liam-m Jun 19, 2014

I built @Arelius's branch and tested with 3 displays - it's a big improvement and has worked as expected in my testing.

It would be good if the displays could wrap around, so pressing 'Previous Display' on the leftmost display would make the window go to the rightmost, but maybe this is worth a separate issue/discussion.

liam-m commented Jun 19, 2014

I built @Arelius's branch and tested with 3 displays - it's a big improvement and has worked as expected in my testing.

It would be good if the displays could wrap around, so pressing 'Previous Display' on the leftmost display would make the window go to the rightmost, but maybe this is worth a separate issue/discussion.

@eczarny eczarny reopened this Jun 20, 2014

@eczarny

This comment has been minimized.

Show comment
Hide comment
@eczarny

eczarny Jun 20, 2014

Owner

I merged manually on master.

Owner

eczarny commented Jun 20, 2014

I merged manually on master.

@eczarny eczarny closed this Jun 20, 2014

@eczarny

This comment has been minimized.

Show comment
Hide comment
@eczarny

eczarny Jun 20, 2014

Owner

@Arelius I'm trying to clean up the logic a bit so that's easier to digest. Could you explain each check in the conditional?

NSInteger lastDeltaX = 0;
NSInteger lastDeltaY = 0;

NSInteger screenDeltaX = (frameOfScreen.origin.x - currentFrameOfScreen.origin.x) ;
NSInteger screenDeltaY = (frameOfScreen.origin.y - currentFrameOfScreen.origin.y) ;

NSInteger dir = (action == SpectacleWindowActionNextDisplay) ? -1 : 1;

if ((screenDeltaX * dir > 0 ||
     (screenDeltaX == 0 &&
      screenDeltaY * dir > 0)) &&
    (result == nil ||
     (screenDeltaX * dir) < lastDeltaX ||
     (screenDeltaX == 0 &&
      (screenDeltaY * dir) < lastDeltaY))) {
    result = [screens objectAtIndex: i];
    lastDeltaX = screenDeltaX * dir;
    lastDeltaY = screenDeltaY * dir;
}
Owner

eczarny commented Jun 20, 2014

@Arelius I'm trying to clean up the logic a bit so that's easier to digest. Could you explain each check in the conditional?

NSInteger lastDeltaX = 0;
NSInteger lastDeltaY = 0;

NSInteger screenDeltaX = (frameOfScreen.origin.x - currentFrameOfScreen.origin.x) ;
NSInteger screenDeltaY = (frameOfScreen.origin.y - currentFrameOfScreen.origin.y) ;

NSInteger dir = (action == SpectacleWindowActionNextDisplay) ? -1 : 1;

if ((screenDeltaX * dir > 0 ||
     (screenDeltaX == 0 &&
      screenDeltaY * dir > 0)) &&
    (result == nil ||
     (screenDeltaX * dir) < lastDeltaX ||
     (screenDeltaX == 0 &&
      (screenDeltaY * dir) < lastDeltaY))) {
    result = [screens objectAtIndex: i];
    lastDeltaX = screenDeltaX * dir;
    lastDeltaY = screenDeltaY * dir;
}

@eczarny eczarny reopened this Jun 20, 2014

@eczarny

This comment has been minimized.

Show comment
Hide comment
@eczarny

eczarny Jun 20, 2014

Owner

The logic is a bit confusing. I'm wondering if using a stable sort to order the screens by descending X, then descending Y, coordinates would be any easier. This way at least screen selection is consistent?

Owner

eczarny commented Jun 20, 2014

The logic is a bit confusing. I'm wondering if using a stable sort to order the screens by descending X, then descending Y, coordinates would be any easier. This way at least screen selection is consistent?

@eczarny

This comment has been minimized.

Show comment
Hide comment
@eczarny

eczarny Jun 20, 2014

Owner

I'm actually going to revert this change until I can fully understand it. The regression in screen wrapping is an unpleasant side effect to the fix.

Sorry about the aggressive merge!

Owner

eczarny commented Jun 20, 2014

I'm actually going to revert this change until I can fully understand it. The regression in screen wrapping is an unpleasant side effect to the fix.

Sorry about the aggressive merge!

@ScatteredRay

This comment has been minimized.

Show comment
Hide comment
@ScatteredRay

ScatteredRay Jun 20, 2014

Contributor
  if ((screenDeltaX * dir > 0 || // canidate x pos is in switching direction or
     (screenDeltaX == 0 && // canidate has the same x pos and
      screenDeltaY * dir > 0)) && canidate y pos is in switching direction
    (result == nil || // is first possible screen
     (screenDeltaX * dir) < lastDeltaX || smallest delta in x in switching direction or
     (screenDeltaX == 0 && // has no delta in x in switching direction and 
      (screenDeltaY * dir) < lastDeltaY))) { // smallest delta in y in switching direction

With many monitors, this will first try to cycle between monitors that have matching X coords in case they are stacked, and then failing that, cycle to the side. dir specifies if you want to search in positive or negative delta directions, basically switching directions in both axis.

This could be done with a stable sort for sure. just sort by Y position, and then X position.

If you wanted to wrap, you could after the loop, if no screens are found, just find the furthest screen in that direction

Contributor

ScatteredRay commented Jun 20, 2014

  if ((screenDeltaX * dir > 0 || // canidate x pos is in switching direction or
     (screenDeltaX == 0 && // canidate has the same x pos and
      screenDeltaY * dir > 0)) && canidate y pos is in switching direction
    (result == nil || // is first possible screen
     (screenDeltaX * dir) < lastDeltaX || smallest delta in x in switching direction or
     (screenDeltaX == 0 && // has no delta in x in switching direction and 
      (screenDeltaY * dir) < lastDeltaY))) { // smallest delta in y in switching direction

With many monitors, this will first try to cycle between monitors that have matching X coords in case they are stacked, and then failing that, cycle to the side. dir specifies if you want to search in positive or negative delta directions, basically switching directions in both axis.

This could be done with a stable sort for sure. just sort by Y position, and then X position.

If you wanted to wrap, you could after the loop, if no screens are found, just find the furthest screen in that direction

@ScatteredRay

This comment has been minimized.

Show comment
Hide comment
@ScatteredRay

ScatteredRay Jun 20, 2014

Contributor

The logic is a bit confusing. I'm wondering if using a stable sort to order the screens by descending X, then descending Y, coordinates would be any easier. This way at least screen selection is consistent?

That's the same order that this selection works in, so if you wanted to sort first, then just cycle through the list, it should have the same effect.

Contributor

ScatteredRay commented Jun 20, 2014

The logic is a bit confusing. I'm wondering if using a stable sort to order the screens by descending X, then descending Y, coordinates would be any easier. This way at least screen selection is consistent?

That's the same order that this selection works in, so if you wanted to sort first, then just cycle through the list, it should have the same effect.

@eczarny

This comment has been minimized.

Show comment
Hide comment
@eczarny

eczarny Jun 20, 2014

Owner

Ah, okay. I may play around with that. I'm not terribly concerned about the particular ordering as long as it is consistent. Thanks for all of the work you've done on this!

Owner

eczarny commented Jun 20, 2014

Ah, okay. I may play around with that. I'm not terribly concerned about the particular ordering as long as it is consistent. Thanks for all of the work you've done on this!

@ScatteredRay

This comment has been minimized.

Show comment
Hide comment
@ScatteredRay

ScatteredRay Jun 20, 2014

Contributor

Of course, sorry I can't do more work getting it nicer. Not working on a Mac anymore makes that problematic.

Contributor

ScatteredRay commented Jun 20, 2014

Of course, sorry I can't do more work getting it nicer. Not working on a Mac anymore makes that problematic.

@eczarny

This comment has been minimized.

Show comment
Hide comment
@eczarny

eczarny Jun 20, 2014

Owner

No problem! I can probably take it from here. I'll probably close out this pull request and go down the stable sort approach. I just wish I had more than two external displays to test with.

Owner

eczarny commented Jun 20, 2014

No problem! I can probably take it from here. I'll probably close out this pull request and go down the stable sort approach. I just wish I had more than two external displays to test with.

@eczarny eczarny closed this Jun 20, 2014

@liam-m

This comment has been minimized.

Show comment
Hide comment
@liam-m

liam-m Jun 21, 2014

Let me know if you want me to test anything, I have 3 displays (2 external) too

liam-m commented Jun 21, 2014

Let me know if you want me to test anything, I have 3 displays (2 external) too

@eczarny

This comment has been minimized.

Show comment
Hide comment
@eczarny

eczarny Jun 21, 2014

Owner

Thanks @liam-m! I should have something to test in a few days.

Owner

eczarny commented Jun 21, 2014

Thanks @liam-m! I should have something to test in a few days.

@eczarny

This comment has been minimized.

Show comment
Hide comment
@eczarny

eczarny Jun 22, 2014

Owner

@liam-m I just pushed a change to master that I hope will help. Could you try testing it out? I was only able to verify the changes based on some simple tests of the algorithm. I won't be able to actually try it out until Monday, but would love to get another set of eyes on it.

Thanks!

Owner

eczarny commented Jun 22, 2014

@liam-m I just pushed a change to master that I hope will help. Could you try testing it out? I was only able to verify the changes based on some simple tests of the algorithm. I won't be able to actually try it out until Monday, but would love to get another set of eyes on it.

Thanks!

@liam-m

This comment has been minimized.

Show comment
Hide comment
@liam-m

liam-m Jun 29, 2014

@eczarny I've tested it - it seems to work as described, it's now consistent. However, is it possible to make it go from left-to-right and wrap around? In other words:

  • Making 'Next Display' send the window to the display to the right of the display (from System Preferences > Displays > Arrangement) that the window is currently on; opposite for 'Previous Display'
  • Making 'Next Display' on the rightmost display send the window to the leftmost; opposite for 'Previous Display' from leftmost display

liam-m commented Jun 29, 2014

@eczarny I've tested it - it seems to work as described, it's now consistent. However, is it possible to make it go from left-to-right and wrap around? In other words:

  • Making 'Next Display' send the window to the display to the right of the display (from System Preferences > Displays > Arrangement) that the window is currently on; opposite for 'Previous Display'
  • Making 'Next Display' on the rightmost display send the window to the leftmost; opposite for 'Previous Display' from leftmost display
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment