New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always close filemanger before open document #3935

Merged
merged 2 commits into from May 20, 2018

Conversation

Projects
None yet
3 participants
@robert00s
Contributor

robert00s commented May 7, 2018

@Frenzie

This comment has been minimized.

Member

Frenzie commented May 7, 2018

To be sure though, did you test various failure scenarios? Also I worry that unless you put it a little further down (closer to UIManager:show(reader))you'll just make showing the file manager in case of error take longer without a tangible benefit.

NB That's just a quickie; don't have time to really think about it atm and also I have a pretty heavy cold so my brain's not running at full speed. :-P

@robert00s

This comment has been minimized.

Contributor

robert00s commented May 10, 2018

I tested various scenarios. Open documents from history, opds catalog, from find a book, last document. I also try open damaged file. No problem. Everything looks like it works.

@Frenzie

This comment has been minimized.

Member

Frenzie commented May 10, 2018

Same for putting it after the potential showFileManager() calls? Shouldn't FileManager.instance:reinit() be faster than FileManager:showFiles()?

@robert00s

This comment has been minimized.

Contributor

robert00s commented May 10, 2018

Can you say something more? I don't really know what and where to change :)

@Frenzie

This comment has been minimized.

Member

Frenzie commented May 10, 2018

I mean somewhere underneath these lines instead of right above them:

if not document then
UIManager:show(InfoMessage:new{
text = _("No reader engine for this file or invalid file.")
})
self:showFileManager()
return
end
if document.is_locked then
logger.info("document is locked")
self._coroutine = coroutine.running() or self._coroutine
self:unlockDocumentWithPassword(document)
if coroutine.running() then
local unlock_success = coroutine.yield()
if not unlock_success then
self:showFileManager()
return
end
end
end

Otherwise you make these lines useless:

if FileManager.instance then
FileManager.instance:reinit(last_dir, last_file)
else
FileManager:showFiles(last_dir, last_file)
end

You're kind of making them useless anyway because now it'll be slower to open the FileManager from ReaderUI, but at least if ReaderUI actually loaded properly it's for a good cause. (The menu not being in sync issue you mentioned.)

But actually it should go after UIManager:show(reader) (unless there's a good reason for it to go before), so as not to hold up showing the reader with closing actions.

@poire-z

This comment has been minimized.

Contributor

poire-z commented May 20, 2018

I've had that on my kobo since yesterday: no problem noticed.
Anything preventing merging after this last change?

@robert00s robert00s merged commit 6a98dba into koreader:master May 20, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@Frenzie Frenzie added the bug label May 20, 2018

@robert00s robert00s deleted the robert00s:close_fm branch Aug 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment