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

feat: add event_duration as a config parameter #50

Closed

Conversation

mkvoya
Copy link

@mkvoya mkvoya commented Mar 21, 2021

This PR adds the event_duration configuration which sets the duration of each event sent to aw-server.
Users can set the configuration to the same value of poll_time, so that in the web-ui's timeline two consecutive events show as two lines rather than two dots, even if the two events indicate different windows and cannot be merged. This makes the timeline look more nature, but as a price, it adds a new assumption that the window keeps unchanged within the event_duration time.
The default event_duration is 0, which does not change the current behavior of aw-watcher-window.

This PR is a follow-up to the PR in aw-client.

@ErikBjare
Copy link
Member

ErikBjare commented Mar 22, 2021

Sorry but this is a bit too much of a hacky workaround for me to want to merge it.

I realize I indicated otherwise in ActivityWatch/aw-client#53 (comment), but "polluting" the data with assumptions like this at the watcher level seems like trouble.

@mkvoya
Copy link
Author

mkvoya commented Mar 22, 2021

Hi @ErikBjare, I don't think the PR is that hacky since what it does is simply exposing the default parameter to the configuration file.
The default value of the event duration parameter (which is 0 as in the modified config file) does not change the current aw-watcher-window's behavior, and whether to change the event duration parameter (and take the assumption) is totally up to the user.

Additionally, exposing the event duration parameter is orthogonal to the improved window title query in #40. Even if querying the window title can be faster, users may not want to invoke that too frequently.

@ErikBjare
Copy link
Member

ErikBjare commented Mar 22, 2021

The default value of the event duration parameter (which is 0 as in the modified config file) does not change the current aw-watcher-window's behavior, and whether to change the event duration parameter (and take the assumption) is totally up to the user.

Breaking the core tenet of 'leave assumptions to the analysis stage' is bound to cause issues. If we let users set it that will break assumptions we make about zero-duration events in the analysis stage, likely leading to confusing bugs and other trouble (that I don't want to have to investigate when they inevitably create bug reports...).

I simply don't want to merge and maintain a configuration option that only a single (or very few) user(s) will use as a workaround for another issue, especially when it comes with significant drawbacks.

Even if querying the window title can be faster, users may not want to invoke that too frequently.

Why not? If it's fast there's no reason not to. Setting poll_time to a higher value, like 30s in your case, will ruin the data resolution to an unacceptable degree imo (I have regrets about making this configurable in the first place...)

@mkvoya
Copy link
Author

mkvoya commented Mar 22, 2021

Breaking the core tenet of 'leave assumptions to the analysis stage' is bound to cause issues. If we let users set it that will break assumptions we make about zero-duration events in the analysis stage, likely leading to confusing bugs and other trouble (that I don't want to have to investigate when they inevitably create bug reports...).

Okay. Then as I stated in the previous comment, different events may have different poll_time, which the analyzer is not aware of. One possible solution could be adding the current poll_time to each event, but it will make the event (i.e., transfer size) larger.

I simply don't want to merge and maintain a configuration option that only a single (or very few) user(s) will use as a workaround for another issue, especially when it comes with significant drawbacks.

Okay, I can understand. Never mind. :)

Why not? If it's fast there's no reason not to. Setting poll_time to a higher value, like 30s in your case, will ruin the data resolution to an unacceptable degree imo (I have regrets about making this configurable in the first place...)

Because for some users, the frequency can be unnecessarily high for daily usage tracking.
No matter how fast the process is, it's overhead to both performance and battery life.

At end, never mind if you think the PR is unnecessary, and feel free to close it. :)

@ErikBjare
Copy link
Member

No matter how fast the process is, it's overhead to both performance and battery life.

The hope is that improved speed will make the impact on performance and battery life negligible. For example, aw-server-rust is probably a 10x improvement over aw-server-python in both regards (on other platforms than macOS, where aw-watcher-window isn't the bottleneck).

Regardless, I'm grateful you took the time to engage and contribute this PR, even if it won't get merged, so thank you for contributing! ❤️

@ErikBjare ErikBjare closed this Mar 22, 2021
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.

2 participants