-
-
Notifications
You must be signed in to change notification settings - Fork 131
feat: Add ability to better visualize value of custom events if not using title #698
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
Open
petrroll
wants to merge
1
commit into
ActivityWatch:master
Choose a base branch
from
petrroll:visualize-non-standard-event-values
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
feat: Add ability to better visualize value of custom events if not using title #698
petrroll
wants to merge
1
commit into
ActivityWatch:master
from
petrroll:visualize-non-standard-event-values
Conversation
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
Contributor
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.
Important
Looks good to me! 👍
Reviewed everything up to c528deb in 1 minute and 53 seconds. Click for details.
- Reviewed
26lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/util/color.ts:127
- Draft comment:
Using 'if (title && typeof title === "string")' will skip empty strings. Confirm if an empty title should be treated as absent. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The current code checks both for truthiness (title &&) and type. This means empty strings will be falsy and skip to the fallback logic. The function has a clear fallback strategy - it tries to get a title from a single key's value, and ultimately returns empty string if nothing works. This seems intentional - empty strings are treated the same as missing titles. Maybe empty strings should be valid titles and returned as-is rather than triggering the fallback logic? The comment raises a valid technical point about string handling. The code's behavior of treating empty strings as invalid titles appears intentional, as evidenced by the careful fallback logic. The comment is technically correct but suggests changing intentional behavior. Delete the comment. While technically accurate about string handling, it questions intentional behavior and doesn't identify a clear issue that needs fixing.
2. src/util/color.ts:131
- Draft comment:
Fallback logic: If no valid title exists and e.data has only one key, its string value is used. Consider adding a comment to document this assumption for custom events. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment is about code that was just added in this PR. It's suggesting a documentation improvement which is actionable. However, this appears to be an internal utility function and the fallback behavior is quite straightforward from the code itself. The logic isn't really an "assumption" - it's just a fallback mechanism for extracting a displayable title. The fallback logic might be important for maintainers to understand. Not documenting it could lead to confusion about why this specific fallback was chosen. The code is self-documenting - it clearly shows the fallback logic in a straightforward way. The behavior isn't complex enough to warrant additional documentation. While the comment points out something real, the suggested documentation would be redundant given how clear and straightforward the code already is.
3. src/util/color.ts:134
- Draft comment:
Only string values are considered in fallback. If non-string (e.g. number or boolean) values might be meaningful, consider converting them with String(val). - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The code is a title attribute getter that falls back to different strategies. The current implementation deliberately chooses to only use string values and falls back to empty string otherwise. Converting non-string values could lead to undesirable titles like "true" or "123". The code's approach of being strict about string values seems intentional. The comment could be right - maybe there are legitimate use cases where numeric or boolean titles would be meaningful. The empty string fallback might be too restrictive. Without more context about the application's requirements and what kind of data typically flows through this function, suggesting to blindly convert non-string values is speculative. The strict string-only approach seems like a deliberate design choice. The comment should be deleted as it makes a speculative suggestion without strong evidence that non-string values should be handled.
4. src/util/color.ts:135
- Draft comment:
The fallback value is truncated to 50 characters. Consider making this limit configurable or document why 50 characters is chosen. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is a UI-related change that truncates display text. The 50-character limit seems arbitrary but is likely chosen for display purposes. Making it configurable would add complexity for minimal benefit since this is just a fallback case. The rules say not to comment on pure UI/styling changes. The truncation could affect functionality if other parts of the system rely on the full string value. The 50-character limit might need to be consistent with other truncation limits in the codebase. Since this is in getTitleAttr() which is clearly for display purposes, and it's a fallback case, the exact truncation length is not critical to functionality. This falls under UI styling decisions that we should trust the author on. Delete the comment as it relates to a UI display decision and doesn't highlight a clear code quality issue requiring action.
Workflow ID: wflow_dRlIyRDH9D5PeHKz
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
For my virtual desktop name watcher (), I'm using custom event-type with custom key (which might be wrong?) which means the visualization doesn't pick up the events well. All events are just blue and don't have a title. The data is there in tooltip but that's not good for glancing.
I think that if there's only one piece of data, and there's no title, we can extract the single piece of data as event.
Happy to also be told that custom watchers should use title for that purse, if that's the design. And I can prepare PR for docs and abandon this one.
Important
Enhances
getTitleAttrincolor.tsto extract a single data piece as title for custom events without a title.getTitleAttrincolor.ts, ife.data.titleis missing, extract single data piece as title if only one key exists ine.dataand its value is a string.titleor multiple data keys.This description was created by
for c528deb. You can customize this summary. It will automatically update as commits are pushed.