-
-
Notifications
You must be signed in to change notification settings - Fork 114
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
Implemented audible-as-active #85
Conversation
src/queries.js
Outdated
// that can be used to filter away non-active time. | ||
function events_active(afkbucket, audibleAsActive) { | ||
return [ | ||
'events_active = [];', |
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.
Why empty array and perdiod_union?
why not just events_active = flood(query_bucket("${afkbucket}"));
?
src/queries.js
Outdated
`events_afk = flood(query_bucket("${afkbucket}"));`, | ||
'events_active = period_union(events_active, filter_keyvals(events_afk, "status", ["not-afk"]));', | ||
].concat(audibleAsActive ? [ | ||
'events_audible = filter_keyvals(events_browser, "audible", [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.
Does [true]
work?
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.
Just experimented with a similar query in aw-research (ActivityWatch/aw-research#12) and no, it doesn't.
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.
1 or TRUE might work though, test 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.
Added boolean support in ActivityWatch/aw-core@3c4187e
src/queries.js
Outdated
'not_afk = merge_events_by_keys(not_afk, ["status"]);', | ||
'RETURN = not_afk;' | ||
]; | ||
return events_active().concat([ |
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 will make a huge performance hit, the aw-watcher-web bucket is much larger than aw-watcher-afk.
Might still be fast enough still though, I have no idea so you have to test.
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.
Ah shit, true. Don't know what I was thinking.
@@ -174,6 +174,8 @@ div | |||
div | |||
b-form-checkbox(v-model="filterAFK") | |||
| Filter away AFK time | |||
b-form-checkbox(v-model="countAudibleAsActive") |
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.
Nice!
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.
Thinking of removing this though, just adds unnecessary complexity. It's not like I'm expecting anyone to not want this, and it isn't saved across page reloads anyway which makes it kinda bad UX.
It's useful in testing though, which is an argument in favor of it I guess.
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.
It is important if someone for example often leaves their computer for extended periods of time with for example Spotify or YouTube in the foreground while not actually being at the computer but only having it for playing music.
A complete (and working) version of the query was added to aw-research here: ActivityWatch/aw-research#12 |
How's this PR doing? Needs a rebase, but seems fine otherwise? |
function windowQuery(windowbucket, afkbucket, appcount, titlecount, filterAFK) { | ||
// TODO: Take into account audible browser activity (tricky) |
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 comment should probably be removed, after testing.
@ErikBjare are there any blockers on this? I'm currently using some scripts to automatically send myself notifications based on total time spent on my PC and would love to take advantage of this feature :) |
To merge this we need to first merge #203 and then update this PR to fit those changes. |
@ErikBjare could this be added to the 1.0 roadmap? Currently absence of this PR leads to wrong reporting any time you watch any media or have an in-browser call, and the reporting can be way off depending on how long that activity goes for. Judging from number of issues on this spread through the AW repos, I would say a reasonable number of users see this as an important feature(since it directly affects the accuracy of the software) |
@nicolae-stroncea I'm not sure what the 1.0 roadmap even means at this point, but I agree that it's an important change. |
@johan-bjareholt The 1.0 roadmap is purely aspirational. I more or less made it to have an overview of more fundamental/longer-term issues, and "things we'd want in an eventual 1.0" label seemed like a good qualifier. |
It looks like all the explicit dependencies of this pull request have been merged. What is the remaining effort to get this feature available? |
What is the status of this? Does this still need testing (how could one test this)? I think this is easily one of the most important features for ActivityWatch, as this directly impacts, how accurate the data is. |
// Helper function returning a query that defines a variable events_active | ||
// that can be used to filter away non-active time. | ||
// TODO: Handle events from all available browser buckets, as done in: https://github.com/ActivityWatch/aw-webui/pull/108 | ||
// TODO: Don't count audible as activity if window isn't active |
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 a blocker.
I think we would need to finish implementing the combination of window events and browser events (ActivityWatch/aw-server-rust#179) before making headway on it.
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.
Nevermind, it works just fine as I did it in #262
Closing in favor of #262 |
Very much WIP, very untested.
Depends on ActivityWatch/aw-core#65