Replace tweak_buttons_func with an event instead

[nairyosangha]

There is nothing really wrong with the current approach, considering it was added for one specific reason, giving the vocabbuilder plugin access to the popup dictionary.

I created a custom plugin which also adds a button to the popup dictionary, and up until now I didn't care about it overwriting what the vocabbuilder one does.

However, I got a request last week for it not to do that 🙂

I tried getting around making changes to koreader itself by wrapping the tweak_buttons_func so it still calls the original one, but also my own on top of it. This didn't work out quite right because each time the user switches between reader and filemanager all plugins get reloaded, meaning it forgets about the function already being wrapped (since DictQuickLookup itself persists the whole time), and it wraps it again, meaning I end up with one extra button on each reload.

On top of that, vocabbuilder also sets this function to nil again at some points (when the setting to automatically add all words looked up is turned on) which I can't detect at all currently

@NiLuJe suggested to make tweak_buttons_func a table instead where each plugin which wants to modify it can store their function in, that still didn't feel very clean to me, and it would mean we're still storing plugin state in the actual DictQuickLookup module directly itself which doesn't seem great to begin with.

I ended up creating a customt even instead, which plugins that care about which buttons appear in the popup dict can consume, it takes in the popup window itself, and the list of buttons. This list is modified in-place.
The upside of this is that you don't need to pass that tweak_buttons_func around anywhere in ReaderDictionary and no plugin state is stored in the persisting DictQuickLookup module

If people have suggestions for better ways to do this I would love to hear it. It would also be great if someone more familiar with vocabbuilder looks at it. I tested it and it seems to work fine but I'm not an active user so I might have missed something.

FYI this was discussed on Gitter first

---

This change is 

[Frenzie]

Seems sane enough to me.

[nairyosangha]

A downside of this approach is that when you click an item in the vocabulary builder window, it opens a new popup which fires off an event which then potentially needs to be consumed by multiple entries in that vocab window before it actually gets to the entry that was clicked on, while before you immediately know which entry was clicked on.
I don't know if this is enough of a downside to not do it like this

Also pinging @weijiuqiao for feedback on the PR in general

[nairyosangha]

Something weird is still going on, when closing the vocabbuilder widget it closes fine, but the items displayed in it seem to persist still, if you click in the correct place it will pop up the dictionary item for it. It's as if the vocabbuilder widget was visually closed but is actually still there

[poire-z]

If people have suggestions for better ways to do this I would love to hear it

I'm fine with this PR (I have just looked at the frontend part, which it makes cleaner, so nothing against - haven't looked at the plugin, not familiar with it).

Just mentionning we somehow have hooks to allow plugin to plug/tweak (may be only plug ? less flexible than getting the button to find the right place like vocabbuilder does), which are both quite similar:

(not used by any plugin, although I think I remember we made all that thinking about usage by plugins:)

koreader/frontend/apps/reader/modules/readerhighlight.lua

Lines 1060 to 1070 in 8211208

	function ReaderHighlight:addToHighlightDialog(idx, fn_button)
	-- fn_button is a function that takes the ReaderHighlight instance
	-- as argument, and returns a table describing the button to be added.
	self._highlight_buttons[idx] = fn_button
	end
	function ReaderHighlight:removeFromHighlightDialog(idx)
	local button = self._highlight_buttons[idx]
	self._highlight_buttons[idx] = nil
	return button
	end

(used by the wallabag plugin:)

koreader/frontend/apps/reader/modules/readerlink.lua

Lines 1496 to 1504 in 8211208

	function ReaderLink:addToExternalLinkDialog(idx, fn_button)
	self._external_link_buttons[idx] = fn_button
	end
	function ReaderLink:removeFromExternalLinkDialog(idx)
	local button = self._external_link_buttons[idx]
	self._external_link_buttons[idx] = nil
	return button
	end

So, you may give it some thoughts if what you'd like to achieve and the vocab plugin could use something similar instead - just for the sake of API consistency.
But not issue with this PR if that model doesn't fit.

