Review of "Extension List" version 39

Details Page Preview

Simple GNOME Shell extension manager in the top panel For support, please report any issues via the homepage link below.

Extension Homepage
https://github.com/tuberry/extension-list

No comments.

Diff Against

Files

Note: Binary files aren't shown on the web site. To see all files, please download the extension zipfile.

All Versions

Previous Reviews on this Version

grroot posted a review
Sorry, I forgot the DBusProxy class need `run_dispose` in (`fubar.js` line 107-109), like in gnome-shell https://gitlab.gnome.org/GNOME/gnome-shell/-/blob/817ae5069230820f8027e44400c76e5caab56521/js/ui/calendar.js#L269-L271
JustPerfection posted a review
unexport and null out the reference should be enough. no need to call `run_dispose()` for that.
grroot posted a review
`unexport` should be a method of `Gio.DBusInterfaceSkeleton/Gio.DBusExportedObject()` like (`fubar.js` line 94),`Gio.DBusProxy` doesn't have it. https://gjs-docs.gnome.org/gio20~2.0/gio.dbusinterfaceskeleton#method-unexport
JustPerfection posted a review
That's an old code. No need to call `run_dispose()` for that.
JustPerfection rejected
Just noticed `run_dispose()`. Extensions cannot use `run_dispose()` (line 21 `fubar.js`): > This function should only be called from object system implementations. [gjs-docs: run_dispose](https://gjs-docs.gnome.org/gobject20~2.0/gobject.object#method-run_dispose)
grroot posted a review
I noticed that `run_dispose()` is widely used in gnome-shell's code, If the use cases in GNOME Shell could be considered as `called from object system implementations`. Is that rule being violated here?
JustPerfection posted a review
Well, doesn't mean that's a good thing when it is used in GNOME Shell. You are already using `disconnectObject()` in your code which is considered the alternative and a better way of disconnecting the signals related to an object.
grroot posted a review
I don't think it's good practice, either. `destroy` and `disconnectObject` are sufficient in general, `run_dispose` here handles some corner/thorny cases like: https://gitlab.gnome.org/GNOME/gnome-shell/-/blob/448df5c58e7cd7ee6c2de1e0e57c3d7a64f5a0f2/js/ui/keyboard.js?page=3#L2002-L2004 Objects that don't have a destroy method but need to be explicitly destroyed?
JustPerfection posted a review
We allow `run_dispose()` in some special cases though. Should I look at any specific line of the code in this package?
grroot posted a review
Nope, It won't be called in the package, but might be used in other package. It's the rarely used fallback resort, putting them together is to simplify the logic. I'm ok to separate them if that's a problem.
JustPerfection posted a review
Yeah, please separate them so we know when you are using `run_dispose()`.
JustPerfection posted a review
btw, we've just added the `run_dispose()` to ego review guidelines: [EGO Review Guidelines: run_dispose](https://gjs.guide/extensions/review-guidelines/review-guidelines.html#extensions-should-not-force-dispose-a-gobject)