Skip to content

Combine and display chrome + firefox data in Browser Section#108

Closed
nicolae-stroncea wants to merge 6 commits intoActivityWatch:masterfrom
nicolae-stroncea:allBrowserDatanoArrows
Closed

Combine and display chrome + firefox data in Browser Section#108
nicolae-stroncea wants to merge 6 commits intoActivityWatch:masterfrom
nicolae-stroncea:allBrowserDatanoArrows

Conversation

@nicolae-stroncea
Copy link
Copy Markdown
Member

@nicolae-stroncea nicolae-stroncea commented Aug 30, 2018

Copy link
Copy Markdown
Member

@johan-bjareholt johan-bjareholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

I was a bit sceptical about this at first due being unsure where we are supposed to use "flood" and not in this case. After thinking a bit about it this seems to be correct, to flood the browser buckets but not the window events after filtering by app.

I have not looked at the rest of the code nor tested it, only looked at the new query.

Thoughts @ErikBjare?

@ErikBjare
Copy link
Copy Markdown
Member

@johan-bjareholt The best way to figure out if you've done flooding right is to simply inspect the events in the source buckets and the final events used in the query result (I'd suggest using the new timeline).

But I think this is the right way to go, flooding should generally be done as early as possible.

@nicolae-stroncea
Copy link
Copy Markdown
Member Author

nicolae-stroncea commented Aug 31, 2018

I tested both approaches initially. Flooding the window events led to the durations being too large, which I assume is due to both browser events intersecting with the same timestamps from the window watcher. Doing it separately ends up with the correct intersections.

@ErikBjare
Copy link
Copy Markdown
Member

@nicolae-stroncea Alright, just fix the issues in ActivityWatch/aw-core#70 and we'll merge this.

@ErikBjare
Copy link
Copy Markdown
Member

Also, it looks like you did something weird with aw-client-js which broke Travis. Check out the latest aw-client-js commit from master and commit that.

@johan-bjareholt
Copy link
Copy Markdown
Member

Oh, I forgot about this PR completely.

Fix the conflict and we'll merge this directly.

@ErikBjare
Copy link
Copy Markdown
Member

I've fixed the issues and cleaned up stuff in #141.

Closing this one, thanks for the amazing work @nicolae-stroncea! (and sorry for the delay)

@ErikBjare ErikBjare closed this May 7, 2019
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