that still didn't feel very clean to me, and it would mean we're still storing plugin state in the actual DictQuickLookup module directly itself which doesn't seem great to begin with.

Feels like these 2 snippets I mentionned are what you'd feel like not very clean ? :)

[Frenzie]

Feels like these 2 snippets I mentionned are what you'd feel like not very clean ? :)

I was going to reply I don't see that as problematic (because in my view it's not "plugin state" but "a button that should be there") but I'd forgotten about this particular function. Copying it would indeed be better if it fits this use case.

[nairyosangha]

Feels like these 2 snippets I mentionned are what you'd feel like not very clean ? :)

If it's already done like that in other places I'm fine with it 😄

It would work for the normal buttons (which is what I care about), but it wouldn't work for the special case vocabbuilder needs, where it modifies the existing buttons to use the popup dict as a sort of review window:

That's why that tweak_buttons_func needed to be added in ReaderDictionary too, in the other case it could count on the function that's been saved (on plugin startup) in the DictQuickLookup module directly, while here it needs to overwrite the function with the specific new buttons and with access to the item that is currently being reviewed (like it's due time, text, etc). This gets passed along in the DictQuickLookup instance instead

So if I would use something like you suggest, then I would still need to restore those deletions I did related to tweak_buttons_func in ReaderDictionary directly

[poire-z]

So if I would use something like you suggest, then I would still need to restore those deletions

I'm fine with these deletions :)
If that model doesn't really fit - as it seems you suggest - I'm fine with that, considering that it's better if it is different because there may be re-entrants/stacked widgets that need different handling and more checks and adapations than just pushing a button and an position idx.
Something explaining why we went that way could be written in a comment somewhere, for references.

[NiLuJe]

Something weird is still going on, when closing the vocabbuilder widget it closes fine, but the items displayed in it seem to persist still, if you click in the correct place it will pop up the dictionary item for it. It's as if the vocabbuilder widget was visually closed but is actually still there

That's... concerning ^^.

The smelly part might be those Close events being sent. Despite the confusing but appealing name, nobody and nothing should really ever be sending Close events. It has very very weird and specific semantics that nobody should really ever have to deal with. (e.g., it's generally broadcast as a means to close all the things. Close handlers are probably not expecting it not to be a broadcast, and might be stopping it in its track somewhere)

I'm not familiar with the plugin or what it's trying to do, but for that sort of stuff, we usually target the specific widget we want to close and call its actual usual close method (which is often, granted, onClose, buuut, might not :D).

[nairyosangha]

Something weird is still going on, when closing the vocabbuilder widget it closes fine, but the items displayed in it seem to persist still, if you click in the correct place it will pop up the dictionary item for it. It's as if the vocabbuilder widget was visually closed but is actually still there

That's... concerning ^^.

The smelly part might be those Close events being sent. Despite the confusing but appealing name, nobody and nothing should really ever be sending Close events. It has very very weird and specific semantics that nobody should really ever have to deal with. (e.g., it's generally broadcast as a means to close all the things. Close handlers are probably not expecting it not to be a broadcast, and might be stopping it in its track somewhere)

I'm not familiar with the plugin or what it's trying to do, but for that sort of stuff, we usually target the specific widget we want to close and call its actual usual close method (which is often, granted, onClose, buuut, might not :D).

It doesn't seem to be related to that, replacing them doesn't fix the issue at least.
It's definitely related to me inserting VocabularyBuilderWidget (the custom UI widget that's displayed by the plugin) in VocabBuilder (table of the actual plugin itself). When I don't do that, it closes fine, but then it doesn't propagate the events of course.

I spent way too long messing with it and it just doesn't wanna work, idk what I'm missing 😅

I ended up working around it instead

[weijiuqiao]

Apologies for being late.
I looked at the code and checked related functionalities. It all looks good to me. The code is also cleaner.
As to the smell, I believe it was my unwarrented hack to close the dictionary popup view without getting a reference to it. I was not familiar with the infrastructure of KOReader (and am even less so) so some intervention would best.

[weijiuqiao]

Oops, I tried looking up words from within the definition popup and my emulator freezes. It's alright when not in the plugin. I'm not sure how it would behave on a real device though.

[nairyosangha]

Oops, I tried looking up words from within the definition popup and my emulator freezes. It's alright when not in the plugin. I'm not sure how it would behave on a real device though.

Damn, that's what I get for trying to be smart with that iter function, it's was getting stuck in an infinite loop

I believe it was my unwarrented hack to close the dictionary popup view without getting a reference to it.

But you already do have a reference to it right? that's the obj parameter in the tweak_buttons_func. I've modified it now and it still seems to work fine

[NiLuJe]

But you already do have a reference to it right? that's the obj parameter in the tweak_buttons_func. I've modified it now and it still seems to work fine

Yup, that's exactly how I would have proposed doing that.

Small (and opinionated, so feel free to ignore) style nit: I tend to use this for that (because, C++/Java). But that's just me ;).

[NiLuJe]

Yup, that's exactly how I would have proposed doing that.

But to expand on my earlier "Close" is weird rant, while what you did would be perfect for a "sane"¹, modern widget, here, you'll want to explicitly call this:onClose() because it does... more stuff. (And that's what its TitleBar close_callback does, so I'll trust it's necessary stuff ;)).

