Skip to content

Update to using the latest aw-client#6

Merged
johan-bjareholt merged 6 commits intoActivityWatch:masterfrom
devzsolt:feature/switch-to-events
Dec 9, 2018
Merged

Update to using the latest aw-client#6
johan-bjareholt merged 6 commits intoActivityWatch:masterfrom
devzsolt:feature/switch-to-events

Conversation

@devzsolt
Copy link
Copy Markdown
Contributor

@devzsolt devzsolt commented Dec 9, 2018

This changeset contains several small changes:

  • update submodules aw-client-js and media to latest
  • fix the extension to work with the latest aw-client:
    • AppEditorActivityHeartbeat --> AppEditorEvent
    • createBucket --> ensureBucket
    • options object in AWClient constructor
    • timestamp should be a Date not a string
  • fix lint error in catch of _sendHeartbeat
  • restore _getProjectName() to populate field projectName (I liked the previous logic better so restored it into a new field keeping project intact logging the project path)
  • also includes my previous PR fix: security vulnerability of an outdated dependency: url-parse #5

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 with the addition of the new version of aw-client-js!

I haven't had time to test this and @Otto-AA is the one with the most expertise with this watcher so I'll let him review.

But looks pretty good to me!

Comment thread src/extension.ts
duration: 0,
data: {
language: this._getFileLanguage() || 'unknown',
projectName: this._getProjectName() || 'unknown',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When I and @Otto-AA first made the support for editors and discussed the format and came up with this after some discussions:
https://activitywatch.readthedocs.io/en/latest/buckets-and-events.html#app-editor-activity

The reason why we don't have a read-friendly "projectName" property is because many editors don't support that and we want to support as many as possible, so the web-ui simply checks for a path and takes the name of the top folder. In the case of vscode where we actually have a read-friendly project name that's great, but we are not going to be using that property to display in aw-webui so it's essentially going to be worthless in the end unless someone does third-party visualization with an edge-case for this watcher specifically which doesn't seem very likely.

So in conclustion, while the code looks good it's not going to be used so I suggest to just keep the code base small and remove it.

@Otto-AA
Copy link
Copy Markdown
Collaborator

Otto-AA commented Dec 9, 2018

Thanks for your contribution, it seems fine to me 👍
I haven't installed activitywatch since my last computer reset, so I can't test it, but from the logic it seems nice.

Regarding merging and publishing via vsce, I guess it would be better if someone else becomes the maintainer of this watcher. I myself sadly don't have time for maintaining, testing new pull requests, finding bugs, etc. anymore, so I would suggest to find a new maintainer or mark it as unmaintained.
@johan-bjareholt

And here is the discussion about the format if you want to read it: https://forum.activitywatch.net/t/bucket-and-event-design-fo-vs-code-extension/120/9

@johan-bjareholt
Copy link
Copy Markdown
Member

@Otto-AA Alright, I guess that I can take up the maintainership until we find a better maintainer I guess then.
I'm not using vscode on a daily basis, but I feel comfortable with the code so it'll do.

@Otto-AA
Copy link
Copy Markdown
Collaborator

Otto-AA commented Dec 9, 2018

@johan-bjareholt Is your email address from github still correct? If yes, I will give you owner rights for the extension (for publishing via vsce)

@johan-bjareholt
Copy link
Copy Markdown
Member

@Otto-AA Yes that is still my mail, that'll work fine.

Also, I tested the code and everything seems to work fine

@johan-bjareholt
Copy link
Copy Markdown
Member

I'll merge this, fix the comment I made and add it to the ChangeLog because I'm likely not going to have time to review this later in the week.

@johan-bjareholt johan-bjareholt merged commit d47d3a4 into ActivityWatch:master Dec 9, 2018
@johan-bjareholt
Copy link
Copy Markdown
Member

johan-bjareholt commented Dec 9, 2018

@devzsolt Thanks for the PR by the way! 😃

@devzsolt
Copy link
Copy Markdown
Contributor Author

@johan-bjareholt @Otto-AA you're welcome. Sorry that I didn't follow the conversation. I'm glad that you have found my PRs useful. I'm using it for almost a week now and works well. Thank you for developing this extension in the first place!

Thanks for explaining why you discarded the project name idea. I'm not sure yet if I'll ever use it but in the future I'm thinking about some pattern based categorization and a simplified custom visualization. Of course the full path is unique for pattern matching and possibly the patterns would have a metadata containing the project name (which I would enter once manually)... so you're right, it's possibly useless data :)

@ErikBjare
Copy link
Copy Markdown
Member

@devzsolt If you're interested in categorizing activity using pattern-matching, check out my work in aw-research here: ActivityWatch/aw-research#12

And the general issue on the matter: ActivityWatch/activitywatch#95

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.

4 participants