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

Some footer tweaks #5203

Merged
merged 7 commits into from Aug 16, 2019

Conversation

@robert00s
Copy link
Contributor

commented Aug 13, 2019

Base on #4594
Close: #4854

Changes include:

  • adding unicode characters to menu and footer
  • move alignment to settings submenu
  • new text footer separator
  • progress percentage format (#4854)
  • date in 12/24 format
  • time format in 1h30' as option

Edit - new screenshots

obraz

obraz

obraz

obraz

obraz

obraz

obraz

obraz

@robert00s robert00s requested review from Frenzie and poire-z Aug 13, 2019

@Frenzie Frenzie added this to the 2019.09 milestone Aug 13, 2019

@Frenzie Frenzie added the UX label Aug 13, 2019

@Frenzie
Copy link
Member

left a comment

Nice adaptation of the user tweaks! Not caused by this PR as such, but apparently the localization of the footer has been a bit of a forgotten area.

else
return "W:Off"
return "Off"

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 13, 2019

Member

translation

This comment has been minimized.

Copy link
@robert00s

robert00s Aug 14, 2019

Author Contributor

done

frontlight = _("Frontlight level"),
mem_usage = _("KOReader memory usage"),
wifi_status = _("Wi-Fi status"),
page_progress = _("Current page") .. " (/)",

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 13, 2019

Member

Why concatenation? This way a translator can't reverse the order if desired. The string will show up as a 95 % match suggestion so minor changes aren't generally a big problem.

This comment has been minimized.

Copy link
@robert00s

robert00s Aug 14, 2019

Author Contributor

done

mins = string.format("%.f", round(seconds / 60))
return mins .. "'"
end
return hours .. "h" .. mins .. "'"

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 13, 2019

Member

It's a bit of a weird string to translate sure, but in Dutch for example it'd be 1u30, not 1h30.

This comment has been minimized.

Copy link
@robert00s

robert00s Aug 14, 2019

Author Contributor

done

@@ -43,24 +44,30 @@ end
local footerTextGeneratorMap = {
empty = function() return "" end,
frontlight = function()
if not Device:hasFrontlight() then return "L: NA" end
if not Device:hasFrontlight() then return " NA" end

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 13, 2019

Member

Huh, it seems odd that this is available as an option at all if not Device:hasFrontlight().

This comment has been minimized.

Copy link
@robert00s

robert00s Aug 14, 2019

Author Contributor

Done. Thanks for review :)

@poire-z
Copy link
Contributor

left a comment

Overall'y nice :)
Only thing missing is the ability to set the order of the items (they have been re-ordered in some refactoring 2 years ago, I still patch readerfooter to have them in the order I've been used to 3 years ago :)

(I'll see how the icons do, fearing they may be a little eye catching, unlike simple letters like B: or M:. Might be for another setting: Icons | Letters, for the old conservative annoying people :)

{
text = _("Vertical line") .. " (|)",
checked_func = function()
return self.settings.text_separator == "vertiacal_line" or self.settings.text_separator == nil

This comment has been minimized.

Copy link
@poire-z

poire-z Aug 13, 2019

Contributor

vertiacal_line => vertical_line - or just "bar" ?
and "none" below instead of "no_separator", text_separator == "no_separator" feels a bit redundant :)

This comment has been minimized.

Copy link
@NiLuJe

NiLuJe Aug 13, 2019

Member

Technically called a pipe ;).

This comment has been minimized.

Copy link
@robert00s

robert00s Aug 14, 2019

Author Contributor

Thanks.
I added another settings to switch between icons and letters.

{
text = T(_("No decimal point (%1)"), self:progressPercentage(0)),
checked_func = function()
return self.settings.progress_perc_format == "0" or self.settings.progress_perc_format == nil

This comment has been minimized.

Copy link
@poire-z

poire-z Aug 13, 2019

Contributor

progress_perc_format => progress_pct_format ? "pct" feels more natural than "perc" to me.
Or progress_pct_digits to be more explicite. (What's the word for digit after the dot? fractional digits?)

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 13, 2019

Member

Are you talking about decimals? :-P

This comment has been minimized.

Copy link
@robert00s

robert00s Aug 14, 2019

Author Contributor

Done.

{
text = _("Modern"),
checked_func = function()
return self.settings.time_format == "modern"

This comment has been minimized.

Copy link
@poire-z

poire-z Aug 13, 2019

Contributor

Bit confusing Date format (above) vs Time format (here).
Your Date format (12/24) indeed sets the formating of the time.
Your Time format (classic/modern) actually set the style of durations (12:04 vs 12m4s).

This comment has been minimized.

Copy link
@robert00s

robert00s Aug 14, 2019

Author Contributor

Good tip. Switched.

mins = string.format("%.f", round(seconds / 60))
return mins .. "'" .. secs .. "''"
end
return hours .. "h" .. mins .. "'" .. secs .. "''"

This comment has been minimized.

Copy link
@poire-z

poire-z Aug 13, 2019

Contributor

Mhh, in french, I'm not used to read minutes abbreviated with ' :) A simple m and s would talk to me more.
Or may be just nothing when withoutSeconds=true: 1h09 instead of 1h09' or 1h09m

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 13, 2019

Member

Agreed for English, and it should also be a localizable string.

This comment has been minimized.

Copy link
@poire-z

poire-z Aug 14, 2019

Contributor

But we need something when the minute when zero hour.
⤻ 9' is somehow ok,
⤻ 9m feels a bit strange (could feel like Mb)
⤻ 9 without anything is just confusing.
⤻ 1h09 is ok.

This comment has been minimized.

Copy link
@robert00s

robert00s Aug 14, 2019

Author Contributor

Done. I hope it will work :)

end,
},
{
text = _("Dot") .. " (•)",

This comment has been minimized.

Copy link
@NiLuJe

NiLuJe Aug 13, 2019

Member

Bullet? (or Bullet point)

This comment has been minimized.

Copy link
@robert00s

robert00s Aug 14, 2019

Author Contributor

Done. Thanks :)

@@ -100,16 +109,16 @@ local footerTextGeneratorMap = {
local rss = infos:match("^%S+ (%S+) ")
-- we got the nb of 4Kb-pages used, that we convert to Mb
rss = math.floor(tonumber(rss) * 4096 / 1024 / 1024)
return ("M:%d"):format(rss)
return ("%d"):format(rss)

This comment has been minimized.

Copy link
@NiLuJe

NiLuJe Aug 13, 2019

Member

That's a keyboard, right? Do we have a chip-like glyph available instead? That vaguely sounds more appropriate here, but I'm nitpicking ;).

This comment has been minimized.

Copy link
@NiLuJe

NiLuJe Aug 13, 2019

Member

A quick look at FreeSerif and the Unicode listing would point to "no". Unless "tape drive" (✇ U+2707) counts ;p

This comment has been minimized.

Copy link
@poire-z

poire-z Aug 14, 2019

Contributor

Or use these ones, swithing from the non-distracting white one to the heavily distracting black one when memory reaches some level:
image

At 200M on my Kobo, things begin to get slow - but I don't know how to decide that triggering limit scientifically :)

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 14, 2019

Member

At 200M on my Kobo, things begin to get slow - but I don't know how to decide that triggering limit scientifically :)

You won't reach that much memory unless it's a large and/or complex document, so that's probably expected for page turning responsiveness, but not so much for menu responsiveness.

You can do it quote unquote scientifically by simply filling up a couple 100 MB on the side. An interesting suggestion that might work on busybox (depending on how much Bash syntax it supports):

function malloc() {
  if [[ $# -eq 0 || $1 -eq '-h' || $1 -lt 0 ]] ; then
    echo -e "usage: malloc N\n\nAllocate N mb, wait, then release it."
  else 
    N=$(free -m | grep Mem: | awk '{print int($2/10)}')
    if [[ $N -gt $1 ]] ;then 
      N=$1
    fi
    sh -c "MEMBLOB=\$(dd if=/dev/urandom bs=1MB count=$N) ; sleep 1"
  fi
}

From https://unix.stackexchange.com/a/99390

(Except you'd want to sleep for a minute or two I imagine.)

This comment has been minimized.

Copy link
@poire-z

poire-z Aug 14, 2019

Contributor

You won't reach that much memory unless it's a large and/or complex document

We reach it very easily when using dict or wikipedia lookups. The TextBoxWidget code uses a lot of memory for the various Lua tables holding chars/widths/justification.
Do that, switch EPUB documents, do that again, and the memory might end up very fragmented and hardly re-used, and the usage rises.
At some point, using a gesture to show the History or TOC or top menu gets from instant to 1 or 2 seconds delay.

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 14, 2019

Member

Does the problem go away after a little while (i.e., post garbage collection)? Perhaps it could be of interest to trigger garbage collection as the final step in closing a document. On the flipside, it'd probably make it slower to reopen.

This comment has been minimized.

Copy link
@poire-z

poire-z Aug 14, 2019

Contributor

Nope. I may just sometimes get -1 or -2 Mb when switching to another EPUB document, after the next page turn.

trigger garbage collection as the final step in closing a document

We already do that:

function DocumentRegistry:openDocument(file, provider)
-- force a GC, so that any previous document used memory can be reused
-- immediately by this new document without having to wait for the
-- next regular gc. The second call may help reclaming more memory.
collectgarbage()
collectgarbage()

(It's quite possible the previous document is not yet closed at this point, as crengine may still delay the opening of the new one by saving the cache of the previous one, but switching from a big document, to a small one, and then between 2 small docs does not reclaim the large memory used by the first one.)

collectgarbage() does not help much with the memory usage rises I witness, I've already tried to set it at various places (and let some calls in the code just to be sure to get back what we can... which is not much).

This comment has been minimized.

Copy link
@robert00s

robert00s Aug 14, 2019

Author Contributor

That's a keyboard, right? Do we have a chip-like glyph available instead? That vaguely sounds more appropriate here, but I'm nitpicking ;).

@NiLuJe
I like that icon. It look almost like chip :)

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

(Looking nice on my device, the icons are not distracting.)

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

Unrelated, just for information, in case other feels it makes more sense than the => 8 (which says there are 8 pages left in this chapter, and it becomes 0 when on the last page), I have it modified to show 8/8 ... 1/8 , reverse number of page in this chapter / total number of pages in this chapter.
(I was always mentally computing +1 to => 8 for some reason...)
Or this could become another distinct foooter item.

     pages_left = function(footer)
         local left = footer.ui.toc:getChapterPagesLeft(
             footer.pageno, footer.toc_level)
-        return "=> " .. (left and left or footer.pages - footer.pageno)
+        -- return "=> " .. (left and left or footer.pages - footer.pageno)
+        -- so we see the total nb of page of this chapter
+        local done = footer.ui.toc:getChapterPagesDone(
+            footer.pageno, footer.toc_level)
+        left = left or (footer.pages - footer.pageno)
+        done = done or footer.pageno-1
+        -- return left.." > "..(left+done+1)
+        return (left+1).." > "..(left+done+1)
     end,

I had also added Wifi status with an icon to the percentage item - but I just realized we have a Wifi status footer item, which is enabled only on Android. Why is that? It's quite valuable on other devices to not forget to dismiss Wifi to save battery when done looking up Wikipedia. (I'm appending an eye catching enough ¶ WIFI for that.)

@Frenzie

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

On Android it's asynchronously updated and returns a value immediately, elsewhere it's very slow. See, for instance, #2706 (comment).

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

Oh right. I forgot I have other related tweaks, which make it usable.
(In my patch, I also have some isWifiEnabled() just checking if the wifi module appears in the content of /proc/modules so it can be synchronous and cheap - i'm personnally just interested in that for power saving.)

@Frenzie

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

Yes, I suppose that makes sense. PS More relevant reference: #3078

We currently have two states, that'd make three. Wifi enabled, wifi connected (= current wifi enabled), internet connected.

Edit: that being said, ideally you'd still want the indicator icon to show which one of those it is.

@KenMaltby

This comment has been minimized.

Copy link

commented Aug 14, 2019

I wonder if you need three, once the "Connected to ..." box is dissmissed, you could use the regular WiFi icon and have it change to the internet (globe) icon?

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

@poire-z: I'd gladly merge a /proc/modules approach instead of the lsmod one we currently use in a couple places ;). (:isWifiOn on Kobo, f.g.).

@Frenzie

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

@KenMaltby In the long run that sounds like a good idea.

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

@NiLuJe : to spare you a few minutes if you go at it, here's what I have for my Kobo:

-- We should not standby/suspend if wifi kernel modules are inserted, otherwise crash
function UIManager:isWifiEnabled()
    local wifi = false
    local fd =  io.open("/proc/modules", "r")
    if fd then
        local modules = fd:read("*all")
        fd:close()
        -- lsmod is usually empty, unless wifi or usb is enabled
        --     dhd 240713 0 - Live 0x7f19c000   (= présence directory /proc/sys/net/ipv4/conf/eth0)
        --     sdio_wifi_pwr 486 0 - Live 0x7f198000
        -- we could alternatively check if lfs.attributes("/proc/sys/net/ipv4/conf/eth0", "mode") == "directory"
        if modules:len() > 0 then
            if modules:find("dhd ") or modules:find("sdio_wifi_pwr ") then
                wifi = true
            end
        end
    end
    return wifi
end
@Frenzie

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

Isn't lsmod and /proc/modules basically the same thing, except lsmod may pull some info from other sources like /sys/module/*?

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

Most likely, but we have to fork() to use lsmod, which arguably puts more pressure on the system ;).

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

BusyBox appears to only be checking /proc/modules ;).

@Frenzie

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

According to the manual for real lsmod it's no different:

lsmod is a trivial program which nicely formats the contents of the /proc/modules, showing what kernel modules are currently loaded.

https://www.mankier.com/8/lsmod

So yes, clearly using lsmod is a bit stupid for our purposes. :-)

NiLuJe added a commit to NiLuJe/koreader that referenced this pull request Aug 14, 2019
Simplify isWifiOn on Kobo
Don't fork for lsmod + grep, we can perfectly well parse /proc/modules
ourselves ;).

