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

End of document action #3943

Merged
merged 5 commits into from May 13, 2018

Conversation

Projects
None yet
6 participants
@robert00s
Contributor

robert00s commented May 10, 2018

Close: #3937 #3545, #3405
Discussion: #3937
Screenshots:
romeo and juliet - koreader_088
romeo and juliet - koreader_089

Limitations: The "next file" option is not available when sorting: "sort by last read date".

elseif settings == "next_file" then
if G_reader_settings:readSetting("collate") ~= "access" then
local info = InfoMessage:new{
text = _("Searching next file..."),

This comment has been minimized.

@Frenzie

Frenzie May 10, 2018

Member

Real ellipsis

UIManager:close(info)
else
UIManager:show(InfoMessage:new{
text = _("Cannot open the next file. Unsupported sorting (sort by last read date)"),

This comment has been minimized.

@Frenzie

Frenzie May 10, 2018

Member

Unsupported sorting method

But I'm thinking maybe something more like:

Could not open next file. Sort by last read date does not support this feature.

function ReaderStatus:openFileBrowser()
local FileManager = require("apps/filemanager/filemanager")
if FileManager.instance == nil then

This comment has been minimized.

@Frenzie

Frenzie May 10, 2018

Member

Probably nicer to read if you write if not FileManager.instance then.

ReaderUI:showReader(next_file)
else
UIManager:show(InfoMessage:new{
text = _("Cannot open the next file. The last one in current folder was opened"),

This comment has been minimized.

@Frenzie

Frenzie May 10, 2018

Member

Maybe something phrased slightly more positively, like This is the last file in the current folder. No next file to open.?

local status_page = BookStatusWidget:new {
thumbnail = self.document:getCoverPageImage(),
props = self.document:getProps(),
document = self.document,
settings = self.settings,
view = self.view,
on_exit = on_exit_func,

This comment has been minimized.

@Frenzie

Frenzie May 10, 2018

Member

@poire-z @NiLuJe Any thoughts? I was thinking it might potentially make more sense to override/set ReaderStatus.onClose from the caller.

This comment has been minimized.

@poire-z

poire-z May 10, 2018

Contributor

I'm not really familiar with these readerstatus & bookstatuswidget, and I'm not quite sure what you mean. But being not familiar, the way it is written here is fine enough for me to get how it works like that.

This comment has been minimized.

@NiLuJe

NiLuJe May 10, 2018

Member

Yeah, I'll defer to people with actual lua knowledge on that one ;D.

This comment has been minimized.

@Frenzie

Frenzie May 11, 2018

Member

It's not about Lua, but about the architecture and structure of the program. I'm not talking about some one-off status widget thing but about how to always implement things like this in the future so the program has an internal logic and is easier to comprehend and maintain.

As is is I think it should be called on_exit_func for clarity, but we exit the program and close a widget.

But on that note, as written could this prevent the program from shutting down? As in, you try to shut down and get the file browser instead?

This comment has been minimized.

@robert00s

robert00s May 11, 2018

Contributor

This is used in option Book status and return to file browser. After finishing book we show book status and after close that widget we automatic go to file browser (execute on_exit_func).

This comment has been minimized.

@Frenzie

Frenzie May 11, 2018

Member

Yes, but what happens if you close the widget as part of exiting? For example, you press the home button your Pocketbook or the top-right X on your computer. Will it still exit or is it going to get stuck in the FileManager?

This comment has been minimized.

@robert00s

robert00s May 11, 2018

Contributor

Right.
I think better solution in Rev2

end,
},
{
text = _("Return to File Browser"),

This comment has been minimized.

@Frenzie

Frenzie May 10, 2018

Member

No need to capitalize

end,
},
{
text = _("Book status and return to File Browser"),

This comment has been minimized.

@Frenzie

Frenzie May 10, 2018

Member

No need to capitalize

text = _("Do nothing"),
checked_func = function()
return G_reader_settings:readSetting("end_document_action")
== "nothing"

This comment has been minimized.

@Frenzie

Frenzie May 10, 2018

Member

I'm not sure what I think of the legibility of this so also pinging @poire-z @NiLuJe ;-)

This comment has been minimized.

@poire-z

poire-z May 10, 2018

Contributor

Could use false, or "disable", as in:

text = _("Leave screen as it is"),
checked_func = function()
if screensaverType() == "disable" then
return true
else
return false
end
end,
callback = function()
G_reader_settings:saveSetting("screensaver_type", "disable")
end

This comment has been minimized.

@Frenzie

Frenzie May 10, 2018

Member

It would be ever so slightly faster I suppose but I was talking about the line split. :-P

This comment has been minimized.

@poire-z

poire-z May 10, 2018

Contributor

Oh, ok :) yes, that line starting with == is strange; could be made on one line (we have longer lines than that elsewhere).

This comment has been minimized.

@NiLuJe

NiLuJe May 10, 2018

Member

FWIW, I tend to go with 120 characters as a max width, since that matches GitHub's own code viewer ;). (And, also, the tiny screen on my laptop :D).

So, yeah, we could defiinitively avoid the split.

This comment has been minimized.

@Frenzie

Frenzie May 11, 2018

Member

Alright, please avoid the line splits. :-)

@KenMaltby

This comment has been minimized.

KenMaltby commented May 11, 2018

Cancel might be best as the last (lower right) selection in a message box. If the "Open next file" option would not work, could that option then be grayed out in the message box and menu.

@NiLuJe

This comment has been minimized.

Member

NiLuJe commented May 11, 2018

Good point on both fronts! (I'd swap FB with Cancel).

@Frenzie

This comment has been minimized.

Member

Frenzie commented May 11, 2018

Per our UX guidelines it should go on the bottom left for consistency: http://koreader.rocks/doc/modules/ui.widget.inputdialog.html

@KenMaltby

This comment has been minimized.

KenMaltby commented May 11, 2018

I stand corrected.

@NiLuJe

This comment has been minimized.

Member

NiLuJe commented May 11, 2018

Then I'd go:

L R
File browser Book status
Cancel Open next file
@Frenzie

This comment has been minimized.

Member

Frenzie commented May 11, 2018

@NiLuJe Is there a reasoning behind the order? But sure, fine by me.

@NiLuJe

This comment has been minimized.

Member

NiLuJe commented May 11, 2018

Vaguely putting the most common actions on top? And stuff that 'exits' on the left?

Granted, all of this is fairly subjective, so, no strong opinion on this ;).

@robert00s

This comment has been minimized.

Contributor

robert00s commented May 11, 2018

@KenMaltby This option is already implemented (Sort by last read date)
romeo and juliet - koreader_091
romeo and juliet - koreader_090

robert00s added some commits May 11, 2018

local status_page = BookStatusWidget:new {
thumbnail = self.document:getCoverPageImage(),
props = self.document:getProps(),
document = self.document,
settings = self.settings,
view = self.view,
}
if on_exit_func then
on_exit_func()
end
UIManager:show(status_page)

This comment has been minimized.

@poire-z

poire-z May 11, 2018

Contributor

Does that mean the file browser will be instantiated and shown before BookStatusWidget? and that BookStatusWidget will just cover it / being above it?
In which case, it's not more an exit_func :) and you could do the following directly, no?:

    elseif settings == "file_browser" then
        self:openFileBrowser()
    elseif settings == "book_status_file_browser" then
-       local on_exit_func = function() self:openFileBrowser() end
+       self:openFileBrowser()
        self:showStatus(on_exit_func)

This comment has been minimized.

@robert00s

robert00s May 11, 2018

Contributor

We cannot do that :)
In that case when we run self:showStatus after self:openFileBrowser() we get
attempt to index field 'document' (a nil value) because self.document is nil after run openFileBrowser

This comment has been minimized.

@poire-z

poire-z May 11, 2018

Contributor

OK, I see, so now, it's no more a on_exit_func, but a before_show_callback :)
(It also looks less natural than it did before)

@robert00s

This comment has been minimized.

Contributor

robert00s commented May 13, 2018

I think its ready.
Does something needs to be changed yet?

end,
},
{
text = _("File Browser"),

This comment has been minimized.

@Frenzie

Frenzie May 13, 2018

Member

Just lowercase "File browser".

Please merge after that. ;-)

@robert00s robert00s merged commit fbd549b into koreader:master May 13, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@cramoisi

This comment has been minimized.

Contributor

cramoisi commented May 23, 2018

Just to say the thing is damn perfect :-)

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