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

refactor: separate brave browser events into their own category #283

Merged
merged 1 commit into from
May 6, 2024

Conversation

iloveitaly
Copy link
Contributor

It looks like this was partially done, so I just cleaned it up. Also added the name of brave browser on macOS.

@codecov
Copy link

codecov bot commented May 4, 2021

Codecov Report

Merging #283 (6e6d479) into master (165ec2f) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #283   +/-   ##
=======================================
  Coverage   15.91%   15.91%           
=======================================
  Files          22       22           
  Lines        1062     1062           
  Branches      110      110           
=======================================
  Hits          169      169           
  Misses        848      848           
  Partials       45       45           
Impacted Files Coverage Δ
src/queries.ts 15.06% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 165ec2f...6e6d479. Read the comment docs.

@ErikBjare
Copy link
Member

Cool!

Short Q: what is the name of the browser bucket created by aw-watcher-web when you use Brave? I think we used to have some issues with identifying if it's actually running in Brave or if it's indistinguishable from Chrome (they've tried to avoid fingerprinting to a fault...)

@iloveitaly
Copy link
Contributor Author

what is the name of the browser bucket created by aw-watcher-web when you use Brave

I'm not using the browser extensions right now, but here's what is being reported by the window watcher:

2021-05-05 11:46:25 [DEBUG]: {'app': 'Brave Browser', 'url': 'https://www.google.com/?gws_rd=ssl', 'title': 'Google', 'incognito': False}  (aw_watcher_window.main:71)

@ErikBjare
Copy link
Member

I think this might break things (although things were probably already broken), see: ActivityWatch/activitywatch#461

@iloveitaly
Copy link
Contributor Author

@ErikBjare I didn't read the thread in detail, but I don't see how this would break anything: it should just properly categorize brave browser usage to a dedicated category instead of setting it to chrome.

Feel free to close this out if you feel this isn't useful!

@ErikBjare
Copy link
Member

ErikBjare commented May 17, 2021

Basically the "brave" field is currently unused in code since the web extension can't detect if its installed in Brave (it pretends to be Chrome). Therefore, using aw-watcher-web in Brave will report to the aw-watcher-web-chrome bucket instead of the aw-watcher-web-brave bucket (which is what the mapping is used for).

However, as @johan-bjareholt mentioned in ActivityWatch/activitywatch#461 we might be able to address this by attributing Brave the window with the aw-watcher-web-chrome bucket (move all 'brave' entries to the 'chrome' key).

However, this comes with problems if the watcher is installed in both browsers. Since both watchers would report to the same bucket, messing up the data.

It's possible that there are already Brave users on some platform (with the specific appname this PR moves) who've effectively already had this workaround, and for them this PR would break things.

Not sure what the right thing to do is here, maybe we should just merge this and fix Brave detection in aw-watcher-web. Will probably wait with merging this until there's a PR for the needed fixes to aw-watcher-web :)

@iloveitaly
Copy link
Contributor Author

Makes sense! Thanks for the explanation.

@sanderlienaerts
Copy link

sanderlienaerts commented Mar 16, 2024

Looked into this. The PR is ready for merge. Paging @ErikBjare

Used library ua_parser in aw-watcher-web includes Brave detection.

aw-watcher-web: https://github.com/ActivityWatch/aw-watcher-web/blob/0b289c4208f308050979fc729a69cf0c57dd7d8f/src/client.js#L4
ua-parser-js: https://github.com/faisalman/ua-parser-js/blob/b29a9a7ffbcb0fd0e99c5a49150c2ddb6fa91102/src/main/ua-parser.js#L1004)

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.

3 participants