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

Wiki save dir: use PathChooser for selecting directory #3762

Merged
merged 4 commits into from Mar 16, 2018

Conversation

Projects
None yet
2 participants
@poire-z
Contributor

poire-z commented Mar 15, 2018

Use PathChooser (instead of InputDialog with keyboard to enter the directory path) to select a directory where to save Wikipedia EPUB to.
I didn't know PathChooser existed at the time I did it, so all this ugly directory path strings check stuff.
It's not perfect (because of PathChooser limitations: it does not display current dir path, it doesn't allow creating a directory), but it will do for this setting most people probably use only once.

@@ -146,7 +148,7 @@ function ReaderWikipedia:addToMainMenu(menu_items)
input = curr_languages,
input_hint = "en fr zh",
input_type = "text",
description = _("Enter one or more Wikipedia language codes (the 2 or 3 letters before .wikipedia.org), in the order you wish to see them available, separated by space(s) (example: en fr zh)\nFull list at https://en.wikipedia.org/wiki/List_of_Wikipedias"),
description = _("Enter one or more Wikipedia language codes (the 2 or 3 letters before .wikipedia.org), in the order you wish to see them available, separated by space (example: en fr zh)\nFull list at https://en.wikipedia.org/wiki/List_of_Wikipedias"),

This comment has been minimized.

@Frenzie

Frenzie Mar 15, 2018

Member

That would be "separated by a space" then. ;-)

@Frenzie

This comment has been minimized.

Member

Frenzie commented Mar 15, 2018

I typed my preferred Wikipedia save location? I guess the default must've been either perfect or close to it for me not to bat my eyes at that.

@poire-z

This comment has been minimized.

Contributor

poire-z commented Mar 15, 2018

You probably didn't type it as it suggested a nice default location (home_dir/Wikipedia), so you just had to confirm, and it created it for you.
It now doesn't do that anymore indeed... and you would happen to be on last book directory or its parent, and you would select your home dir - or have to cancel and go back to filemanager to create manually a Wikipedia/ directory.
But it feels lacking not to use PathChooser for selecting a dir.
Any better idea besides staying with previous InputDialog?

Like on this first menu item call when there is no wikipedia_save_dir setting yet, propose to use and create home_dir/Wikipedia ?

@Frenzie

This comment has been minimized.

Member

Frenzie commented Mar 15, 2018

Both scenarios make sense,[1] but as long as we're talking about a global save location I think it should probably just start in the home directory.

E.g., putting Wikipedia articles related to the book in the same folder is a bit like putting newspaper clippings in books.

@poire-z

This comment has been minimized.

Contributor

poire-z commented Mar 15, 2018

Both scenarios make sense,[1]

Which ? what [1] ? :)

putting Wikipedia articles related to the book in the same folder is a bit like putting newspaper clippings in books.

Most people would call that menu item only once, to choose their global save location, and never come back to it.
Having that PathChooser starting at current book was a little help for the use case in §2 of #3756 (comment) : if you're reading a wikipedia article you have filled in Wikipedia/Science/, you could just go to that menu item (where most people don't go anymore), and you would Hold on the Science subdir (3 taps, 1 hold), so that all the next wikipedia articles you save as EPUB from this article would go in that same Wikipedia/Science/ folder - a little help so you don't have to go fill them here later.

I don't know if it feels cumbersome and needs my little explanation above to make sense :) In which case, I can close this issue with no regret :)

@Frenzie

This comment has been minimized.

Member

Frenzie commented Mar 15, 2018

Which ? what [1] ? :)

The one before "E.g." >_>

@poire-z

This comment has been minimized.

Contributor

poire-z commented Mar 15, 2018

OK, we could start from home_dir.
We would lose the easy auto-creation of homedir/Wikipedia. So, does it need a first dialog when no wikipedia_save_dir has ever been set to propose creating it?

And may be another check box just below it: Save wikipedia EPUB in current book directory for that other use case (I won't use it, but it would help the wikipedia saving campaign auto-filling, and may be people reading a book could like having the wikipedia articles jumped-to-from-that-book filled alongside that book) ?

@Frenzie

This comment has been minimized.

Member

Frenzie commented Mar 15, 2018

Makes sense. :-)

Makes sense (with a bit of everything)
On first launch, propose to use ~/Wikipedia/.
Allow selecting an existing directory with PathChooser.
Add checkbox to save EPUB in current book directory
@poire-z

This comment has been minimized.

Contributor

poire-z commented Mar 16, 2018

Any more comment about this ?

if home_dir and lfs.attributes(home_dir, "mode") == "directory" then
local wikipedia_dir = home_dir.."/Wikipedia"
local text = _([[

This comment has been minimized.

@Frenzie

Frenzie Mar 16, 2018

Member

I assume the extra newline serves some purpose?

This comment has been minimized.

@poire-z

poire-z Mar 16, 2018

Contributor

(I had to do that too in coverbrowser's BookInfoManager:extractBooksInDirectory())
With:

blah = [[
some test
]]

The first \n is stripped, but not the last \n.
Keeping the last \n makes the text/code more readable by pushing the ]])... on next line, and somehow, I like the air it gives at top and bottom of the ConfirmBox, so I added one \n at start of string for this.
I took the [[ ]] from your readerhighlight's info_message_ocr_text from some days ago, where there is none of my \n trick. You may want to look how it looks, dunno if you intended to have different padding at top and bottom.

This comment has been minimized.

@Frenzie

Frenzie Mar 16, 2018

Member

If we want more padding it should be part of the widget, not hacked with newlines imo. Also, my bad. :-P

This comment has been minimized.

@poire-z

poire-z Mar 16, 2018

Contributor

I agree.
And the air is not even that needed, and no real need to add these to widgets.
The visual feeling depends on the length of the lines, as they are not justified.
When you have short lines, and so some free space on the right, it feels prettier when you add some space at top and bottom (the left being used by the image).
But you can't really know how short your lines will be on the different devices, so let's do as we did with no newlines and no additional padding.

local wikipedia_dir = home_dir.."/Wikipedia"
local text = _([[
Wikipedia articles can be saved as EPUB for more confortable reading.

This comment has been minimized.

@Frenzie

Frenzie Mar 16, 2018

Member

confortable is French :-P

(comfortable)

This comment has been minimized.

@Frenzie

Frenzie Mar 16, 2018

Member

Also as *an* EPUB?

poire-z added some commits Mar 16, 2018

@poire-z poire-z merged commit b46628c into koreader:master Mar 16, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@poire-z poire-z deleted the poire-z:wiki_save_dir branch Mar 16, 2018

},
{
{
text = _("Change (select directory by long-pressing"),

This comment has been minimized.

@Frenzie

Frenzie Mar 17, 2018

Member

Missing closing parenthesis after long-pressing.

This was referenced Mar 17, 2018

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