fix: a lot of events that are stored show 0.0 duration (#39).#119
Open
EvertJanDeBruin wants to merge 1 commit intoActivityWatch:masterfrom
Open
fix: a lot of events that are stored show 0.0 duration (#39).#119EvertJanDeBruin wants to merge 1 commit intoActivityWatch:masterfrom
EvertJanDeBruin wants to merge 1 commit intoActivityWatch:masterfrom
Conversation
There was a problem hiding this comment.
❌ 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.
AsyncTask is deprecated. Consider using Kotlin coroutines or java.util.concurrent for better performance and maintainability.
0xbrayo
reviewed
Sep 14, 2024
Comment on lines
+259
to
+264
| // 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 | ||
| } |
Member
There was a problem hiding this comment.
We wish to still track the launcher events, so this bit will have to be removed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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