c.f., koreader#5203 (comment)
@NiLuJe NiLuJe referenced this pull request Aug 14, 2019
robert00s added 2 commits Aug 14, 2019
@Frenzie
Copy link
Member

left a comment

Looking pretty nice :-)

Frenzie added a commit that referenced this pull request Aug 14, 2019
Simplify isWifiOn on Kobo (#5211)
Don't fork for lsmod + grep, we can perfectly well parse /proc/modules
ourselves ;).

c.f., #5203 (comment)
-- function that generates footer text for each mode
function ReaderFooter:footerTextGeneratorMap(option)
local ret_func = function() return "" end
if option == "empty" then

This comment has been minimized.

Copy link
@poire-z

poire-z Aug 14, 2019

Contributor

Why was the switch from the local footerTextGeneratorMap = {} table to this function needed?
Not saying it should not be done, but after a really quick look, I don't see why you needed that, and i'm curious :)

Also just fearing you may create now more anonymous functions than before (I haven't looked at the code when and how often this would be called to confirm or not my suspicion thus :)

This comment has been minimized.

Copy link
@robert00s

robert00s Aug 15, 2019

Author Contributor

Long story short. I needed use global variable (eg self.settings.text_prefix) so I created ReaderFooter:footerTextGeneratorMap(). After that I made a trick with local function

frontlight = function(footer)
    local aaa = footer.settings.text_prefix
    ...
end

so no need to use global function anymore.
I restore changes with local footerTextGeneratorMap = {}.
Thanks for paying attention :)

