Review of "Fixed IME List" version 10

Details Page Preview

Prevent MRU re-sorting of IME list on switching input method so it will always be the same sequence as you set in Settings.

Extension Homepage
https://github.com/AlynxZhou/gnome-shell-extension-fixed-ime-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

Version Status
11 Active
10 Rejected
9 Rejected
8 Rejected
7 Active
6 Active
5 Active
4 Active
3 Active
2 Rejected
1 Rejected

Previous Reviews on this Version

JustPerfection active
JustPerfection rejected
JustPerfection posted a review
Extensions modifying keyboard cannot use session modes: [Review Guidelines: Session Modes](https://gjs.guide/extensions/review-guidelines/review-guidelines.html#session-modes)
AlynxZhou posted a review
> All signals related to keyboard events MUST be disconnected in unlock-dialog session mode. I don't think this extension modifies keyboard events?
JustPerfection posted a review
Line 363-384 `extension.js` shouldn't be really happening in unlock dialog. and you already have the `_inputSourceManager` (the actual instance). What's the reason for using prototype in keybindings? I couldn't take a closer look at the GNOME Shell source code for this since the review queue is full these days but maybe by modifying the instance instead of prototype on enable, you won't need `reloadKeybindings()` at all? btw, the session mode comment should be in the disable function: > The disable() function MUST have a comment explaining why you are using unlock-dialog.
JustPerfection posted a review
btw, please add a more descriptive description. End users won't understand what this extension is doing by the current description.
AlynxZhou posted a review
> Line 363-384 `extension.js` shouldn't be really happening in unlock dialog. > > and you already have the `_inputSourceManager` (the actual instance). What's the reason for using prototype in keybindings? > I couldn't take a closer look at the GNOME Shell source code for this since the review queue is full these days but maybe by modifying the instance instead of prototype on enable, you won't need `reloadKeybindings()` at all? Nope, the original GNOME Shell code uses `.bind()` when making keybindings (<https://gitlab.gnome.org/GNOME/gnome-shell/-/blob/main/js/ui/status/keyboard.js?ref_type=heads#L357>), this will return a new function instead of using the function on prototype (<https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_objects/Function/bind>), so even if you injected the function on prototype, the keybindings still use the copy of the original function instead of the injected function, so the only way is after you injected the function, re-bind the keybinding to the current one, and after clearing injection, re-bind to the original one. The same reason applies to instance, function returned by `.bind()` is not referenced to the instance method, but more like a COPY, so even you modify the instance method, it still won't work as you think. To test this behavior you can use the following GJS script, no matter how you change the instance and prototype function, the bound function always prints `aaa`: ```JavaScript #!/usr/bin/env gjs class A { test() { console.log("aaa"); } } const a = new A(); a.test(); // => aaa const bound = a.test.bind(a); bound(); // => aaa A.prototype.test = function () { console.log("bbb"); } a.test(); // => bbb bound(); // => aaa a.test = function () { console.log("bbb"); } a.test(); // => bbb bound(); // => aaa ``` For this extension, if you don't do this, the original function will still always select the first IME in InputSourcePopup, which is not what users want because we use fixed IME list so the current IME is not always the first. Also, using InjectionManager on prototype is recommended (https://gjs.guide/extensions/topics/extension.html#injectionmanager). >> The disable() function MUST have a comment explaining why you are using unlock-dialog. > btw, please add a more descriptive description. End users won't understand what this extension is doing by the current description. Those could be easily solved by a new submit if we reach a consensus with the above topic.
JustPerfection posted a review
Ok. Please send the package with the description and comment fix.
AlynxZhou posted a review
Thanks, please check the new submit.