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 Center Two Screens action #1314

Conversation

jordanbertasso
Copy link

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

Implements the feature from this discussion

@rxhanson
Copy link
Owner

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

@rxhanson
Copy link
Owner

rxhanson commented Jan 3, 2024

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

@rxhanson
Copy link
Owner

rxhanson commented Jan 5, 2024

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
  1. 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
Copy link
Author

jordanbertasso commented Jan 6, 2024

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
Copy link

codedog commented Jan 10, 2024

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

@rxhanson
Copy link
Owner

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 rxhanson force-pushed the add-center-two-screens-action branch from 614bd17 to bd5aeae Compare February 1, 2024 03:53
@rxhanson
Copy link
Owner

rxhanson commented Feb 1, 2024

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.

@rxhanson rxhanson closed this Feb 5, 2024
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