Removing queues from UI #79
Conversation
|
Why did you remove the Swift Concurrency commit? There were few minor issues but it was pretty good. |
|
After you mentioned about switching to the main queue in Menu, I have realized that Swift Concurrency changes are not done right and require more thought through approach. Let me come up with proposal in Discussions later today. But I would like to merge this PR as I see it as good improvement to the codebase. And I have branch with further improvements together with feature of pinning device to the top in the menu. |
|
Sounds good! I would leave this calls on the main queue to ensure we are not breaking stuff and we can merge it |
| if self.items.contains(item) { | ||
| item.keyEquivalent = keyEquivalent | ||
| } | ||
| if self.items.contains(item) { |
There was a problem hiding this comment.
Here we are still missing the DispatchQueue.main
There was a problem hiding this comment.
I've added it back here.
There was a problem hiding this comment.
We need to check if item exists inside of the DispatchQueue.main.async call as it might not exists when we get to executing the inner closure on the main thread. Can you please for now bring back the old approach? And later on we can refactor it with Async/Await.
| if self.items.contains(item) { | ||
| item.keyEquivalent = keyEquivalent | ||
| } | ||
| if self.items.contains(item) { |
There was a problem hiding this comment.
We need to check if item exists inside of the DispatchQueue.main.async call as it might not exists when we get to executing the inner closure on the main thread. Can you please for now bring back the old approach? And later on we can refactor it with Async/Await.
| self.safeInsertItem(menuItem, at: isAndroid && UserDefaults.standard.enableiOSSimulators ? (isFirst ? index : iosDevicesCount) + 3 : 1) | ||
| } | ||
|
|
||
| let iosDevicesCount = self.devices.filter({ $0.platform == .ios }).count |
There was a problem hiding this comment.
Also here we are missing the jump back to the main thread. You could add the DispatchQueue.main.async to the safeInsertItem method.
|
@Garfeild Just two more small adjustments 😅 |
| @@ -34,19 +34,16 @@ class Menu: NSMenu { | |||
| } | |||
|
|
|||
| func getDevices() { | |||
| DispatchQueue.global(qos: .userInitiated).async { | |||
There was a problem hiding this comment.
This code was the root cause of everything being not in the main queue. And that's the most important change in the PR. New DeviceService.getAllDevices executes callback closure on the main queue by default. This allows us to remove any DispatchQueue.main calls in the Menu.
There was a problem hiding this comment.
Okay, great thanks for the explanation!
|
Hi @okwasniewski! Thanks for feedback. But I believe we are doing something wrong here.🙂I've left comment on the place, where we have main change, which makes all To be 100% sure, I've added print statements on the places you wanted me to add queue back. And here what it looks like if I run the app: I also installed Monterey in VM and I want to check binary with my changes. I will post here once I've tested it there. |
|
Ah okay, if that's the case then we can remove those calls. I don't want to introduce regressions but if they were redundant from the beginning we can remove them |
|
Let me know how your testing went and I will approve the PR 👌🏻 |
|
Update: It's working without issues on macOS 12.6.1 |
This PR is intended to improve codebase by removing explicit queue usage when DeviceService functions are called from UI. Existing async functions are changed to accept callback closure and queue, where this closure should be executed.