---

That's because the event that UIManager:close sends is CloseWidget, not Close, so you'd be skipping a bit of the widget's teardown here.

[1]. Ideally, all widget teardown would be stuffed in the ClsoeWidget handler, and the Close handler should be essentially limited to calling UIManager:close(self). Alas, Close is weird, and very few widgets are actually designed that way, especially really old ones ;).

[nairyosangha]

But you already do have a reference to it right? that's the obj parameter in the tweak_buttons_func. I've modified it now and it still seems to work fine

Yup, that's exactly how I would have proposed doing that.

Small (and opinionated, so feel free to ignore) style nit: I tend to use this for that (because, C++/Java). But that's just me ;).

Is that how it's usually done? I wasn't really a fan of the obj but I left it since that's what it was called already before. obj doesn't really tell you what this thing is exactly, and I'm not sure this would be any better. I would've probably just called it popup_dict or something like that since that's what it is

But to expand on my earlier "Close" is weird rant, while what you did would be perfect for a "sane"¹, modern widget, here, you'll want to explicitly call this:onClose() because it does... more stuff. (And that's what its TitleBar close_callback does, so I'll trust it's necessary stuff ;)).

That's because the event that UIManager:close sends is CloseWidget, not Close, so you'd be skipping a bit of the widget's teardown here.

[1]. Ideally, all widget teardown would be stuffed in the CloseWidget handler, and the Close handler should be essentially limited to calling UIManager:close(self). Alas, Close is weird, and very few widgets are actually designed that way, especially really old ones ;).

I'm still a bit confused. Looking at where that 'Close' event is actually being generated it seems to be specifically for when KOReader gets completely turned off. Is that really what I wanna use here?

I would think that it's fine if it just closes the widget, there can still be state remaining since I might (and often do) show the widget again at a later point. The problem I was having here is that the widget didn't actually seem to be closed properly at all, I didn't see it anymore but I could still interact with it, like it was still there.

Also, I don't know what that TitleBar:onClose is supposed to be doing exactly, I don't see where that function is actually defined, I'm not seeing it in TitleBar itself nor in any of its super ''class'' widgets.

[nairyosangha]

For my own understanding, let's say I have a plugin. This plugin itself is a WidgetContainer (which seems to be the case for a lot of them.
The plugin itself contains some different sub widgets (also of type WidgetContainer), which get displayed at different times, sometimes multiple ones at the same time.

1. If I don't actively insert these sub-widgets in the plugin's table itself (which is how this container widget is supposed to be used), is there any point in this widget being a Container at all? It might as well be plain Widget right?
2. Considering these sub-widgets need to be displayed and closed again at different times, and probably completely destroyed when changing between reader and filemanager, (and maybe even something different still on shutdown?), it sounds like I would need to implement a bunch of different events on these sub-widgets for this to be possible. I've seen onShow, onClose, onCloseWidget, or in a custom way using UIManager directly. What is the proper way to go about this?

