Add pages remaining option to status bar #7047

JasonInOttawa · 2020-12-24T22:58:38Z

A trivial implementation of adding pages remaining to the status bar, for those times when you want to know how many pages remain and aren't up to doing the subtraction.

This is added to the Status bar settings:

This is how things look in the status bar

This change is

yparitcher · 2020-12-25T01:43:59Z

frontend/apps/reader/modules/readerfooter.lua

-    time = 2,
-    pages_left = 3,
-    battery = 4,
-    percentage = 5,
-    book_time_to_read = 6,
-    chapter_time_to_read = 7,
-    frontlight = 8,
-    mem_usage = 9,
-    wifi_status = 10,
-    book_title = 11,
-    book_chapter = 12,
-    bookmark_count = 13,
+    page_remaining = 2,
+    time = 3,
+    pages_left = 4,
+    battery = 5,
+    percentage = 6,
+    book_time_to_read = 7,
+    chapter_time_to_read = 8,
+    frontlight = 9,
+    mem_usage = 10,
+    wifi_status = 11,
+    book_title = 12,
+    book_chapter = 13,
+    bookmark_count = 14,


Will changing the mode values have an effect on existing users?
Does it make sense to add page_remaining at the end as number 14?

I honestly don't know - I thought they were like const values and changing them would just work.

Please be sure of that :)
I think the items we added the most recently were added last.
But looking at the code, it feels it would be fine with stuff added at any place in the list (it's possible we made it better because it was a mess previously and these kind of stuff were risky :)
Please just test that it's fine with a user previous custom ordering. (Can't test myself atm.)

There's also another ordered list as a comment at top of ReaderFooter:applyFooterMode(mode). Dunno if it should be updated.

Thanks very much!

I have tried turning on/off all the status bar options and moving them about and haven't seen any issues. Is that sufficient, or should I try something else?

The comment you mentioned reads

-- three modes switcher for reader footer -- 0 for footer off -- 1 for footer page info -- 2 for footer time info -- 3 for footer next_chapter info -- 4 for battery status -- 5 for progress percentage -- 6 for from statistics book time to read -- 7 for from statistics chapter time to read -- 8 for front light level -- 9 for memory usage -- 10 for Wi-Fi status -- 11 for book title -- 12 for current chapter -- 13 for bookmark count

and the "local mode" reads

off = 0, page_progress = 1, page_remaining = 2, time = 3, pages_left = 4, battery = 5, percentage = 6, book_time_to_read = 7, chapter_time_to_read = 8, frontlight = 9, mem_usage = 10, wifi_status = 11, book_title = 12, book_chapter = 13, bookmark_count = 14,

Should they line up? I'm not sure how to handle the "footer page info" and "footer time info" entries.

The comment you mentioned reads
-- three modes switcher for reader footer
-- 0 for footer off
-- 1 for footer page info

Yep, and I don't even understand what this comment is trying to say :) It has been completed over the years with the added items - so, I guess they should line up for consistency.
But I'd also be happy if you just deleted the whole comment - so there's no second set to keep updated for nothing.

should I try something else?

I just checked:

no ordering done before applying this PR - then PR applied - all ok.

previous user ordering done before applying this PR - then PR applied - ordering kept, the new item is put at the end of the user order

So, I guess it's ok.

hius07 · 2020-12-25T04:43:16Z

Is it the same symbol as for Pages left in chapter?

JasonInOttawa · 2020-12-25T13:02:07Z

It's the -331 / 373 (there are 331 pages left in the book). The minus sign is meant to show that it's pages remaining.

hius07 · 2020-12-25T13:09:24Z

Oh, I see, thanks.

JasonInOttawa · 2020-12-25T13:25:12Z

Thanks. I'd appreciate any thoughts on making it clearer. I thought it would be something to put up instead of "Current page" since having both up seems redundant.

A trivial implementation of adding pages remaining to the status bar

Sync up with master

JasonInOttawa · 2020-12-27T18:03:20Z

I hope you don't mind a git question. When I look at the repository I forked to create this PR:

https://github.com/JasonInOttawa/koreader

I see that it is 14 commits behind master. The branch created for this PR is add-pages-remaining.

How should I rebase (?) to get my repository synced up with master?

I did try

git pull upstream master
git checkout add-pages-remaining
git rebase master
git push

The result of git push is "Everything up-to-date"

But I still see the commits-behind message.

Frenzie · 2020-12-27T18:10:43Z

Your upstream points to here, e.g.?

$ git remote show upstream 
* remote upstream
  Fetch URL: https://github.com/koreader/koreader.git
  Push  URL: https://github.com/koreader/koreader.git
  HEAD branch: master
  Remote branch:
    master tracked
  Local ref configured for 'git push':
    master pushes to master (local out of date)

What I'd do though:

git checkout add-pages-remaining
# possibly git checkout -b copy-to-mess-up

# rebase commits in branch on top of whatever I'm pulling
git pull -r upstream master

JasonInOttawa · 2020-12-27T18:22:06Z

Thanks very much. I pretty sure I'm missing something quite basic :)

This is what I see. I seem to be in sync with master, but my repository is still out of date.

$ git checkout add-pages-remaining
M	l10n
Already on 'add-pages-remaining'
Your branch is up to date with 'origin/add-pages-remaining'.
$ git remote show upstream
* remote upstream
  Fetch URL: https://github.com/koreader/koreader.git
  Push  URL: https://github.com/koreader/koreader.git
  HEAD branch: master
  Remote branch:
    master tracked
  Local ref configured for 'git push':
    master pushes to master (up to date)
$ git pull -r upstream master
From https://github.com/koreader/koreader
 * branch              master     -> FETCH_HEAD
Current branch add-pages-remaining is up to date.
$ git push
Username for 'https://github.com': JasonInOttawa
Password for 'https://JasonInOttawa@github.com': 
Everything up-to-date

Frenzie · 2020-12-27T18:27:20Z

It's not though? You have to look at that branch, not your master branch. ;-)

https://github.com/JasonInOttawa/koreader/tree/add-pages-remaining

JasonInOttawa · 2020-12-27T18:53:31Z

Thank you! In future, I should check

https://github.com/JasonInOttawa/koreader/tree/add-pages-remaining

to see whether I'm behind in commits, and if so do this for a fresh build.

