Review of "IBus Tweaker" version 37

Details Page Preview

Tweaker of IBus for theme, font, input mode and clipboard history For support, please report any issues via the homepage link below.

Extension Homepage
https://github.com/tuberry/ibus-tweaker

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
Really? 30ms to lock screen or close lid sounds a little hard to me.
JustPerfection posted a review
We had a discussion about your extension in the extensions room: GNOME Matrix channel: https://matrix.to/#/#extensions:gnome.org IRC Bridge: irc://irc.gimpnet.org/shell-extensions You can join the room and read that discussion. Basically you can hold all timeout ids in an array and remove them on disable() or you can check if the extension is enabled on the callback.
JustPerfection rejected
1. Please move line 18, 29, 30, 31 (extension.js ) to enable(): https://gjs.guide/extensions/review-guidelines/review-guidelines.html#only-use-init-for-initialization 2. You should also remove timeouts returning GLib.SOURCE_REMOVE (like extension.js 593) on disable(): https://gjs.guide/extensions/review-guidelines/review-guidelines.html#remove-main-loop-sources
grroot posted a review
It is only 30ms. The only result should be 'source id xxxx ... removed' in log if removing manually.
JustPerfection posted a review
It's for the security reason. We don't want the timeout get triggered in lock screen. While the timeout is very short, still can happen in lock.
grroot posted a review
Thanks for reply. My focus is not how to do, but whether it is necessary to do. Also, what is the focus of the discussion that I need to pay attention to? Why a jsObject is allowed in the global scope but not a GObject?
JustPerfection posted a review
Creating js object isn't allowed in the global scope too (still depends). Also the answers for those questions are already in the extensions room with more detailed explanations. Please join the extensions room. We have so many friendly people there.
grroot posted a review
I understand the effort to keep the extensions from tainting the gnome shell itself, but I don't mind using some global objects just to write a few lines less, after all they are part of the gnome shell I use myself and there are no taints for me. I post them here only to share them with people who need them, there is no subjective malice in them, only regret that objective malice prevents them from meeting the review guidelines. Thank you for your patience and time in reviewing and responding.
andyholmes posted a review
Hi, I think I can help clear up some things since this was brought up in the channel. The guideline about no objects in global scope or `init()` amounts to "Don't use any unnecessary memory or CPU when your extension is disabled". Since there is no `uninit()` hook or a way for GNOME Shell to "unload" JavaScript, uninstalling an extension does the same thing as disabling it. So even if a user uninstalls an extension, they will have to wait until the next session before it's actually freed, especially because of how SpiderMonkey allocates memory in batches/chunks. The difference between JS Objects and GObjects depends on the type of object. Just by creating a GSettings object you will be spawning threads, setting up file monitors, or whatever the storage backend uses. It will also continue to emit signals and add events to main loop in some situations. An object like GIcon will do nothing and use so little memory it doesn't matter, but the cost of recreating it is just as small and the more often you do it the more likely it is to be JIT compiled. So while there may be some tiny benefit to creating the objects only once in some cases, a general guidelines of "don't do that" is the better policy. In the case of timeout sources, you'll have to remember that the GLib event loop dispatches functions based on their priority. So a call like `GLib.timeout_add(GLib.PRIORITIY_DEFAULT, 30, timeout_cb)` actually means "wait at least 30ms before dispatching" not "dispatch at 30ms". You could easily delay this callback indefinitely by calling `GLib.idle_add(GLib.PRIORITY_DEFAULT + 1, () => { return true; })`, but more realistically ending or locking a session will queue many events in the main loop with a higher priority than your timeout. Another option we discussed in channel is using a top-level boolean like `isEnabled` and using that like an `if (isEnabled)` guard, so that your callback doesn't touch any code after the extension is disabled. I hope that clears some things up :)
grroot posted a review
I don't mind extensions taking up some tiny amount of resources while idle, unless it causes the gnome shell to crash or a memory leak. One possibility I now see for this to cause a memory leak is that the user repeatedly installs and uninstalls these extensions infinitely many times in a session (infinitely close to 0 possibility), sorry they are designed to be used and not uninstalled for a long time. I assume that the user locks the screen via keyboard or power button, which for most humans takes at least 50ms, meaning that only a sufficient number of tasks with higher priority (not emitted by ending the session) blocking this task for more than 20ms could eventually cause the callback to trigger unexpectedly (which is also extremely unlikely). Manually removing the resource anyway will definitely consume more resources when it is used. So I choose to take the risk of a very small probability and discard the very large probability of garbage code. Thank you for your continued help and detailed explanation, now I understand the reason for the rejections. Will deal with them if I change my mind. :)