search

rxhanson / Rectangle

Move and resize windows on macOS with keyboard shortcuts and snap areas
https://rectangleapp.com
Other
25.1k stars 742 forks source link

Add Center Two Screens action #1314

Closed jordanbertasso closed 5 months ago

jordanbertasso commented 6 months ago

Adds an action that centers a window across two picture by picture (PBP) displays

Implements the feature from this discussion

rxhanson commented 6 months ago

Thanks for contributing. Sorry for the delay on this one, I’m planning on going through it this weekend.

rxhanson commented 6 months ago

Ah, I ended up catching a cold this past week. I will get to this soon though!

rxhanson commented 6 months ago

Alright, I finally got a chance to take a look, and there are a couple of items to work through.

  1. In my testing with a small display on the left and a main display on the right, a window was placed on the right display, sort of centered in the right half of the display:

    center two screen incorrect placement

  2. This isn't something that I will add into the UI, unless there is significant demand. There are other actions that have also been added to Rectangle that aren't in the UI, like the extra centering commands, ninths, eighths, etc, for reference. In order for this change to go in, you'll have to revert out your change to the Main.storyboard and the PrefsViewController. I like the icon you made, though!

jordanbertasso commented 6 months ago

Thanks so much for having a look! I did not know about those hidden actions. It makes sense for this to go there.

I've reimplemented the calculation, but please could you try your scenario again? I've listed the assumptions and steps in a docstring as well to make what I am trying to do a bit clearer.

For some reason this does not work with vertically stacked displays. I can see the window flick into the right position and then immediately get placed somewhere else. I think this might be a quirk of MacOS with vertically stacked displays? I'm not sure. If you have no idea about this either, maybe I can just remove support for vertically adjacent screens?

I think I've done all I need for the command to be configurable via the terminal? Have I missed anything?

codedog commented 5 months ago

I would definitely like to have a center two-thirds option!

rxhanson commented 5 months ago

I tested it out and it works fine now for me. I don't think I'll take the time to figure out why vertically stacked displays don't work here, but I'm fine merging it without this working (I view the terminal command configured options as ones that are ok with some quirks). Hope you don't mind, I adjusted some items here - removed it from the menu bar menu, adjusted some code styling/formatting to fit with the rest of the app. Noteworthy items: swift switches don't fall through, so I removed the break statements, used ternaries where possible, and removed redundant enum naming.

There are too many comments in its current state to merge. I appreciate making your thoughts known for the sake of the PR, but definitely think it should be improved by only leaving in things that really need some 'splainin. When that's done, I'm ready to merge, unless you want to spend more time figuring out the vertical stack problem.

rxhanson commented 5 months ago

Since it's been a while, and I felt like maybe I overstepped initially (sorry!), I rolled my changes out of your PR. When I last went through it, I started down a path of getting vertically stacked displays working and realized I was going to rewrite your effort. Let me know what you'd like to do here - if you want to pass off what you have, I can jump back into it when I have time, or you can keep at it and let me know when it's ready for another look. If I don't hear back, I'll probably close this PR for the time being.