Improve type definitions #11
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I've turned this into a draft as now that I've overcome #10 I have a few more fixes ready. |
1 task
|
I think I've fixed all errors I've hit in my extension, see swsnr/gnome-shell-extension-picture-of-the-day#38 |
Merged
1 task
Closed
schnz
reviewed
Jan 1, 2024
Align types with documentation at https://gjs.guide/extensions/topics/extension.html#extensionmetadata Mark all properties as readonly according to documentation. Make required properties name, description, and shell-version non-optional, as GNOME Shell would not load an extension without these properties anyway, see ExtensionManager.createExtensionObject. Add missing dir and path properties which GNOME Shell adds to the metadata when creating the extension instance, see ExtensionManager._callExtensionInit Add version-name metadata field, which extensions can set these days to use a human-readable version instead of the auto-generated counter on extensions.gnome.org
Add label properties for PopupMenuItem, PopupSeparatorMenuItem, and
PopupSubMenuMenuItem.
Make "text" argument to PopupSeparatorMenuItem optional because the
constructor handles the case of it being falsy as it initializes the
label with:
this.label = new St.Label({text: text || ''});
Remove sensitive property from PopupMenuSection, because the
implementation does not enforce that sensitive is not changed.
PopupMenuBase.addAction explicitly checks whether `icon` is undefined, see https://gitlab.gnome.org/GNOME/gnome-shell/-/blob/2b72a3fd81bb98f042ce0a37da6b7949deabfdaa/js/ui/popupMenu.js#L560 let menuItem; if (icon !== undefined) menuItem = new PopupImageMenuItem(title, icon); else menuItem = new PopupMenuItem(title); As Javascript allows callers to omit arguments which then become undefined this effectively makes icon an optional argument.
Notification handles the params in Notification.update which uses
Params.parse on params:
params = Params.parse(params, {
gicon: null,
secondaryGIcon: null,
bannerMarkup: false,
clear: false,
datetime: null,
soundName: null,
soundFile: null,
});
See https://gitlab.gnome.org/GNOME/gnome-shell/-/blob/2b72a3fd81bb98f042ce0a37da6b7949deabfdaa/js/ui/messageTray.js#L401
Params.parse in turn defines the first argument with a default value
of {}:
export function parse(params = {}, defaults, allowExtras) { … }
See https://gitlab.gnome.org/GNOME/gnome-shell/-/blob/2b72a3fd81bb98f042ce0a37da6b7949deabfdaa/js/misc/params.js#L20
Hence, passing undefined for params is possible, which effectively makes
params an optional parameter.
While the default implementation returns a Gio.ThemedIcon the icon is
effectively used to initialize an St.Icon via its gicon property:
// Called to create a new icon actor.
// Provides a sane default implementation, override if you need
// something more fancy.
createIcon(size) {
return new St.Icon({
gicon: this.getIcon(),
icon_size: size,
});
}
getIcon() {
return new Gio.ThemedIcon({name: this.iconName});
}
As such, getIcon can return any kind of Gio.Icon.
ModelDialog uses Params.parse to unpack the constructor parameters, which implies that the params parameter itself as well as every single of its fields are optional.
This allows to extend the signal types for custom base classes of any of these types. The interfaces are empty because I didn't care to figure out all signals for the affected popup menu types, but still provided for forward compatibility in order to allow signal types to be defined without introducing new interfaces.
It not only accepts PopupBaseMenuItem but also a few other types which it explicitly checks for with instanceof and which do not inherit from PopupBaseMenuItem and thus haven't type-checked so far. We mirror the checked types in an explicit enum. Same goes for firstMenuItem which the implementation also narrows down to two specific types via _getMenuItems()
For ESNext module resolution relative imports must have a file extension.
|
@schnz Happy new year as well 🎆 I've addressed your remarks 🙂 |
|
Thank you ❤️ Would you be so kind as to push this change to npm with a new version? 😇 |
|
Thank you for the PR! I had some trouble with figuring out a working The new version is available now :) ( |
|
Thanks 🙏🏿 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request addresses a few issues I stumbled upon while trying to move from my hand-written types to the types in this repo. It's by no means complete (there were more issues), but I'm stuck in the process due to some other issues (see #10), so I figured I'd open a merge request with what I already got.
Recommended review is commit by commit, as every commit addresses one problem.