Global variables will introduce performance penalty to LUAJIT. A better approach would be setting a return value variable in UIManager and return it in UIManager:run().

Done.

naming convention nitpick, restartKOReader should be restart_koreader.

Done.

better use UIManager:nextTick() instead magic number 0.1.

This is a copy-paste from original logic, I was also curious about the scheduleIn(0.1, ...).
If you also prefer to use nextTick, I am happy to do so.

A similar case happens in touchmenu.lua.

Yup, I specifically wrote nextTick to replace all the scheduleIn(0.1) hacks. There are still multiple scheduleIn(0.1) hacks that haven't been cleaned up yet.

Flushing setting on every widget close seems like a very expensive change. What's the reason for doing this?

If we do not do it here, most of the widgets won't receive onFlushSettings event without suspending device, because they will be closed before "FlushSettings" events have been fired in device.

Good catch. I was under the impression that it's doing broadcast. handleEvent is probably not too much overhead.

Would it be better to do flush settings first before the close event?

Reasonable. Though currently we do not have conflicts between CloseWidget and FlushSettings.

Yes, this change won't fix anything in the current code base. I am more worried about bugs introduced by future developers assuming flushsettings event will always happen before close.

I think assert is great for development and debugging, but bad for production run. If an error is recoverable, we should not crash the reader. Can we use something similar to dbg:guard to turn this and other asserts off in non-debug mode?

I think the answer is something like a logger.pcall (or logger.assert or util.assert whatever which uses pcall in the background) because even in debug mode I just exiting may not be the most helpful. Especially if you want to get some debugging output from a less technically inclined user.

On the other hand that might get complicated because then you'd be implementing all kinds of busted-like stuff (is_same, is_true, etc.).

We can print more context about the assertion before exit to help with the debug, but I think we should exit immediately after the print. Fail loudly will force developer to not ignore the bug.

Usually the assert should be good enough because it points out which line it's failing.

True, true.

It's indeed a culture of Google :). And in MS, because of the heritage from client software development, assert is usually disabled in production. But I still find the code quality in Google is much higher than in MS.
Personally, I really like this culture, e.g. no error is acceptable, even it's recoverable.
An unexpected situation means something is wrong: it may be recoverable, but the side effect is a more serious result.

Maybe I can add a dassert() function which triggers only when dbg is enabled.

C++ and C can achieve this easily with macro system. Unfortunately, Lua doesn't have macro. So I wrote dbg.guard to achieve a similar effect. You can use it to enforce assert in debug mode (see UIManager), in non-debug mode, it will be optimized to a no-op.

Yes, but I found it a little bit unnatural. Comparing the following two implementations,

function A:f(p1, p2)
end
dbg:guard(A, "f", function(self, p1, p2)
  assert(p1 > 0)
end)

function A:f(p1, p2)
  dbg:dassert(p1 > 0)
end

The second one seems much cleaner, and guard() can only check input parameters, but dassert() can do almost everything.

Yes, here is the tradeoff, dbg:guard will get optimized to nothing in non-debug mode. But dbg:dassert will still introduce runtime penalties because the boolean statements are still evaluated every time before getting passed to dassert. Keep in mind that LUAJIT really doesn't like branching ;)

Another issue with dassert is it will still crash the program if the boolean statement has a bug. For example, when p1 == nil.

In languages like C++/C, you can define dassert as a macro and have the compiler optimize it to nothing at compile time. Unfortunately, Lua doesn't have macro support.

So yes, there are reasons for why it looks weird :)

If you care about performance and safety, it's better to reimplement all the busted like helper methods like @Frenzie mentioned. i.e. do the following:

dbg:dassert_is_gt(p1, 0)

instead of

dbg:dassert(p1 > 0)

Then you can set dbg:dassert_is_gt to a noop (empty function) at module load time. The drawback is you will need to write more code ;)

Is there a reason that in some scripts it's RESULT and in others RETURN_VALUE?

Because RESULT was here before. Updated.

Btw, 1c25d0f

So the dialog should look like this:

Cancel Don't save Save

Done.
I do believe it's a very good suggestion to be more clear about what will happen once a button is clicked.

Should like is a rather formal way of saying would like and possibly mostly British. I'd stick with "If you want to reset the data" or "If you would like to reset the data".

Done.

If you're correcting these things anyway it should be something more like should scroll backward on the first page without crashing ;-)

Done.

See above.

Done.

Don't save

Done.