Skip to content
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

UI Changes #5508

Merged
merged 41 commits into from Oct 21, 2019
Merged

UI Changes #5508

merged 41 commits into from Oct 21, 2019

Conversation

@dontcrash
Copy link
Contributor

dontcrash commented Oct 20, 2019

Have made several changes to the UI to improve the experience and make it less "clunky" and more polished/refined, please let me know what you think

dontcrash added 5 commits Oct 20, 2019
KOReader looks nicer than KOReader File Browser,
Rather than having a files icon, use the home icon, it makes more sense, as users can consider the File Browser "home"
Removes it from the bottom of the file browser
Removes it from the top menu, if there is only one page, why show page 1 of 1
Since the file browser can be considered "Home"
frontend/ui/widget/menu.lua Outdated Show resolved Hide resolved
frontend/ui/widget/touchmenu.lua Outdated Show resolved Hide resolved
plugins/coverbrowser.koplugin/main.lua Outdated Show resolved Hide resolved
@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Oct 20, 2019

(Tested on my device.) I thought I'd mind most of the changes, but no :)
(Our file browser icon is a bit ugly (hard to recognise it shows files :) - so I don't mind having a house now.)

But regarding your comments on gitter:

I don’t believe people would have a problem with replacing ~ with Home, the people I have talked to about that, they only have that setting turned on because it’s unpleasant to look at /mnt/onboard/path, you have to remember who the target audience is for this, it’s not just programmer

As you see, I do have a problem with that (but solvable :) see my comment).
And you don't have to remember me/us who the target audience is. My primary target audience is... myself :) and the secondary audience probably mostly technical people (given that it needs a bit of skills to be installed on a device).
And I'm personally not interested in simplifying stuff to make it easier for that third audience you're talking about :)

a lot of people (myself included) love KOReader as it’s a better experience, but think that the UI is clunky, no offence, it works great but just visually, some things could be improved

No offence taken. But I think we have done most things right (and some really more cleverly than other software, even for non technical users) - the alternative being to kill or hide features and be limited just like the other readers software.
(I'm still persuaded people don't like our UI just because we use a Sans font, unlike Kobo Nickel which has a nice Serif font - but I like our Sans font.)

Also tweaked the charging icon, ⚡ looks nicer than +, tweaked seperator between time and battery, - instead of @
@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Oct 20, 2019

2 things noticed while re-testing:

The house icon replacing the filebrowser icon is not right: that icon takes you to File browser, but to the directory that contain the book you were reading. It does not take you to your home directory. And I don't want it to take me to my home directory: the current target directory is right.
So, a Home icon is confusing: the home icon at top left of file browser takes you to your home directory - having that same icon in reader take you elsewhere is confusing.

About the ~/ vs Home/: may be we could just show nothing when we are under the home directory.
So, we'll just see blank in home dir, and Wikipedia/Science/ when in a sub-directory, without a leading slash.
When in a directory outside the home directory, we'll see a leading slash /.
That way, no translation involved, and even shorter.
(If really needed, have a "Home" translatable string for when we're just in that home directory, to replace the blank just in that case.)

@Frenzie Frenzie added this to the 2019.11 milestone Oct 20, 2019
@Frenzie Frenzie added the UX label Oct 20, 2019
@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Oct 20, 2019

Gitter convo here.

@dontcrash

This comment has been minimized.

Copy link
Contributor Author

dontcrash commented Oct 21, 2019

Okay, I'm happy enough with just "Home", no need to get fancy and try and guess what the user wants the home text to be

Co-Authored-By: Frans de Jonge <fransdejonge@gmail.com>
@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented on 42138f6 Oct 21, 2019

That one's got a bunch of friends too.

Copy link
Member

Frenzie left a comment

Okay, maybe just one friend.

frontend/ui/widget/touchmenu.lua Outdated Show resolved Hide resolved
Co-Authored-By: Frans de Jonge <fransdejonge@gmail.com>
@dontcrash

This comment has been minimized.

Copy link
Contributor Author

dontcrash commented Oct 21, 2019

Okay, from here I think all the changes I wanted to make have been explored to their full potential, unless any further changes are needed I won't be sending any further commits to this branch :)

Co-Authored-By: Frans de Jonge <fransdejonge@gmail.com>
dontcrash and others added 6 commits Oct 21, 2019
Co-Authored-By: Frans de Jonge <fransdejonge@gmail.com>
@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Oct 21, 2019

Raah, I hate the code review comments autoclosing as resolved when one line change (and there's some many little code reviews that you may have missed it).
So, reposting it here.
You can make your 2 menu items' callbacks (that toggle a setting) one-line:

callback = function()
G_reader_settings:flipNilOrFalse("wikipedia_save_in_book_dir")
end,

(which would be flipNilOrTrue if you're making it default as true)

@dontcrash

This comment has been minimized.

Copy link
Contributor Author

dontcrash commented Oct 21, 2019

Raah, I hate the code review comments autoclosing as resolved when one line change (and there's some many little code reviews that you may have missed it).
So, reposting it here.
You can make your 2 menu items' callbacks (that toggle a setting) one-line:

callback = function()
G_reader_settings:flipNilOrFalse("wikipedia_save_in_book_dir")
end,

(which would be flipNilOrTrue if you're making it default as true)

I tried using flipNilOrTrue which resulted in it always being stuck on, are you sure this is right?

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Oct 21, 2019

You need to use the adequate check in the checked_func, eg:

text = _("Low battery alarm"),
sub_item_table = {
{
text = _("Enable"),
checked_func = function()
return G_reader_settings:nilOrTrue("battery_alarm")
end,
callback = function()
G_reader_settings:flipNilOrTrue("battery_alarm")

local len = home_dir:len()
local start = path:sub(1, len)
if start == home_dir then
return home_dir_name .. path:sub(len+1)
return path:sub(len+2)

This comment has been minimized.

Copy link
@poire-z

poire-z Oct 21, 2019

Contributor

I need to stop commenting, I need help

Don't stop :) Add one here -- remove leading / of sub-directory (because I will surely wonder why that +2 ?! next time I'll look at that code)

This comment has been minimized.

Copy link
@Frenzie

Frenzie Oct 21, 2019

Member

Like I said, I'm a commentophile. It's just that the particular one I called redundant was saying the same thing as the code almost verbatim.

-- Only do this if there's more than one thing.
if thing > 1 then

This comment has been minimized.

Copy link
@Frenzie

Frenzie Oct 21, 2019

Member

Instead of a comment for a mystery number you can also use a self-explanatory variable name.

local leading_slash_len = 2

Depends a bit on the specifics.

@Frenzie Frenzie merged commit ef22e85 into koreader:master Oct 21, 2019
1 check passed
1 check passed
ci/circleci: build Your tests passed on CircleCI!
Details
@ptrm

This comment has been minimized.

Copy link

ptrm commented Oct 21, 2019

Hooray for the charging icon 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.