-
Notifications
You must be signed in to change notification settings - Fork 56
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
Comments
I agree that it would be useful to have an event to observe when the extension's Following the naming convention used by other extension platform APIs, here's a potential shape of the API in 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}'`);
}); |
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. |
I like the TypeScript declaration format for this. |
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. |
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). |
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.
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 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.
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. |
Adding
supportive: chrome
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;
} |
The MV3 new API
chrome.action.getUserSettings
returnsisOnToolbar
, 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
https://foo.com
and user's toolbar icon is pinnedThis value change API can achieve the above without the extension polling the
chrome.action.getUserSettings
or refreshing the home page.The text was updated successfully, but these errors were encountered: