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

Allow manually configuring the hostname #143

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kepstin
Copy link

@kepstin kepstin commented Dec 4, 2024

Without any obvious ability for the extension to determine the machine's hostname automatically, the best available option seems to be to allow users to configure it manually. A field is added to the extension popup to configure the hostname.

The field is blank by default, and the updates were designed to ensure that the behaviour if the hostname is not set will be unchanged (so people using workflows that expect browser data in the "unknown" host bucket won't see anything break; they can set the hostname once they're prepared to do so).

Fixes #132


If anyone using Firefox wants to test this out, I've created a signed version of the extension from this PR: aw-watcher-web-9462f488983245cbad32-0.4.8.9999.xpi. Please note that this extension is signed using my personal key and uses a different extension ID from the official ActivityWatch extension - as a result it will install alongside the official extension and will not share settings.


Important

Adds manual hostname configuration to the extension, allowing users to set a hostname via the popup UI, while maintaining default behavior if not set.

  • Behavior:
    • Adds manual hostname configuration in client.js using a new input field in the popup.
    • Defaults to previous behavior if hostname is not set, maintaining compatibility.
  • UI Changes:
    • Adds Hostname input field and Save button in popup.html.
    • Updates popup.js to handle hostname input and save functionality.
  • Functions:
    • Modifies setup() and getBucketId() in client.js to incorporate hostname logic.

This description was created by Ellipsis for 53e1345. It will automatically update as commits are pushed.

Without any obvious ability for the extension to determine the machine's
hostname automatically, the best available option seems to be to allow
users to configure it manually. A field is added to the extension popup
to configure the hostname.

The field is blank by default, and the updates were designed to ensure
that the behaviour if the hostname is not set will be unchanged (so
people using workflows that expect browser data in the "unknown" host
bucket won't see anything break; they can set the hostname once they're
prepared to do so).

Fixes ActivityWatch#132
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Reviewed everything up to 53e1345 in 1 minute and 19 seconds

More details
  • Looked at 139 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. src/client.js:82
  • Draft comment:
    Use !== for strict comparison to avoid type coercion issues.
    if (hostname !== "")
  • Reason this comment was not posted:
    Marked as duplicate.
2. static/popup.js:42
  • Draft comment:
    Use !== for strict comparison to avoid type coercion issues.
    hostnameTextbox.value = obj.hostname !== "" ? obj.hostname : "";
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_MUjTg3FZ8rM1bBz4


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.

});
},

getBucketId: function () {
return "aw-watcher-web-" + client.browserName.toLowerCase();
if (client.hostname != "")
Copy link

Choose a reason for hiding this comment

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

Use !== for strict comparison to avoid type coercion issues.

Suggested change
if (client.hostname != "")
if (client.hostname !== "")

@cweiske
Copy link

cweiske commented Jan 4, 2025

The extension works fine for me, but I think that the hostname settings should be in the preferences instead of the main window.

cweiske added a commit to cweiske/aw-watcher-web that referenced this pull request Jan 4, 2025
Options can be opened from the popup window.

Related: ActivityWatch#143
@cweiske
Copy link

cweiske commented Jan 4, 2025

I've modified the patch and moved the settings to a separate options page: kepstin#1

This way the hostname configuration is out of the popup window, but can be accessed when needed.

@maaasyn
Copy link

maaasyn commented Jan 9, 2025

Same could be done for browser selection. Currently the flow expects the correct output of the ua_parser. In the case of Arc browser the ua_parser says its chromium which breaks the aw watcher. On my side I've hardcoded the browser to Arc, but select option in popup would be better.

@adamjames
Copy link

Just want to +1 this -- I just noticed the hostname not being set and will really appreciate being able to fix up my data when it lands :)

@cweiske
Copy link

cweiske commented Jan 29, 2025

@ErikBjare @johan-bjareholt What can we do to get this patch (respective my adjusted version) merged?

@BelKed
Copy link
Contributor

BelKed commented Feb 4, 2025

What about loading the hostname from localhost:5600/api/0/info? 👀
It may also have a nice implementation in https://github.com/ActivityWatch/aw-client-js


Reference: ActivityWatch/activitywatch#307

@ErikBjare
Copy link
Member

@BelKed Yes, that was the intended way to do it. (sorry y'all for not mentioning this sooner)

Not sure if it breaks peoples existing installs on update if the default behavior changes though, a safe-guard could be to check for the presence of a -unknown bucket and still use it if found (since having both would presumably break the Activity view).

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.

Update on issue of aw-watcher-web reporting unknown hostname
6 participants