@@ -160,6 +101,7 @@ function ReaderFooter:init()
frontlight = false,
mem_usage = false,
wifi_status = false,
text_prefix = "icons"

This comment has been minimized.

Copy link
@poire-z

poire-z Aug 14, 2019

Contributor

text_prefix, text_separator... is "text_" needed ? just "prefix" or "separator" ? (or item_prefix and item_separator, we output text indeed, but well :)

This comment has been minimized.

Copy link
@robert00s

robert00s Aug 15, 2019

Author Contributor

Done.

wifi_status = T(_("Wi-Fi status (%1)"), symbol_prefix[symbol].wifi_status),
}
return option_titles[option]
end

function ReaderFooter:addToMainMenu(menu_items)

This comment has been minimized.

Copy link
@poire-z

poire-z Aug 14, 2019

Contributor

That menu is getting crowded :) I wouldn't mind a separator between the global options and the list of available footer items (or have these in a sub menu?).
Also may be move the Settings> among the top ones instead of having it burried after the list of footer items?

This comment has been minimized.

Copy link
@robert00s

robert00s Aug 15, 2019

Author Contributor

Done. Settings moved to top.

@robert00s

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

I also change time icon to ⌚
I'm not completely happy with the icon percentage (◔). Maybe or 🌖 or another?

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