Sorry for all the questions, but whenever I try to interact with any UI related stuff I often lose a LOT of time just trying to get it to work, and usually IDK what I'm actually doing wrong, I would like to understand this stuff properly 😄

[NiLuJe]

I'm still a bit confused. Looking at where that 'Close' event is actually being generated it seems to be specifically for when KOReader gets completely turned off.

Yep.

Is that really what I wanna use here?

No, which was exactly my point ;).

Also, I don't know what that TitleBar:onClose is supposed to be doing exactly, I don't see where that function is actually defined, I'm not seeing it in TitleBar itself nor in any of its super ''class'' widgets.

I probably skipped some vital context in my earlier message ;D. I was looking at how DictQUickLookup itself was handling its own "close my things" codepaths, because as I hinted in my rants, it can be sort of... weird and very much not unified.
That was just to confirm that it essentially did widget:onClose() instead of UIManager:close(widget), which is roughly what the whole thing was meant to convey to you: switch to obj:onClose() ;).
(I then checked that its onClose method to confirm that it was indeed doing a whole lot of things rather than just calling UIManager:close).

Which, okay, might look like that's what would happened if you sent a Close event, but because of sendEvent vs. broadcastEvent shenanigans, might not actually be ;).

[NiLuJe]

If I don't actively insert these sub-widgets in the plugin's table itself (which is how this container widget is supposed to be used), is there any point in this widget being a Container at all? It might as well be plain Widget right?

There's two sides to this question:

1. semantics. We wouldn't be super happy if a Widget actually turned out to be full of non-trivial sub-widgets.
2. More importantly, the whole Event system. While a Widget will receive events, it will never propagate it to its sub-widgets. That's the whole point of WidgetContainer.

The whole thing means what you add or don't add to the WidgetContainer array sort or depends on whether they need to receive Events or not. IIRC, we generally don't bother to make a difference, and just chuck the whole tree in there.

Considering these sub-widgets need to be displayed and closed again at different times, and probably completely destroyed when changing between reader and filemanager, (and maybe even something different still on shutdown?), it sounds like I would need to implement a bunch of different events on these sub-widgets for this to be possible. I've seen onShow, onClose, onCloseWidget, or in a custom way using UIManager directly. What is the proper way to go about this?

onShow pretty much means what it says on the tin. It's called by UIManager:show(). Off the top of my head, there can be weird use-cases where the widget layout is only known at that point, so that might be a good place for a setDirty instead of in init (which pretty much has no semantic reason to ever call setDirty, but, again, legacy ;p).

onClose is extremely badly named, and, for all intents on purposes, is only called on KOReader exit. Its implementation should always pretty much be limited to something along the lines of UIManager:close(self), and should only ever be implemented for WidgetContainers that may be passed to UIManager:show. As my previous answer demonstrates, a lot of widget abuse this event for stuff that should only really be handled by onCloseWidget. (Again: probably because: terrible name).

onCloseWidget is the actual destructor for a widget, and is called by UIManager:close(). (essentially: ctor: init, display: onShow, dismiss & dtor: onCloseWidget)

The whole propagation system means that you generally show/close your top-level WidgetContainer, and then only handle those events where relevant (most likely also just the top-level container ;p. Assuming you don't have sticky pinned references straying in something that is not an instance member (e.g., storing refs in the module and not the instance), destroying the top-level widget container should lead to the whole tree being marked for garbage collection).

[nairyosangha]

I'm still a bit confused. Looking at where that 'Close' event is actually being generated it seems to be specifically for when KOReader gets completely turned off.

Yep.

Is that really what I wanna use here?

No, which was exactly my point ;).

Also, I don't know what that TitleBar:onClose is supposed to be doing exactly, I don't see where that function is actually defined, I'm not seeing it in TitleBar itself nor in any of its super ''class'' widgets.