(Right now I'm getting the "Current branch add-pages-remaining is up to date" because my add-pages-remaining branch is in fact up to date.)

$ git clone https://github.com/JasonInOttawa/koreader.git
Cloning into 'koreader'... 
remote: Enumerating objects: 23, done. 
remote: Counting objects: 100% (23/23), done.
remote: Compressing objects: 100% (20/20), done.
remote: Total 49747 (delta 7), reused 8 (delta 3), pack-reused 49724
Receiving objects: 100% (49747/49747), 43.08 MiB | 18.62 MiB/s, done.
Resolving deltas: 100% (33487/33487), done.
$ cd koreader/
/koreader$ git remote add upstream https://github.com/koreader/koreader.git
/koreader$ git remote -v
origin  https://github.com/JasonInOttawa/koreader.git (fetch)
origin  https://github.com/JasonInOttawa/koreader.git (push)
upstream        https://github.com/koreader/koreader.git (fetch)
upstream        https://github.com/koreader/koreader.git (push)
/koreader$ git checkout add-pages-remaining
Branch 'add-pages-remaining' set up to track remote branch 'add-pages-remaining' from 'origin'.
Switched to a new branch 'add-pages-remaining'
/koreader$ git remote show upstream
* remote upstream
  Fetch URL: https://github.com/koreader/koreader.git
  Push  URL: https://github.com/koreader/koreader.git
  HEAD branch: master
  Remote branch:
    master new (next fetch will store in remotes/upstream)
  Local ref configured for 'git push':
    master pushes to master (local out of date)
/koreader$ git pull -r upstream master
From https://github.com/koreader/koreader
 * branch              master     -> FETCH_HEAD
 * [new branch]        master     -> upstream/master
Current branch add-pages-remaining is up to date.

NiLuJe · 2020-12-27T21:32:41Z

Wild guess, because I seem to recall you're on macOS: the stock git shipped by macOS is hilariously old, that may be a factor here.

NiLuJe · 2020-12-27T21:34:58Z

I tend to do it the other way around, e.g., in my topic branch (which is already tied to a remote branch, because I have a git-rbranch alias¹):

git fetch upstream
git rebase -i upstream/master

(Or not interactive if it's a trivial rebase).

[1]

function git-rbranch() {
        if (( ${ARGC} < 1 )) ; then
                echo "Branch name not specified!"
        fi
        local new_branch="${1}"
        
        # Create it locally
        git checkout -b "${new_branch}"
        # Push it
        git push origin "${new_branch}"
        # Set up tracking info
        git branch -u "origin/${new_branch}" "${new_branch}"
}

JasonInOttawa · 2020-12-28T13:43:35Z

Thanks very much for your help with git.

I do have a Macbook (for work) and Linux running on an old Elitebook G1 (for messing around with). I'm mostly playing with git on Linux during the holidays.

poire-z · 2020-12-30T21:47:41Z

Please just remove your 2nd commit - your first commit is enough, no rebase/sync needed (readerfooter has not been updated by anybody since you started working on it).

poire-z · 2020-12-30T21:50:15Z

frontend/apps/reader/modules/readerfooter.lua

@@ -352,6 +378,7 @@ function ReaderFooter:init()
        bookmark_count = true,
        time = true,
        page_progress = true,
+        page_remaining = true,


We most probably want page_remaining = false - as having both page_progress and page_remaining enabled by default will confused people when they set "all at once".

(Also, it's the counterpart of page_progress so it's fine it's named page_remaining - but I would spell it more naturally as pages_remaining - plural. Dunno which is best.)

A quick Google shows many ways to delete a commit - could you kindly suggest the "best" method to use here?

I named it page_remaining to match page_progress - should I change one or both of them?

Thanks!

I guess you can't change page_progress as it's already used and saved in current users' settings.
I let @Frenzie tell us that page_remaining is just fine :)

could you kindly suggest the "best" method to use here?

Assuming you still have only these 2 commits:

git checkout add-pages-remaining git reset --hard HEAD~1 # this resets 1 commit back, so removing your 2nd commit git status # check all looks fine git log # check all looks fine git push -f origin add-pages-remaining

I named it page_remaining to match page_progress - should I change one or both of them?

@poire-z's point is that you say "page progress" and "pages remaining" (or "remaining pages") in natural language. ;-) I'd also prefer that s.

Much more important for variable naming though, is that we already have a pages_left which sounds completely synonymous. We should pick either left or remaining and add _chapter or _in_chapter to the one, _book or _in_book to the other, or both.

We should pick either left or remaining and add

But that's a bit tough for this PR, and a possible need for migration of previous settings names.

True, but I was also talking about the menu text.

Also I think that implies that in principle the new setting should be called pages_left_book unless there's a sufficiently strong reason not to.

This reverts commit 66b9c4e.

Change page_remaining to pages_left_book Set default page_remaining to false Remove out-of-date comment in ReaderFooter:applyFooterMode(mode)

poire-z · 2020-12-31T15:10:32Z

frontend/apps/reader/modules/readerfooter.lua

@@ -792,7 +792,7 @@ function ReaderFooter:textOptionTitles(option)
        reclaim_height = _("Reclaim bar height from bottom margin"),
        bookmark_count = T(_("Bookmark count (%1)"), symbol_prefix[symbol].bookmark_count),
        page_progress = T(_("Current page (%1)"), "/"),
-        page_remaining = T(_("Pages remaining (%1)"), "-"),
+        pages_left_book = T(_("Pages remaining (%1)"), "-"),


May be Pages left in book then.

There's no maybe. The distinction between pages left and pages remaining cannot be chapter vs book. ;-)

Or rather, there's no distinction at all. Those are synonyms.

poire-z · 2020-12-31T15:13:23Z

frontend/apps/reader/modules/readerfooter.lua

+    pages_left_book = function(footer)
+        if footer.pageno then
+            if footer.ui.pagemap and footer.ui.pagemap:wantsPageLabels() then
+                -- (Page labels might not be numbers)
+                return ("%s / %s"):format(footer.ui.pagemap:getCurrentPageLabel(true),
+                                          footer.ui.pagemap:getLastPageLabel(true))
+            end
+            if footer.ui.document:hasHiddenFlows() then
+                -- i.e., if we are hiding non-linear fragments and there's anything to hide,
+                local flow = footer.ui.document:getPageFlow(footer.pageno)
+                local page = footer.ui.document:getPageNumberInFlow(footer.pageno)
+                local pages = footer.ui.document:getTotalPagesInFlow(flow)
+                if flow == 0 then
+                    return ("%d // %d"):format(page, pages)
+                else
+                    return ("[%d / %d]%d"):format(page, pages, flow)
+                end
+            else
+                local remaining = footer.pages - footer.pageno
+                return ("-%d / %d"):format(remaining, footer.pages)


Looks like a quick copy and paste, where you just updated with -%d the most used branch. But the others (pagemap, hiddent flow, footer.position) might need to be updated too for this option to be consistent.

It is, yes. I can try revising the other branches but I don't know how to test them :)

Well, for footer.position, not sure when/if it is used, but it should logically be:
return ("%d / %d"):format(footer.doc_height - footer.position, footer.doc_height)

For pagemap, well, these are strings, so you can't do any arithmetic with them. I guess you can let them as-is then.
For hidden flows, you need an EPUB with non-linear stuff. Dunno where to find one. But it should be similar to what you did:

local page = footer.ui.document:getPageNumberInFlow(footer.pageno) local pages = footer.ui.document:getTotalPagesInFlow(flow) + local remaining = pages - page + -- and use it in the return -%d

Change menu text to "Pages left in book" Add proper handling of other situations in pages_left_book function

poire-z · 2020-12-31T19:46:44Z

Tested with pagemap & non-linear flows: looks good to me.

JasonInOttawa · 2020-12-31T20:07:35Z

Great stuff. Did you test it by copying the changed readerfooter.lua file into an existing installation?

poire-z · 2020-12-31T20:47:44Z

I usually just do (when testing on the emulator, but also if I need to test it on my Kobo):

wget https://github.com/koreader/koreader/pull/7047.diff
patch -p1 < 7047.diff

and when done testing (on the emulator):

git checkout frontend

JasonInOttawa · 2020-12-31T21:17:41Z

That's quite cool, thank you

yparitcher reviewed Dec 25, 2020

View reviewed changes

JasonInOttawa added 2 commits December 27, 2020 12:30

Add pages remaining option to status bar

7d21c75

A trivial implementation of adding pages remaining to the status bar

Sync up with master

66b9c4e

Sync up with master

poire-z reviewed Dec 30, 2020

View reviewed changes

JasonInOttawa added 2 commits December 31, 2020 07:28

Revert "Sync up with master"

173fad7

This reverts commit 66b9c4e.

Changes per review

afcfb5e

Change page_remaining to pages_left_book Set default page_remaining to false Remove out-of-date comment in ReaderFooter:applyFooterMode(mode)

poire-z reviewed Dec 31, 2020

View reviewed changes

Changes per review

7fac80f

Change menu text to "Pages left in book" Add proper handling of other situations in pages_left_book function

poire-z merged commit 32b28c0 into koreader:master Dec 31, 2020

NiLuJe added a commit to NiLuJe/koreader that referenced this pull request Jan 1, 2021

Unbreak testsuite after koreader#7047

ffb7043

NiLuJe mentioned this pull request Jan 1, 2021

Fix opacity of the dictionary window #7097

Merged

Frenzie pushed a commit that referenced this pull request Jan 1, 2021

Unbreak testsuite after #7047

048b0b2

JasonInOttawa deleted the add-pages-remaining branch January 1, 2021 15:09

Frenzie added this to the 2021.01 milestone Jan 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pages remaining option to status bar #7047

Add pages remaining option to status bar #7047

JasonInOttawa commented Dec 24, 2020 •

edited by Frenzie

Loading

yparitcher Dec 25, 2020 •

edited

Loading

JasonInOttawa Dec 25, 2020

poire-z Dec 26, 2020

JasonInOttawa Dec 26, 2020

poire-z Dec 30, 2020

hius07 commented Dec 25, 2020

JasonInOttawa commented Dec 25, 2020

hius07 commented Dec 25, 2020

JasonInOttawa commented Dec 25, 2020

JasonInOttawa commented Dec 27, 2020

Frenzie commented Dec 27, 2020

JasonInOttawa commented Dec 27, 2020

Frenzie commented Dec 27, 2020

JasonInOttawa commented Dec 27, 2020

NiLuJe commented Dec 27, 2020

NiLuJe commented Dec 27, 2020 •

edited

Loading

JasonInOttawa commented Dec 28, 2020 •

edited

Loading

poire-z commented Dec 30, 2020

poire-z Dec 30, 2020

JasonInOttawa Dec 31, 2020

poire-z Dec 31, 2020

Frenzie Dec 31, 2020

poire-z Dec 31, 2020

Frenzie Dec 31, 2020

poire-z Dec 31, 2020

Frenzie Dec 31, 2020

Frenzie Dec 31, 2020

poire-z Dec 31, 2020

JasonInOttawa Dec 31, 2020

poire-z Dec 31, 2020

poire-z commented Dec 31, 2020

JasonInOttawa commented Dec 31, 2020

poire-z commented Dec 31, 2020

JasonInOttawa commented Dec 31, 2020

Add pages remaining option to status bar #7047

Add pages remaining option to status bar #7047

Conversation

JasonInOttawa commented Dec 24, 2020 • edited by Frenzie Loading

yparitcher Dec 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hius07 commented Dec 25, 2020

JasonInOttawa commented Dec 25, 2020

hius07 commented Dec 25, 2020

JasonInOttawa commented Dec 25, 2020

JasonInOttawa commented Dec 27, 2020

Frenzie commented Dec 27, 2020

JasonInOttawa commented Dec 27, 2020

Frenzie commented Dec 27, 2020

JasonInOttawa commented Dec 27, 2020

NiLuJe commented Dec 27, 2020

NiLuJe commented Dec 27, 2020 • edited Loading

JasonInOttawa commented Dec 28, 2020 • edited Loading

poire-z commented Dec 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poire-z commented Dec 31, 2020

JasonInOttawa commented Dec 31, 2020

poire-z commented Dec 31, 2020

JasonInOttawa commented Dec 31, 2020

JasonInOttawa commented Dec 24, 2020 •

edited by Frenzie

Loading

yparitcher Dec 25, 2020 •

edited

Loading

NiLuJe commented Dec 27, 2020 •

edited

Loading

JasonInOttawa commented Dec 28, 2020 •

edited

Loading