search

koreader / koreader

An ebook reader application supporting PDF, DjVu, EPUB, FB2 and many more formats, running on Cervantes, Kindle, Kobo, PocketBook and Android devices
http://koreader.rocks/
GNU Affero General Public License v3.0
16.83k stars 1.26k forks source link

Advanced settings are not saved on exit from opened book #5418

Closed derungs closed 5 years ago

derungs commented 5 years ago

Issue

Modified advanced settings are not stored when you exit koreader from the reader, that is, from an open book. When you exit from the file manager, the save dialog pops up and the advanced settings are stored.

Steps to reproduce

Observed: The advanced setting has the old value. Expected: The advanced setting has the new value.

Frenzie commented 5 years ago

Makes sense (or rather, there's only a check for that in the filemanager). The entire workflow for saving advanced settings is pretty weird imho.

Rather than porting the weirdness to the reader I think it should just ask to save on closing the widget, or something along those lines.

derungs commented 5 years ago

I could move the "save/don't save" dialog to filemanagersetdefault, and pass the "real" close function as a callback rather than the "save/don't save" dialog function. That would to some degree unweird the save workflow.

Frenzie commented 5 years ago

Sounds good to me. :+1:

derungs commented 5 years ago

On it, the idea seems to work (once I fixed my rookie mistake with lambdas).

NiLuJe commented 5 years ago

Here's me eating humble pie for saying "every sane exit workflow is wrapped in that check" just yesterday :D.

Hmm, pie! :cake:

derungs commented 5 years ago

Two questions:

Frenzie commented 5 years ago

ReaderMenu seems to use lfs without explicitly setting it. Do I miss something or should I just require lfs at the top of the file?

It's not a pure Lua module but a library, so it doesn't have any functional consequences. If you think it should be enforced as a code style then you can remove the exception from .luacheckrc. The current situation is probably primarily a case of harmless legacy and less a case of purposeful consideration.

What does OTAManager do? It calls exitOrRestart, but I haven't discovered a way to trigger it.

OTA updates are only for embedded devices. On Android it merely starts the download of a newer package if available.