I probably skipped some vital context in my earlier message ;D. I was looking at how DictQUickLookup itself was handling its own "close my things" codepaths, because as I hinted in my rants, it can be sort of... weird and very much not unified. That was just to confirm that it essentially did widget:onClose() instead of UIManager:close(widget), which is roughly what the whole thing was meant to convey to you: switch to obj:onClose() ;). (I then checked that its onClose method to confirm that it was indeed doing a whole lot of things rather than just calling UIManager:close).

Which, okay, might look like that's what would happened if you sent a Close event, but because of sendEvent vs. broadcastEvent shenanigans, might not actually be ;).

Aha, makes sense now, I'll change it to onClose then. But in general it's still best to call UIManager:close if I wanna close a visible widget?

onClose is extremely badly named, and, for all intents on purposes, is only called on KOReader exit. Its implementation should always pretty much be limited to something along the lines of UIManager:close(self), and should only ever be implemented for WidgetContainers that may be passed to UIManager:show

Why should it be implemented for these?

Thanks a lot for the write-up btw, that cleared things up for me.

I went ahead and replace self:onClose() calls with UIManager:close(self) everywhere, assuming this is actually good practice. If not, I'll just roll it back.
I did notice that I can't just get rid of the actual onClose() implementations, because they're being called by these key_events:

koreader/plugins/vocabbuilder.koplugin/main.lua

Line 116 in f0a3bcf

self.key_events.Close = { { Device.input.group.Back } }

Is this another legacy thing? 😄

While doing that I also figured out what the issue was here #11777 (comment)

I guess I was assuming that closing a widget did more than it actually does. Closing VocabularyBuilderWidget doesn't automatically clear all these InputContainer instances inside VocabularyBuilderWidget.main_content. So after the widget is closed, clicking anywhere on the screen would now forward these events to these items in main_content - because I added this widget in the plugin's table itself - meaning they were now getting events they weren't before.
Clearing that main_content list fixes the issue. It's not doing any extra work now either, since we were already reloading that list each time the widget is displayed anyway.

So with that problem solved I could go back to my initial implementation, which actually works properly now

[NiLuJe]

Aha, makes sense now, I'll change it to onClose then.

Yep!

But in general it's still best to call UIManager:close if I wanna close a visible widget?

Ideally, yes. In practice, rarely, unless you're actually working from within the widget in question itself.

I went ahead and replace self:onClose() calls with UIManager:close(self) everywhere

insert whilhelm scream here :D. See my first answer? :D.

---

Why should it be implemented for these?

Because you can only close things that have been show'n ;).

Is this another legacy thing? 😄

There's a reason for the nested arrays. I can't recall what it is exactly, but it makes sense, I think :D.

EDIT: Okay, yeah, it's to differentiate between exact matches and "match anything found in this table". See InputContainer's docs.

[NiLuJe]

Clearing that main_content

WidgetContainer has a clear method for doing funky stuff like that.

(Which, incidentally, VocabBuilder already makes use of on that very array... Hmm).

[nairyosangha]

Aha, makes sense now, I'll change it to onClose then.

Yep!

But in general it's still best to call UIManager:close if I wanna close a visible widget?

Ideally, yes. In practice, rarely, unless you're actually working from within the widget in question itself.

I went ahead and replace self:onClose() calls with UIManager:close(self) everywhere

insert whilhelm scream here :D. See my first answer? :D.

Dammit, I figured it'd be okay as long as I keep the onClose() one only for DictQuickLookup, it seems to work fine 😅 I'll change it back for all these 'building block' widgets then and only keep it for widgets created by the vocabbuilder plugin itself.

Why should it be implemented for these?

Because you can only close things that have been show'n ;).

Makes sense, but can't you close things just fine with the closeWidget one instead? Although that doesn't actually do the UIManager:close call, I didn't check if that also automatically happens at shutdown/restart, I assume so.

Is this another legacy thing? 😄

There's a reason for the nested arrays. I can't recall what it is exactly, but it makes sense, I think :D.

EDIT: Okay, yeah, it's to differentiate between exact matches and "match anything found in this table". See InputContainer's docs.