I also change time icon to ⌚
I'm not completely happy with the icon percentage (◔). Maybe or 🌖 or another?

U+1F56E (🕮) would work, but I'm not sure we have anything that ships that glyph...

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

Random other idea: U+2920 (⤠)

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

FreeSerif's fancy % sign is at U+E879 (it's in the private use block, so it may be short-circuited by something else entirely...).

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

I'm not completely happy with the icon percentage (◔)

Same here :) This black quarter makes it the only icon that catch my eye (there's something black at the bottom: when reading text, my peripheral vision catches it, or something :).
(No alternative suggestion in my small reference epub with a limited set of symbols, sorry :|)

---- @treturn string clock string in the form of 1h30' or 1h30'10''
function util.secondsToHClock(seconds, withoutSeconds, hmsFormat)
seconds = tonumber(seconds)
if seconds == 0 or seconds ~= seconds then

This comment has been minimized.

Copy link
@poire-z

poire-z Aug 15, 2019

Contributor

seconds ~= seconds ? will always be false and of no use :)

---- @bool withoutSeconds if true 1h30', if false 1h30'10''
---- @bool hmsFormat, if true format 1h30m10s
---- @treturn string clock string in the form of 1h30' or 1h30'10''
function util.secondsToHClock(seconds, withoutSeconds, hmsFormat)

