-
Notifications
You must be signed in to change notification settings - Fork 94
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
Resize window, transport mouse, and more. #16
Conversation
@Stengo, please look at this pull request when you have time. Feel free to reach out if needed. Thanks in advance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for your contribution! It's amazing to see people step in to improve the app 🙂
Your implementation appears to work beautifully, so almost all of my comments are about architectural concerns.
I'm aware that Redux is still quite unusual in the Swift community, so contributing to DeskPad comes with a bit of a learning curve, but I truly believe it's the most sensible general-purpose architecture for application programming and I think learning it (or its derivates like The Composable Architecture) is well worth the while and DeskPad is still small enough to easily be able to grasp the whole flow 🙂
DeskPad/AppDelegate.swift
Outdated
@objc func toggleCheckboxMoveCursorItem(_ sender: NSMenuItem) { | ||
// Toggle checkbox | ||
sender.state = (sender.state == .on) ? .off : .on | ||
UserDefaults.standard.set(sender.state == .on, forKey: "CheckboxState") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a Redux-based architecture, the state of this feature should be kept as part of the unified AppState and the adjustments to the user defaults should happen in a middleware, or in this particular architecture in a 'side effect'.
The view controller should be informed about the change via the ScreenViewData
to maintain the unidirectional dataflow (same for the menu item).
It's a little more overhead in the short-term, but keeps the overall codebase much more structured and easy to browse and modify in the long run 🙂
There should be examples for most of this in the DeskPad codebase itself, but for something very similar to this particular use-case you could also check out the settings on one of my other projects, Exposition. I can also recommend the official Redux documentation, since the core of DeskPad's codebase, ReSwift, is a fairly faithful reproduction of it in Swift 😄
Let me know if you have any follow-up questions about any of that!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HI @Stengo! Thank you so much for your inputs. It took me a while to understand the Redux architecture, but I finally got it and implemented the requested changes. Please take a look and let me know your thoughts.
} | ||
} | ||
return $0 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to my first comment, this logic should likely live in a side effect to keep any non-view related logic out of the view layer 🙂 Again, there's a very similar situation in Exposition that might be useful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
view.window?.setContentSize(viewData.resolution) | ||
view.window?.contentAspectRatio = NSSize(width: viewData.resolution.width, height: viewData.resolution.height) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the general idea of having a flexible window size, but unfortunately anything but a native 1:1 pixel mapping leads to an incredibly fuzzy picture.
If we would still like to support this feature, I think we would need a snapping behavior around the 1:1 pixel target, so that it is easy to properly size the window.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree - the fuzziness is driving me crazy!!! An option to snap window size to resolution is required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm working on the requested changes. This is my first interaction with Swift and Redux, which is not very straightforward, but I expect to have it ready soon.
Regarding the fuzziness, it might depend on the monitor, but I have this issue only when the high resolution is selected and it works amazingly with low resolution (in display settings), even if the window size is not mapped 1:1 to the native resolution. Could you please check if it's fuzzy with both high and low resolutions?
I already checked in 4 different monitors + the mac display, so it might be enough to disable the high-resolution options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My proposed solution is as follows:
- Added a menu button to reset the window size to have a 1:1 size with the configured resolution.
- Added a menu button to enable/disable the ability to resize the window so that you can stick with the 1:1 size with the configured resolution without having to deal with unwanted resizes.
Please take a look at the updated code. It should be good enough to cover all use cases.
view.window?.delegate = self | ||
view.window?.contentMinSize = CGSize(width: 400, height: 300) | ||
view.window?.contentMaxSize = CGSize(width: 3840, height: 2160) | ||
view.window?.styleMask = [.titled, .closable, .resizable, .miniaturizable] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe all of this should be moved into the AppDelegate
, where the window is created. I'm also surprised to see the view controller being set as the window's delegate. How is that being used? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
DeskPad.xcodeproj/project.pbxproj
Outdated
@@ -384,6 +384,7 @@ | |||
ASSETCATALOG_COMPILER_APPICON_NAME = AppIcon; | |||
ASSETCATALOG_COMPILER_GLOBAL_ACCENT_COLOR_NAME = AccentColor; | |||
CODE_SIGN_ENTITLEMENTS = DeskPad/DeskPad.entitlements; | |||
"CODE_SIGN_IDENTITY[sdk=macosx*]" = "-"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind reverting this change? 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Amazing! Love your work:) |
38682a5
to
60d91f7
Compare
This will preserve the aspect ratio and snap to a 1:1 pixel mapping if the desired window width is within 20 pixels of the original resolution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once again thank you for the work you put in! 👏 I've added a few comments to point out some of the pitfalls when working with a Redux architecture, but ultimately I think a lot of this is not actually necessary for these features.
I've just sat down and reworked the implementation from first principles and ended up with a few assumptions that I think make both the code as well as the user experience easier:
- teleportation is a generally useful feature and I can't think of anyone who would want to disable it, so I don't think we need a menu item for it. We can however simplify the implementation.
- similarly, screen resizing should be useful to anyone, but I think snapping is a far more common and intuitive experience than a menu button
I pushed what I ended up with to the earl-rework
branch (main...earl-rework), which I based off of main.
Of course I'd still like for you to get the credit since you did the bulk of the work, so if you'd like you could apply those changes to your branch 🙂 If you've got any feedback or concerns about my reimplementation definitely let me know and we can adjust it!
DeskPad/AppDelegate.swift
Outdated
|
||
if UserDefaults.standard.object(forKey: "isTeleportEnabled") == nil { | ||
UserDefaults.standard.set(true, forKey: "isTeleportEnabled") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any access (read and write) to the user defaults should live in a side effect to ensure that there's a unified, testable interface that keeps any outside interference out of the core redux flow.
DeskPad/AppDelegate.swift
Outdated
NSEvent.addLocalMonitorForEvents(matching: [.leftMouseUp]) { | ||
store.dispatch(MouseLocationEvent.localMouseClicked(mouseLocation: NSEvent.mouseLocation, event: $0)) | ||
return $0 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also live in a side effect or the view layer itself, in this case the view can get the same information via a gesture recognizer 🙂
DeskPad/AppDelegate.swift
Outdated
store.dispatch(MouseLocationEvent.globalMouseMoved(mouseLocation: NSEvent.mouseLocation, event: $0)) | ||
} | ||
|
||
store.dispatch(MouseLocationAction.setWindow(window: window)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Objects are never allowed to be stored in a redux state, as the entire architecture is based around pure functions without any side effect. Any access to this would have to work through a state observer or side effect!
DeskPad/AppDelegate.swift
Outdated
} | ||
|
||
NSEvent.addLocalMonitorForEvents(matching: [.mouseMoved]) { | ||
store.dispatch(MouseLocationEvent.localMouseMoved(mouseLocation: NSEvent.mouseLocation, event: $0)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would lead to an incredibly noisy redux flow, as these are very high frequency changes that we don't actually need. Ideally we keep things like this local to a side effect unless multiple components within the app need access to it!
DeskPad/AppDelegate.swift
Outdated
store.dispatch(AppDelegateAction.didFinishLaunching) | ||
} | ||
|
||
func applicationShouldTerminateAfterLastWindowClosed(_: NSApplication) -> Bool { | ||
return true | ||
} | ||
|
||
func applicationSupportsSecureRestorableState(_: NSApplication) -> Bool { | ||
return true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also not quite permissible in a Redux architecture, since there is a unified state that is responsible for keeping track of things!
// 16:9 minus menu bar and title bar | ||
CGVirtualDisplayMode(width: 3840, height: 2095, refreshRate: 60), | ||
CGVirtualDisplayMode(width: 2560, height: 1375, refreshRate: 60), | ||
CGVirtualDisplayMode(width: 1920, height: 1015, refreshRate: 60), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you point out what the intent of these resolutions is? I'd like to avoid adding highly specific ones unless there is a broadly applicable use case 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The primary goal behind window resizing is to maximize the actual DeskPad window area when sharing the screen while maintaining a clear and sharp visual representation. I recognize the importance of preserving a 1:1 window-to-pixel mapping to avoid any visual distortion.
A straightforward (naive) approach to achieve this balance involves setting 16:9 resolutions but excluding the height of the DeskPad title bar and macOS menu bar.
Upon contemplation, I explored two alternative approaches that, due to the intricacies of Apple's AppKit and Swift, weren't immediately straightforward to implement, given my limited experience:
- Menu Option for Custom Resolutions: Implementing a menu item to enable users to create and remove custom resolutions.
- Menu Option to Set Resolution Based on the Current Window Size: Providing a menu option to create a new custom resolution aligned with the current window size.
Considering my typical usage scenario, which I believe is quite common, users typically work with up to two monitors with varying resolutions (one for work and one for home displays). They might need, at most, a couple of custom resolutions to optimize the display when screen sharing.
The intention behind these suggested approaches is to offer a user-friendly way to manage resolutions. I don't expect the user to add several custom resolutions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it's ok, I can keep my custom resolutions in my fork :)
I genuinely appreciate the effort you've put into reworking my PR! It was incredibly enlightening to see how the same result could be achieved with a much simpler approach, leading to a more robust architecture. This experience has been a valuable learning curve for me, nurturing a strong interest in dedicating more time to Swift and understanding the Redux architecture. As someone from a Firmware background, this is an entirely new world for me. One final note: the concept of enabling or disabling window resizing, combined with custom resolutions, provides a practical means to lock in the desired window size. I assume users hardly ever change it once it's initially set up (it could actually be annoying if the window keeps changing its size by mistake, resulting in an unwanted fuzzy image). Personally, I rely on the Rectangle app to organize my windows on my Ultrawide Monitor. I use keyboard shortcuts to position my windows (usually arranging them in the center, left half, or right half), and having the DeskPad window size locked helps maintain the 1:1 window-to-pixel ratio when re-positioning it. I felt it was worth mentioning. However, similar to the custom resolutions, this is something I can manage in my fork. |
60d91f7
to
cc3379c
Compare
I reset my branch to yours. You wrote the code so the credit is yours :) Feel free to approve and merge! |
Just released v1.2 with all of these changes 🚀 |
This pull request addresses the following improvements: