-
-
Couldn't load subscription status.
- Fork 31
fix: a lot of events that are stored show 0.0 duration (#39). #119
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
base: master
Are you sure you want to change the base?
Conversation
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.
❌ Changes requested. Reviewed everything up to b843e45 in 32 seconds
More details
- Looked at
218lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. mobile/src/main/java/net/activitywatch/android/watcher/UsageStatsWatcher.kt:247
- Draft comment:
TheheartbeatsSentvariable is declared but not used effectively. Consider removing it or using it to track the number of heartbeats sent. - Reason this comment was not posted:
Confidence changes required:50%
TheheartbeatsSentvariable is declared but not used effectively. It should be removed or properly utilized.
2. mobile/src/main/java/net/activitywatch/android/watcher/UsageStatsWatcher.kt:288
- Draft comment:
EnsureactiveAppEventis reset appropriately to avoid incorrect behavior when processing new events. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The code already resetsactiveAppEventto null after processing an ACTIVITY_PAUSED event, which seems to handle the concern raised by the comment. The comment is speculative and asks for verification, which is not allowed by the rules.
I might be overlooking a scenario whereactiveAppEventis not reset correctly, but the current logic seems to handle the reset appropriately.
The logic for resettingactiveAppEventappears to be correctly implemented, and the comment does not point out a specific issue that needs addressing.
The comment should be removed as it is speculative and the code already handles the reset ofactiveAppEventappropriately.
3. mobile/src/main/java/net/activitywatch/android/watcher/UsageStatsWatcher.kt:266
- Draft comment:
EnsurelogEventDetailsis necessary for debugging, as it is only called when logging is enabled. This is good for performance but verify its necessity. - Reason this comment was not posted:
Confidence changes required:30%
ThelogEventDetailsfunction is called only if logging is enabled, which is good for performance, but ensure it's necessary for debugging.
4. mobile/src/main/java/net/activitywatch/android/watcher/UsageStatsWatcher.kt:223
- Draft comment:
Consider makingpulseTimeconfigurable instead of using fixed values to allow more flexibility for different use cases. - Reason this comment was not posted:
Confidence changes required:40%
ThesendHeartbeatfunction uses a fixed pulse time which might not be flexible for all use cases.
Workflow ID: wflow_uy4fcNtvRzGwYnnO
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| } | ||
| } | ||
|
|
||
| // TODO: AsyncTask is deprecated, replace with kotlin concurrency or java.util.concurrent |
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.
AsyncTask is deprecated. Consider using Kotlin coroutines or java.util.concurrent for better performance and maintainability.
| // do not include launchers, since they are used all the time to switch between apps. It distorts the timeline while | ||
| // it is more part of the OS than an app which we want to monitor | ||
| if( event.packageName.contains("launcher", false) ) { | ||
| Log.d(TAG,"Skipping launcher event for package " + event.packageName) | ||
| continue@nextEvent | ||
| } |
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.
We wish to still track the launcher events, so this bit will have to be removed.
Note: I did not build unit-tests (yet) since this requires quite some restructuring. Also, some discussion is required regarding the 'unlock' bucket (it seems total screentime, where each event is an unlock, is perhaps better).
Summary:
Fixes event duration issue by updating
EventandUsageStatsWatcherto handle app activity more accurately, ensuring correct heartbeat timing.Key points:
Event.fromUsageEventinmobile/src/main/java/net/activitywatch/android/models/Event.ktto acceptcustomTimestampfor precise timing.SendHeartbeatsTaskinmobile/src/main/java/net/activitywatch/android/watcher/UsageStatsWatcher.ktto handleACTIVITY_RESUMEDandACTIVITY_PAUSEDevents.Generated with ❤️ by ellipsis.dev