I'm not talking about the nested array, but about the fact it's using Close as a key, which forces me to implement onClose anyway, even though I don't actually need this to be called on shutdown/restart but when I tell it to close the widget. Not a big deal of course but it does make it kinda confusing to know when to use what 😄

[nairyosangha]

Clearing that main_content

WidgetContainer has a clear method for doing funky stuff like that.

(Which, incidentally, VocabBuilder already makes use of on that very array... Hmm).

Ah nice, yeah it seems like it uses it already but only on init, not on destruction when I actually need it now

[nairyosangha]

Dammit, I figured it'd be okay as long as I keep the onClose() one only for DictQuickLookup, it seems to work fine 😅 I'll change it back for all these 'building block' widgets then and only keep it for widgets created by the vocabbuilder plugin itself.

Hmm... but that's actually all the stuff I replaced, it's all for container widgets created in the plugin code itself, so it should be safe to use UIManager:close instead of self:onClose(), no?

[NiLuJe]

but about the fact it's using Close as a key

Oh, right, mapping the "Back" group of keys to onClose happens a fair bit, yeah, I'd forgotten about that.

So, yeah, you do need an implementation for those cases; and again: confusing, definitely ^^.

Dammit, I figured it'd be okay as long as I keep the onClose() one only for DictQuickLookup, it seems to work fine 😅 I'll change it back for all these 'building block' widgets then and only keep it for widgets created by the vocabbuilder plugin itself.

Hmm... but that's actually all the stuff I replaced, it's all for container widgets created in the plugin code itself, so it should be safe to use UIManager:close instead of self:onClose(), no?

I stopped checking after a while, but you'll notice that all of these classes happen to do extra things in their onClose handler (e.g., the close_callback stuff, at least).

So you'd be skipping all that stuff by using UIManager:close(widget) instead of widget:onClose() since UIManager:close does not send a Close event.

Makes sense, but can't you close things just fine with the closeWidget one instead? Although that doesn't actually do the UIManager:close call, I didn't check if that also automatically happens at shutdown/restart, I assume so.

Yeah, CloseWidget is sent by UIManager:close, so it's the other way around, it's sent after a close ;).

[nairyosangha]

I stopped checking after a while, but you'll notice that all of these classes happen to do extra things in their onClose handler (e.g., the close_callback stuff, at least).

So you'd be skipping all that stuff by using UIManager:close(widget) instead of widget:onClose() since UIManager:close does not send a Close event.

Indeed, that's why I moved that stuff to closeWidget instead, so it would still be called. Because of that I think it'd be safe to do. However, since I need to keep those onClose() calls around anyway so the key bindings still work, I might as well not touch that stuff at all, it is kinda out of scope here anyway.

I did notice another small thing still, because the VocabularyBuilderWidget actually still persists after calling onClose(), it was receiving (and consuming!) key events that were supposed to go to the filemanager/readerui. I fixed it by just not returning true, although I'm not sure if that stuff really needs to remain after we were closed, but changing that will probably result in another bunch of changes (and potential breakage) again so I'd rather leave it alone.

[NiLuJe]

I'm not sure if that stuff really needs to remain after we were closed

Yeah, that's... weird. I'm unfamiliar with this widget in particular, but a widget instance really ought to be gone after a close.

[NiLuJe]

Yeah, that's... weird. I'm unfamiliar with this widget in particular, but a widget instance really ought to be gone after a close.

Eeh, because it's recycled by VocabBuilder, so there's still a live ref via self.widget in that.

[nairyosangha]

Ok, I gave it yet another go, making sure that VocabularyBuilderWidget is gone once we actually close it, I also moved some of that callback stuff around because it wasn't that easy to read (imo) and it felt like it belonged in that VocabularyBuilderWidget 'class' directly instead of being inserted on init as callbacks

Tested it a bunch and it all seems okay still.
I realize this is pretty out of scope here so idk if I should be touching this stuff 😄

[NiLuJe]

Haven't looked at it too deeply (and am not familiar with it to begin with), but at a glance that looks more like what I would have expected something like that to look like, so, +1 ;).

[poire-z]

Fine with the core changes.
(Did not review the plugin parts, not familiar with it.)