fix: misc fixes to swift script on macos, incl URL inspection #69
Conversation
|
@ErikBjare @modderme123 mind taking a look at this one? |
|
Looks good, although if you are removing the idle detection loop you might be able to remove the system idle time function as well |
|
Nice tip! Just removed in the latest commit. @modderme123 do you recall what this comment is referring to?
What's not right about the current pulsetime setup? |
|
Here's how I understand pulsetime is used: https://docs.activitywatch.net/en/latest/_modules/aw_transform/heartbeats.html#heartbeat_reduce This is very much intended for a polling system so that it doesn't ever need to diff anything, and can just send the latest value, and it will get merged if the pulsetime is long enough. Because we intentionally don't poll, pulsetime doesn't make as much sense for us. On a new event, we just want to make sure the previous event duration is extended until the start of our new event, and the new event is created ASAP. To extend the previous duration, we send the previous event again (except at the current time), with a pulsetime long enough that it gets merged. Then it doesn't really matter what the new event pulsetime is, because it shouldn't get merged (it necessarily has a different value, because our notifier was triggered). I assume the original comment added by @ErikBjare was referring to wanting to switch partially over to a hyrbrid polling model like the chrome extension does (use a notifier for precise updates and then send them on a timeout, which seems like it accidentally loses resolution if you rapidly switch between apps, but will also automatically extend events even without switching between apps)... |
Reason the existing logic wasn't working is the previous heartbeat is not sent with the current timestamp to the server does not merge it
|
@modderme123 thanks for the comment, very helpful! @ErikBjare this is ready for review and is working great for me locally. The main changes here are:
|
| @@ -342,6 +390,7 @@ class MainThing { | |||
| } | |||
| } | |||
|
|
|||
| // TODO I believe this is handled by the python wrapper so it isn't needed here | |||
There was a problem hiding this comment.
Hmm, probably just so long as they are not separate binaries. (Note that the built in one should probably use a while loop because otherwise you can accidentally silence it by opening settings)
There was a problem hiding this comment.
Yeah, I don't think it's a massive deal either way just additional overhead to make sure they are hitting the right server APIs.
aw_watcher_window/macos.swift
Outdated
| let refreshedOldHeartbeat = Heartbeat( | ||
| // it is important to refresh the hearbeat using the timestamp where the user stopped working on the previous application | ||
| // TODO unsure if this is necessary: does the timestamp of the old vs new event need to be distinct | ||
| timestamp: heartbeat.timestamp - 1, |
There was a problem hiding this comment.
Why timestamp-1? (I think it would be better to make pulsetime extra long instead of making anything with inaccurate timestamps)
There was a problem hiding this comment.
What I wasn't certain about here if we can send two events with the same timestamp (i.e. the old, refreshed heartbeat) and the new heartbeat. Sounds like you didn't have any issues with that?
(this was me being lazy and not testng)
There was a problem hiding this comment.
Oh I understand what this is doing now. Are the units ms or s? This seems reasonable if it is ms
There was a problem hiding this comment.
Pretty certain all units on the aw-server side of things are in seconds. Do we know if we can send two events with the same timestamp without causing any issues?
There was a problem hiding this comment.
Do we know if we can send two events with the same timestamp without causing any issues?
It shouldn't cause issues, but I've guarded for similar situations in other places by subtracting/adding 1ms to events.
There was a problem hiding this comment.
Got it, I'll drop it to a millisecond subtraction and call it a day. I think all comments are addressed—good to merge!
|
|
||
| // Then, send heartbeat starting new event | ||
| let payload_new = heartbeat | ||
| // if you resize a window a ton of events (subsecond) will be fired |
There was a problem hiding this comment.
We should probably instead just compare if the new title is the same and then skip that message (and/or decrease this to 0.1), that way we don't lose rapid tab switches
There was a problem hiding this comment.
Ah, this is definitely better. We can compare app & title instead—I'll do this.
There was a problem hiding this comment.
Actually, thinking about this further I am not sure the title comparison works. In my situation, resizing an iterm window changes the reported title of the window to represent the size of the terminal. I imagine there are other scenarios like this as well where the title changes during resize.
Ideas:
- Add an additional check to ensure app names are the same
- Drop interval requirement to 0.5s
I think the interval change makes the most sense—it's simple, and will handle most cases (who is switching tabs more than once a second?)
|
@iloveitaly Waiting for you to address @modderme123's comments :) |
|
@ErikBjare @modderme123 comments addressed! |
|
@iloveitaly Before merging this, does this resolve ActivityWatch/activitywatch#787? |
|
@ErikBjare I think so, although I wasn't running into the identical issue he experienced. My window bucket was empty, but I had no issues with accessibility permissions. I've been running this for a couple weeks at this point and my window activity is back to normal on macos. |
|
Alright, I tested it and it worked for both Chrome and Safari. However, I decided to dig a bit deeper, and removed the Automation permissions for my terminal (Alacritty). After that, titles and URLs were blank again. At minimum, it feels like we should at least get a window title even without Automation permissions, right?
(took the screenshot after I enabled again, at which point it started working again) |
…tion-provided window title
|
Not at my Macbook, so can't test it myself, but going to eagerly assume this is good to merge. Thanks again for the contributions @iloveitaly! Sorry for the slow merge :) |
ErikBjare
changed the title
The new macos window watcher is working great so far! Will do some more testing over the next couple weeks.