Noticed while testing #11122 (as it effectively allows you to dismiss the "Disabled input" notification early, and I wanted to check that it didn't screw up the expected state after that).

Mostly harmless in practice, except for InfoMessage with a dismiss_callback, as it would have effectively ran twice.

Scratch that, the callback kills itself on exec ;o).

So, mostly harmless, except for confusing logs ;p.

Mostly harmless in practice, except for InfoMessage with a dismiss_callback, as it would have effectively ran twice.

Scratch that, the callback kills itself on exec ;o).

Methinks it should simply attempt to run it onCloseWidget instead of peppering it all over the places that will ultimately end up in there anyway ;p.

Methinks it should simply attempt to run it onCloseWidget instead of peppering it all over the places that will ultimately end up in there anyway ;p.

Trapper may have timing constraints expecting the current behavior, though, so I'll let @poire-z think about that one ;p.

(I'm relatively confident it would be mostly harmless, CloseWidget is sent extremely early in UIManager:close).

Trapper may have timing constraints expecting the current behavior, though, so I'll let @poire-z think about that one ;p.

Looks like no change to me :) I don't see how these changes could affect anything.
As for Trapper, quickly looking, it doesn't provide a timeout to its InfoMessages, so it feels non a-in-fected.

Looks like no change to me :)

Yeah, because they were theoretical ^^.

See 160840a for what I meant ;).

Ok :) Well, I don't see (haven't tested though) how this moving things around could have any impact on Trapper. Just save some Wikipedia as epub with images, or refresh cached book info, and tap at some point to check all is good.

Also, may be do the unschedule first/early before calling any dismiss_callback?

Also, may be do the unschedule first/early before calling any dismiss_callback?

I had a similar thought last night, but since we're still inside the same UI frame, I don't think there's a way it could matter? What did you have in mind?

Just save some Wikipedia as epub with images

Scrolling a "full" Wikipedia lookup with images seems to behave, as does cancelling the high-res asset download on long-pressing an image.

I had a similar thought last night, but since we're still inside the same UI frame, I don't think there's a way it could matter? What did you have in mind?

Nothing specific. I also had the same thought as it all being in the same "frame" (that is: only this runs sequentially, nothing else can happen in between), but just for visual consitency :) and so we don't have to wonder if anything can happen in a dismiss_callback that would take a long time.

Scrolling a "full" Wikipedia lookup with images seems to behave, as does cancelling the high-res asset download on long-pressing an image.

The things in Trapper that use the InfoMesage are the dialog driven parts, not used but what you tried, but used by the things I mentionned :)

The things in Trapper that use the InfoMesage are the dialog driven parts, not used but what you tried, but used by the things I mentionned :)

Oh, the actual download process, now I remember ;p.

Oh, the actual download process, now I remember ;p.

Interrupting the download works inasmuch as I get the dialog with the choice to Continue or Abort, but choosing Continue always leads to (immediate) failure: "Saving Wikipedia article failed or interrupted" (without the previous cleanup notice that shows up for abort).

On the "upside", this also happens on master, so, yay? :D

On the "upside", this also happens on master, so, yay? :D

Oh, sh*t .... what broke that? :/

11/19/23-19:04:42 WARN Wikipedia.createEpub pcall: false frontend/ui/widget/textboxwidget.lua:1150: attempt to index field '_bb' (a nil value)

It was already broken in v2022.07. And in v2021.06. And in v2020.06. And in v2020.02-12. (I don't have any nightly older that than.)

It was already broken in v2022.07.

I don't think I've tested interrupting in… quite a while.

This avoids the crash - but have the dismiss stuff work only once:

--- a/frontend/ui/trapper.lua
+++ b/frontend/ui/trapper.lua
@@ -172,24 +172,25 @@ function Trapper:info(text, fast_refresh)
             UIManager:show(abort_box)
             -- no need to forceRePaint, UIManager will do it when we yield()
             go_on = coroutine.yield() -- abort_box ok/cancel from their coroutine.resume()
             UIManager:close(abort_box)
             if not go_on then
                 UIManager:close(self.current_widget)
                 UIManager:forceRePaint()
                 return false
             end
             if self.current_widget then
                 -- Re-show current widget that was dismissed
                 -- (this is fine for our simple InfoMessage)
+                self.current_widget:init()
                 UIManager:show(self.current_widget)
             end
             UIManager:forceRePaint()
         end
         -- go_on_func returned result = true, or abort_box did not abort:
         -- continue processing
     end

(self.current_widget = InfoMessage:new is below the above)

I guess the comment above the line I added is no longer true :) and since then, InfoMessage, when dismissed, has been properly freed on close - and just showing again that dead one failed.
We could recreate it, or add a method to InfoMessage to resurrect itself (with dismiss_callback not nil'ujified)...
But as all this is totally related to what this PR is about (even if Trapper was a bit hacky and taking shorcuts, but that's his spirit), I'll let @NiLuJe think about how to make all that work in the spirit of this PR :)

Yeah, I was assuming it was due to lifecycle fixes ;).

I'm not familiar with the code, but it's probably easier to just sniff the last .text in a dismiss_callback wrapper, and then just spawning a brand new widget? That gets tricky if we care about the dismiss_callback, too, which it seems we do? ^^.

(Or do something awful that returns a new widget instance based on the current one... somewhere. e.g., InfoMessage:new(old_infomessage))

Or just add another flag to InfoMessage, .do_not_cleanup (or something else), so we avoid all the automatic lifecycle management?
A bit hard to grasp the different cases the trapper code should handle, but I think it may need much state of the previous one (movable offset, and its size maybe, I remember that we may start small, but if the text is long and needs 2 or 3 lines, it gets taller and keeps that height even if the next text is small) - so it might be easier to just reuse the same instance like it did.

Or just add another flag to InfoMessage, .do_not_cleanup (or something else), so we avoid all the automatic lifecycle management?

Hmm, yeah, Trapper holds a reference to it.

I'll see if moving the "destructive" cleanup to a separate method and simply NOP-ing (or tweaking) that one for Trapper widgets does the job.

Last commit seems to fix the Wikipedia case, but I wouldn't bet money on everything being quite up to snuff ;p.

Too late for me to, so please check how it does with "Refresh cached book info" on a big dir where you'll get some long filenames that will make the InfoMessage grow to 2 lines, and how it is after you interrupt and "Continue" it.
I think at the time, it kept being fat - but I don't mind if it doesn't with interrupt/continue. The reason for the growth might have been a repaint/refresh issue (so it doesn't get small and we don't refresh/repaint the previous larger area).

so please check how it does with "Refresh cached book info" on a big dir where you'll get some long filenames that will make the InfoMessage grow to 2 lines, and how it is after you interrupt and "Continue" it.

Hard to tell as I don't really have long filenames (hai, calibre ;p), but at least it works and doesn't do something obviously broken ;).

AFAICT, interrupting a larger than one line IM does resume a properly sized (i.e., larger) IM.

Tested this PR on my Kobo, and for the Wikipedia Save as EPUB and Refresh cached book info, it behaves as I remember it did. So, good for me, thanks :)