This comment has been minimized.

Copy link
@poire-z

poire-z Aug 15, 2019

Contributor

Seems to never be called with hmsFormat=true ? Did you mean for this to be a setting?

This comment has been minimized.

Copy link
@robert00s

robert00s Aug 16, 2019

Author Contributor

Not used yet. For future use :)

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

Quick test of rev2 on my device, just some observations (haven't looked at the code to pinpoint the issues):

  • Reading percentage used to get a letter R: (to differentiate it from the battery % which got B). I'd like to get it back in letter mode :)
  • progress % format does not apply (it worked with rev1), I keep getting 52% even if I chose 2 digits after (but it shows fine with the additional digits in the menu item text)
  • Menu: Text prefix> and Text separator>, probably better just Prefix> and Separator>.
  • Code: still self.settings.text_separator, for coherency with your change to item_prefix, it better be items_separator or just separator
  • Menu: Without separator => No separator
  • Alignment does not work when the progress bar is displayed, as the whole thing take the whole width: this item could be made disabled when that happens
  • I'd really like a separator after "Auto refresh time" :) (just a matter of adding separator = true to it
  • May be Show all at once, Reclaim bar height and Auto refresh time could be moved into Settings>, leaving in top menu: Settings> , Progress bar >, separator, and the items - or even have Progress bar a toggle, and Show chapter markers on progress bar in Settings>
  • (Haven't checked much the durations)
robert00s added 2 commits Aug 16, 2019
@robert00s

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

Should be better now.

@poire-z
Copy link
Contributor

left a comment

Not able to test at the moment, but it looks good to me.

@poire-z poire-z merged commit 5da3312 into koreader:master Aug 16, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@poire-z

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

Thanks :)
(very minor issue that is not worth a Rev4: when one has never toggled the progress bar, the Alignment> menu is not disabled - it is after toggle and back.)

}
},
{
text = _("Durations format"),

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 17, 2019

Member

Duration format, like the variable.

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 17, 2019

Member

Or just time format?

robert00s added a commit to robert00s/koreader that referenced this pull request Aug 17, 2019
@robert00s robert00s referenced this pull request Aug 17, 2019
Frenzie added a commit that referenced this pull request Aug 17, 2019
@poire-z

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

≡ (seen at #5277), looking like a vu-meter, could be nice to replace the KOreder memory icon (currently a keyboard ⌨). I guess that's:
image

Frenzie added a commit that referenced this pull request Sep 1, 2019
New memory icon for footer (#5300)
See: #5203 (comment)
Old memory icon: ⌨
New: ≡
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.