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

Make monitored apps scrollable #235

Merged
merged 7 commits into from
Mar 25, 2024
Merged

Conversation

starbugs
Copy link
Contributor

@starbugs starbugs commented Mar 22, 2024

Closes #232.

After some fiddling with the stack views, I gave up and rewrote the whole view to use a proper NSOutlineView, which is pretty standard on macOS. This one correctly adapts to the scroller visibility, which can be tricky on macOS and is hard to work out if you do your own customer NSScrollView with stack views setup.

What is more, there was a bug where, on first app launch, the Monitored Apps window would display all apps as disabled, even though the default apps were enabled (race condition) via MonitoringManager.enableByDefault.

I had a hard time understanding the following in the old code and I didn't overtake it to the new code. Please let me know if we need this and what it's supposed to do:

    @objc func switchToggled(_ sender: NSSwitch) {
        let isSetApp = !sender.tag.isMultiple(of: 2)
        let index = (isSetApp ? sender.tag - 1 : sender.tag) / 2
        var bundleId = MonitoredApp.allBundleIds[index]

        if isSetApp {
            bundleId = bundleId.appending("-setapp")
        }

        MonitoringManager.set(monitoringState: sender.state == .on ? .on : .off, for: bundleId)
    }

The new implementation just does this (which appears to be working fine):

    @objc func switchToggled(_ sender: NSSwitch) {
        guard sender.tag >= 0 && sender.tag < MonitoredApp.allBundleIds.count else { return }
        let bundleId = apps[sender.tag].bundleId
        MonitoringManager.set(monitoringState: sender.state == .on ? .on : .off, for: bundleId)
    }

@starbugs starbugs marked this pull request as ready for review March 22, 2024 11:24
@alanhamlett
Copy link
Member

I had a hard time understanding the following in the old code and I didn't overtake it to the new code. Please let me know if we need this and what it's supposed to do:

I wrote that code. It's not very readable, but what it does is assign a unique key to each NSSwitch based on it's index. But, we have a special case with apps installed via SetApp.com where their bundleIDs are appended with -setapp. We need to check both the original bundleID and the one installed via SetApp with -setapp suffix. Since we only have 1 list of bundleIDs, I added 2 potential NSSwitch for each bundleID. So, if an app is installed normally and with SetApp then we can have 2 NSSwitch for 1 bundleID. This above code is to check via index number if the NSSwitch is turned on, but we have to go backwards from the index number to the app's bundleID.

So, if we modify MonitoredApp.allBundleIds to only have 1 app: ["com.linear"] then we install Linear normally and via SetApp too, we have 2 NSSwitch rows. The two rows have indexes 0 and 1. So we use the formula to get the bundleID for each app via their index: 0 gives com.linear and 1 gives com.linear-setapp.

The new implementation just does this (which appears to be working fine):

That works only because you removed support for apps installed via SetApp. Here's where I added the NSSwitch for the normal bundleID and the SetApp bundleID, so potentially 2 NSSwitch per bundleID in the MonitoredApp.allBundleIds list:

https://github.com/wakatime/macos-wakatime/pull/235/files#diff-7ddf2b2fcf8afdcffb0da42497d87b4bac163f275b74711f0899d0bb0e2946cdL31

@starbugs
Copy link
Contributor Author

Ok I will fix the logic then. Thanks for the explanation.

@starbugs
Copy link
Contributor Author

starbugs commented Mar 25, 2024

This should be fixed now.

I replaced the index logic with this:

    private lazy var apps: [AppData] = {
        var apps = [AppData]()
        let bundleIds = MonitoredApp.allBundleIds.filter { !MonitoredApp.unsupportedAppIds.contains($0) }
        var index = 0
        for bundleId in bundleIds {
            if let icon = AppInfo.getIcon(bundleId: bundleId),
               let name = AppInfo.getAppName(bundleId: bundleId) {
                apps.append(AppData(bundleId: bundleId, icon: icon, name: name, tag: index))
                index += 1
            }

            let setAppBundleId = bundleId.appending("-setapp")
            if let icon = AppInfo.getIcon(bundleId: setAppBundleId),
               let name = AppInfo.getAppName(bundleId: setAppBundleId) {
                apps.append(AppData(bundleId: setAppBundleId, icon: icon, name: name, tag: index))
                index += 1
            }
        }
        return apps
    }()

The switchToggled func then simply becomes this:

    @objc func switchToggled(_ sender: NSSwitch) {
        let appData = apps[sender.tag]
        MonitoringManager.set(monitoringState: sender.state == .on ? .on : .off, for: appData.bundleId)
    }

@alanhamlett alanhamlett merged commit 38e7fb7 into main Mar 25, 2024
6 checks passed
@alanhamlett alanhamlett deleted the feature/scroll-monitored-apps branch March 25, 2024 10:31
@alanhamlett alanhamlett mentioned this pull request Mar 25, 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.

Monitored apps view should be scrollable
2 participants