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
[Status bar] Major UI makeover #11678
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please no Complications.
I do not like "complication". Neutral "items" works for me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard to review with the reorganization and stuff moved around, so I may have missed little changes, even rewording.
But testing and reading menu items, these are the ones I did notice - meaning I'm probably fine with all the other rewordings. Except of course with "Complication" that I don't want to see anywhere (and not in the code).
I probably also did not check the added checked_func, hard to see if these were new or added - and not keen on hunting for settings and confirming the logic, so trusting you.
separator = separator ~= "" and separator or "none" | ||
return T(_("Item separator: %1"), separator) | ||
end, | ||
text = _("Maximum lenght for text complications"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling error: length
Also multiple times below, so please recheck everywhere for this common typo of yours :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that option is still part of the formatting of the text, much like the one before. but I am happy to move the separator if formal request is put forward.
you are right, all three have the same typo. I copied/pasted them so that is probably why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any feedback about the reasoning for the still there separator in yellow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, i've been waiting for you to reply to my reply. 😅
Also if you could help me understand this interesting fact about french dashes and underscores:
tiret du 6 vs., tiret du 8
6 or 8 what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is "divider" to name horizontal lines approved?
|
It's indeed a situation where it should probably be all separator or all divider but keep in mind I'm just quickly typing this on my phone, so I don't have much context. |
In this PR "divider" is a horizontal line dividing the status bar, "separator" is a vertical line separating status bar items. We do not have any more "separators" in the upper menu. |
what do you propose? |
don't trust me. go and find it and put it to the test. the There is nothing new, just the |
the word 'separate' is usually used to indicate physically removing something, as in 'separate the wheat from the chaff' or 'the fat was separated from the lean cuts'. 'divider' is used to break something up into smaller pieces, a whole into parts. all that to say, I think 'divider' is better suited in this context than 'separator'. I also could not help noticing the use of the word 'items' there, to mean: 'bookmarked passages' I guess... mmm. Do you see what I mean? there are also items in the file browser somewhere to mean something else entirely. |
That was a polite way to say: "I don't want to spend time understanding the logic and test it" :), and to invite others who care to do so, and you to double check. So, do you mean you just added ONE |
That is exactly what i mean, everything else was already there. i also added an extra condition (to grey them out when progress bar not selected) to existing |
Yeah, I don't think horizontal or vertical matters.
If there ever was anything to this argument it was back in 1970. Microsoft and Apple have both called this concept That's not an objection to divider, only to that argument, although of course it's hardly a ringing endorsement either. ;-) It might be worth noting that Apple uses Edit: but in short, |
is that for the end user or programmers? remember, this is the UI here; normal people don't speak programmer talk. my Mac returns only one result for Nevertheless I don't have a strict opinion, just saying people wouldn't normally use that word in that sense in normal speech, so if you lads want it back to |
Yes. [Edit: that's a programmer joke to be clear, but also true.] Here's the first search showing how to add or remove them: https://www.youtube.com/watch?v=Kvlm4acVTWM
Apple has recently decided that spaces and dynamic spaces look cleaner than separators, so that's all you'll find in toolbar customization these days, even though they still have separators for their own use. (I'm inclined to agree with Apple but that's a separate discussion.) It's the standard user-facing term. Shown below are standard toolbar elements as they've existed for decades. Using visuals is "recent"; I'd say 30 years ago in Windows 9x this stuff was only textual. A statusbar is a toolbar that doesn't have buttons but only indicators. A "real" toolbar can contain both. (You can see a difference here between Apple using "dynamic space" and just about everybody else using "flexible space," but "expanding space" also exists.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please no complications.
Also a screenshot showing current and new would be awesome.
it is a bit difficult for me, as I run this on a kindle 4 and for whatever reason, koreader doesn't capture screenshots there. this is one of the reasons I wrote this on the description
honestly, it will make a lot more sense this way. |
Question to all, how do I get one of those nice macOS KOReader emulators running on my laptop? it would make my life a lot easier. |
Going to look at it tomorrow ;). In the meantime, making sure it passes the linter would be a good start ;).
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whitespaces
Which means we now get to my most favorite test in the suite...
|
Few questions: what test is this? Why is it your favourite? And more importantly, pardon my french, what the h*ll is going on and how do we fix it? |
all_at_once = _("Show all at once"), | ||
reclaim_height = _("Reclaim bar height from bottom margin"), | ||
all_at_once = _("Show all selected items at once"), | ||
reclaim_height = _("Overlap status bar"), | ||
bookmark_count = T(_("Bookmark count (%1)"), symbol_prefix[symbol].bookmark_count), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we change to "Annotation count" what will be the abbreviation, instead of "BM"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this is in reference to #11822, and you are asking about status bar showing letters instead of icons. the title here would then become "Annotation count" (I guess) so... AC?? is that already used? AN?
does anyone here actually uses the letters instead of icons?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's for you to decide and fix (c.f., the symbol_prefix.letters
table at the top of the file) ;).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are not changing that here (or at least I am not going to), that is a task for another PR as there are a lot more instances of "Bookmarks" elsewhere that would need changing. one step at a time please
Sarcasm, I doubt I'll have time to check until at least Sunday though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing much to add besides that initial nit.
Will give it a look see in practice on the emu, and another pass at the raw diff to avoid the comment distraction ;).
Reviewed 1 of 1 files at r13, 1 of 1 files at r28, all commit messages.
Reviewable status: all files reviewed, 81 unresolved discussions (waiting on @Commodore64user, @Frenzie, @hius07, @pazos, @poire-z, and @yparitcher)
frontend/apps/reader/modules/readerfooter.lua
line 1453 at r23 (raw file):
Previously, poire-z wrote…
Classic comments please, only 2 dash
--
, and as it starts a paragraph, start it with an Uppercase (unlike inline comments which are ok lowercase).
Add a blank line before as it starts a logical section.
This line of dashes is still here ;).
frontend/apps/reader/modules/readerfooter.lua
line 682 at r28 (raw file):
option_help_text["percentage"] = _("Progress percentage can be shown with zero, one or two decimal places.") option_help_text["mem_usage"] = _("Show memory usage in MiB.") option_help_text["reclaim_height"] = _("When status bar is unlocked and hidden, this setting will utilise the entirety of screen real state and will temporarily overlap status bar and text when unhidden.")
nit: I think we tend to prefer en_US, so utilize?
Less of a nit: The locked stated is irrelevant here, so I'd get rid of that mention for clarity (i.e., keep it to hidden/shown).
Nothing jumped out in the practical test & second pass ;). And this fixes the test: diff --git a/spec/unit/readerfooter_spec.lua b/spec/unit/readerfooter_spec.lua
index 127f8eb6e..a3af93d84 100644
--- a/spec/unit/readerfooter_spec.lua
+++ b/spec/unit/readerfooter_spec.lua
@@ -253,7 +253,7 @@ describe("Readerfooter module", function()
footer.footer_text.text)
-- disable show all at once, page progress should be on the first
- tapFooterMenu(fake_menu, "Show all at once")
+ tapFooterMenu(fake_menu, "Show all selected items at once")
assert.are.same('1 / 2', footer.footer_text.text)
-- disable page progress, time should follow
@@ -269,7 +269,7 @@ describe("Readerfooter module", function()
assert.are.same('0%', footer.footer_text.text)
-- disable battery, percentage should follow
- tapFooterMenu(fake_menu, "Battery status".." ()")
+ tapFooterMenu(fake_menu, "Battery percentage".." ()")
assert.are.same('⤠ 50%', footer.footer_text.text)
-- disable percentage, book time to read should follow
@@ -277,15 +277,15 @@ describe("Readerfooter module", function()
assert.are.same('⏳ N/A', footer.footer_text.text)
-- disable book time to read, chapter time to read should follow
- tapFooterMenu(fake_menu, "Book time to read".." (⏳)")
+ tapFooterMenu(fake_menu, "Time left to finish book".." (⏳)")
assert.are.same('⤻ N/A', footer.footer_text.text)
-- disable chapter time to read, text should be empty
- tapFooterMenu(fake_menu, "Chapter time to read".." (⤻)")
+ tapFooterMenu(fake_menu, "Time left to finish chapter".." (⤻)")
assert.are.same('', footer.footer_text.text)
-- reenable chapter time to read, text should be chapter time to read
- tapFooterMenu(fake_menu, "Chapter time to read".." (⤻)")
+ tapFooterMenu(fake_menu, "Time left to finish chapter".." (⤻)")
assert.are.same('⤻ N/A', footer.footer_text.text)
readerui:closeDocument()
readerui:onClose()
@@ -528,7 +528,7 @@ describe("Readerfooter module", function()
assert.is.same(1, found)
-- disable auto refresh time
- tapFooterMenu(fake_menu, "Auto refresh")
+ tapFooterMenu(fake_menu, "Auto refresh items")
found = 0
for _,task in ipairs(UIManager._task_queue) do
if task.action == footer.autoRefreshFooter then
@@ -538,7 +538,7 @@ describe("Readerfooter module", function()
assert.is.same(0, found)
-- enable auto refresh time again
- tapFooterMenu(fake_menu, "Auto refresh")
+ tapFooterMenu(fake_menu, "Auto refresh items")
found = 0
for _,task in ipairs(UIManager._task_queue) do
if task.action == footer.autoRefreshFooter then
@@ -699,7 +699,7 @@ describe("Readerfooter module", function()
-- test in all at once mode
tapFooterMenu(fake_menu, "Progress percentage".." (⤠)")
- tapFooterMenu(fake_menu, "Show all at once")
+ tapFooterMenu(fake_menu, "Show all selected items at once")
assert.is.same(false, footer.has_no_mode)
tapFooterMenu(fake_menu, "Progress percentage".." (⤠)")
assert.is.same(true, footer.has_no_mode) |
Fine with me (not re-reading the whole thing, I had enough of it :)) Still some CI failures:
Not sure they are the same that @NiLuJe proposed a fix for (where it seems it just needed to get reported back the new menu title strings), but you @Commodore64user need to update Not sure when to merge this, there's a lot of new translatable strings, so if soon we'll have to push a bit next release. |
I'd push the next back to June unless there's a particular reason to get out an interim release (pre giant base upgrade). So merging this should be fine. |
Sorry I didn’t know i had to update that file too, will do so later today. |
am I still missing anything else that needs doing here? |
If we want to change anything else we'll do it in a follow-up PR(s). |
As discussed in #11605 and #11533, here is my proposal for the new and improved
status bar
menu.No changes were made to the logic besides, adding a few
if isTouch()
statements to a couple of settings that shouldn't be available to non-touch devices (lock status bar
andhold to skim
) and any necessary changes for the correct operation of the menu. Also achecked function
was added to the battery complication on the alternative status bar.Most of the settings were reworded and/or rewritten. Please don't take it personally if I changed yours.
In defence of "complications": The word is borrowed from horology (science of time and timekeeping) were it refers to features on mechanical watches (smart ones too) that convey information not related to hours, minutes or seconds.
In the KOReader context, they refer to all features that convey any type of information on the status bar. I have decided, against your better judgement, to use the word in this PR as no better alternative was put forward and, all other possible replacements seem to fall short of the goal.
Some suggestions like "components" and "elements" were straight up rejected by other GitHub users, others like "status icons", "symbols", "status indicators", and all possible permutations of those fail to completely convey the idea of "Information". For example, 45/254 (page 45 of 254) is neither a symbol, or "icon" or "indicator", it is simply a piece of information. Besides, some of those same terms are already being used in a different context within the 'status bar' menu.
I encourage EVERYONE to download the files and run them on your machines, as that is the only way in which all of these changes will make sense. Read all the menus, play with them, in short; go have fun.
Due to the sheer number of changes, providing a pdf (like in #11549) documenting all changes would be a monster task I am not happy to go through right now. So I would appreciate it if one of you could kindly provide screenshots like the ones found here #11533 (comment), although bear in mind, those are from an older state of development. But again, preferably run it locally.
Special thanks to @poire-z who provided unrelenting support throughout the redevelopment of the menu.
This change is