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

New API: UserSettings.isOnToolbar change event #346

Open
thusimon opened this issue Jan 31, 2023 · 7 comments
Open

New API: UserSettings.isOnToolbar change event #346

thusimon opened this issue Jan 31, 2023 · 7 comments
Labels
enhancement Enhancement or change to an existing feature supportive: chrome Supportive from Chrome supportive: firefox Supportive from Firefox supportive: safari Supportive from Safari

Comments

@thusimon
Copy link

The MV3 new API chrome.action.getUserSettings returns isOnToolbar, indicating whether the extension icon is pinned on the toolbar.

It would be better to have a new API to listen to UserSettings values change.

Use cases:
With this new API, we will know if user pinned toolbar icon accurately and can show some instruction on the web page
For example

  1. user go to a home page https://foo.com and user's toolbar icon is pinned
  2. user unpins the toolbar icon
  3. extension can send some event or execute some script to the home page
  4. the home page then can show a tooltip to ask user to pin the toolbar icon

This value change API can achieve the above without the extension polling the chrome.action.getUserSettings or refreshing the home page.

@dotproto dotproto added enhancement Enhancement or change to an existing feature agenda Discuss in future meetings and removed needs-triage labels Feb 2, 2023
@Rob--W Rob--W added follow-up: chrome Needs a response from a Chrome representative supportive: safari Supportive from Safari supportive: firefox Supportive from Firefox and removed agenda Discuss in future meetings labels Feb 2, 2023
@dotproto
Copy link
Member

dotproto commented Feb 2, 2023

I agree that it would be useful to have an event to observe when the extension's isOnToolbar state changes, but I don't think such an event should be specific to that property. Instead, I'd suggest introducing a new event to get notified when any action user setting value changes.

Following the naming convention used by other extension platform APIs, here's a potential shape of the API in .d.ts format (based on @types/chrome definitions).

interface GenericChange<T> {
  /** MUST be omitted in the following scenarios:
   * 1. The underlying value did not previously exist.
   */
  oldValue?: T;

  /** MUST be omitted in the following scenarios:
   * 1. The underlying value was deleted. 
   */
  newValue?: T;
}

type GenericChangeList<T> = {
  [Property in keyof T]?: GenericChange<T[Property]>;
}

declare namespace browser.action {
  interface UserSettings {
    isOnToolbar: boolean;
  }
  
  interface UserSettingsChangeList extends GenericChangeList<UserSettings> { }

  interface UserSettingsChangeInfo {
    changeInfo: UserSettingsChangeList;
  }

  interface UserSettingsChangedEvent
      extends browser.events.Event<(changeInfo: UserSettingsChangeList) => void> { }

  export var onUserSettingsChange: UserSettingsChangedEvent;
}

// Example usage
browser.action.onUserSettingsChange.addListener(function(changeInfo) {
  console.log(`isOnToolbar was: '${changeInfo.isOnToolbar?.oldValue}'`);
  console.log(`isOnToolbar is:  '${changeInfo.isOnToolbar?.newValue}'`);
});

@oliverdunk
Copy link
Member

Here's an issue I opened on the Chromium side a while back: https://bugs.chromium.org/p/chromium/issues/detail?id=1378780. I'm going to try to speak to some developers on our side and see what our initial thoughts are.

@xeenon
Copy link
Collaborator

xeenon commented Feb 8, 2023

I like the TypeScript declaration format for this.

@hanguokai
Copy link
Member

I would like to remind that the state of Pin is a synchronized property. Users Pin or Unpin on one device will also synchronize with the other devices. Also, I've submitted a related bug to Chrome, and they didn't seem to be able to solve the problem.

@rdcronin
Copy link
Contributor

I'm generally supportive of this (a way to listen for action settings changes). As thusimon pointed out, this is already possible via polling, and it'd be nice to provide developers with a cleaner solution.

I'm more hesitant about @dotproto 's proposed API surface. In a future world, it'd be great to move more towards typescript-style interfaces, APIs, and types — but I don't think that this would work well with our current extension API system in Chrome (where properly supporting it is a very large undertaking and kludging it together would result in significant drawbacks, like less type-safety, worse documentation, etc). There's also the issue that it less resembles most other APIs (with the exception of the storage API events).

What do you think instead of taking a simpler approach and having this be an event that dispatches an updated UserSettings object (which currently only contains isOnToolbar)? Given there is only one property and that property is a (never undefined) boolean, I think that functionally gets us the same benefit as the more broad interface. If, in the future, we add more settings, we could discuss whether it makes sense to have the sent UserSettings reflect changed properties only or another approach. This, though, would allow this to be something we can get to in the relatively near term (whereas implementing an API using a new style of API interface would likely not be something we could tackle immediately).

@dotproto
Copy link
Member

In a future world, it'd be great to move more towards typescript-style interfaces, APIs, and types — but I don't think that this would work well with our current extension API system in Chrome

Sorry I didn't provide more context about my intent with that TypeScript definition. My primary goal was to communicate the shape of the API that I had in my mind as unambiguously as I could. I also considered using WebIDL, but decided to use TypeScript here because I was more confident in my ability to use it to communicate my intent.

I didn't mean to suggest that the group should specify APIs using TypeScript definitions; that's a whole other issue we can dive into another day.

There's also the issue that it less resembles most other APIs (with the exception of the storage API events).

This is more the type of concern I was hoping to surface. As you indicated, my suggestion was primarily modeled after the Storage API. As an extension developer, one of the things I like about storeage.onChanged() is that by providing both newValue and oldValue in the change event, the extension's event handler can be (more) stateless. The extension doesn't have to track, persist, or load the state of a given property to react to the change; it can take action based on the information provided in the event.

One of my goals with this suggestion was to suggest a common pattern that we could use across the platform for change events. While it may be overkill for this specific case, if all change events across the platform behaved in the same general way, the platform as a whole would be more consistent and predictable for developers.

What do you think instead of taking a simpler approach and having this be an event that dispatches an updated UserSettings object (which currently only contains isOnToolbar)?

While I would like to establish a generic pattern that we can use for change events across the platform, the approach you've suggested has a strong practical advantage. I don't object.

@oliverdunk
Copy link
Member

Adding supportive: chrome Supportive from Chrome . Per our last meeting, we've agreed on an onUserSettingsChanged event which fires with a single object representing the new value. This object may contain all properties or may only contain what has changed - we can decide that later as the two are equivalent until there are multiple properties.

At @dotproto's request, an updated TypeScript interface would look something like this:

declare namespace browser.action {
  interface UserSettings {
    isOnToolbar: boolean;
  }

  interface UserSettingsChangedEvent
      extends browser.events.Event<(changeInfo: UserSettings) => void> { }

  export var onUserSettingsChanged: UserSettingsChangedEvent;
}

@oliverdunk oliverdunk added supportive: chrome Supportive from Chrome and removed follow-up: chrome Needs a response from a Chrome representative labels Feb 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement or change to an existing feature supportive: chrome Supportive from Chrome supportive: firefox Supportive from Firefox supportive: safari Supportive from Safari
Projects
None yet
Development

No branches or pull requests

7 participants