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

ReaderFooter: Make pages_left_book use an actual prefix icon #7807

Merged
merged 2 commits into from Jun 8, 2021

Conversation

@NiLuJe
Copy link
Member

@NiLuJe NiLuJe commented Jun 5, 2021

Instead of baking in a hyphen, so as to avoid signed zeroes, or confusion with page progress if page maps are enabled.

Also, made compact items RTL friendly.


This change is Reviewable

Instead of baking in a hyphen, so as to avoid signed zeroes, or
confusion with page progress if page maps are enabled.

Also, made compact itemps RTL friendly.
@NiLuJe NiLuJe changed the title ReaderFooter: Make pages_left_book use an actual prefix icon [RFC] ReaderFooter: Make pages_left_book use an actual prefix icon Jun 5, 2021
@NiLuJe
Copy link
Member Author

@NiLuJe NiLuJe commented Jun 5, 2021

@NiLuJe
Copy link
Member Author

@NiLuJe NiLuJe commented Jun 5, 2021

Not necessarily super-fond of the arrow choice here (other than the semantic hint as a trail arrow), but I don't personally use this widget, so, open to suggestions ;).

remaining

@poire-z
Copy link
Contributor

@poire-z poire-z commented Jun 5, 2021

Not using it, but looks ok.
(Is it just me, but I can't review/approve, I don't see the Review button in this PR, but I see it in some other PRs...)

@NiLuJe
Copy link
Member Author

@NiLuJe NiLuJe commented Jun 5, 2021

(Is it just me, but I can't review/approve, I don't see the Review button in this PR, but I see it in some other PRs...)

I can see it in the 'Files changed' tab, FWIW. But GH's home page has been doing wonky shit for a couple days on my end now, so, who knows ^^.

@poire-z
Copy link
Contributor

@poire-z poire-z commented Jun 5, 2021

OK, now I have the "Review changes" button... So, nevermind...

(i.e., in-chapter num / total).

Fix koreader#7792
@NiLuJe
Copy link
Member Author

@NiLuJe NiLuJe commented Jun 5, 2021

Also added the FR from #7792. Ping @Jellby because I don't have anything w/ hidden flows on hand right now to test that I didn't botch getChapterPageCount there ;).

(Also, mildly intrigued that we didn't have such a method to begin with?).

Also, unsure how well the (existing) hidden flows stuff handles being the first/last chapter, because, AFAICT, isChapterStart does not match on beginning/end of book?

current = footer.pageno
end
local total = footer.ui.toc:getChapterPageCount(footer.pageno) or footer.pages
return current .. " ⁄⁄ " .. total
Copy link
Contributor

@poire-z poire-z Jun 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dunno how confusing this can be, but // is already used in page_progress when hasHiddenFlows().

Copy link
Member Author

@NiLuJe NiLuJe Jun 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not exactly the same slash, I don't remember which one I settled on, but basically this one has extreme kerning, so you end up with the two being very close together (the idea being it echoes the double arrow of the "remaining pages in chapter" widget ;)).

(In fact, I had to use narrow nbsp around it for the menu entry, otherwise it ran into the parens ;p).

Copy link
Contributor

@Jellby Jellby Jun 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be happy with changing the // with hidden flows to something else. Maybe ?

Copy link
Member Author

@NiLuJe NiLuJe Jun 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slashes

Right now, I'm using the third set (the two narrow slashes very close together). I was thinking of adding a hair space between them: that's the first set (but looking at all of 'em like that, I kinda prefer the current approach, as it's really distinctive).

For comparison, a standard double slash is on second place, and finally the ⌿.

I feel like it's different enough that we can keep the "double means chapter/flow" semantic hint between 'em (so, not touching the flows' double slash).

@Frenzie
Copy link
Member

@Frenzie Frenzie commented Jun 5, 2021

@poire-z You might need to do Ctrl+F5 or something if GH messed up their script cache.

Frenzie
Frenzie approved these changes Jun 5, 2021
Copy link
Member

@Frenzie Frenzie left a comment

Seems okay

@NiLuJe NiLuJe merged commit ba070c2 into koreader:master Jun 8, 2021
1 of 2 checks passed
@Frenzie Frenzie added this to the 2021.06 milestone Jun 8, 2021
@Frenzie Frenzie changed the title [RFC] ReaderFooter: Make pages_left_book use an actual prefix icon ReaderFooter: Make pages_left_book use an actual prefix icon Jun 8, 2021
@hius07
Copy link
Member

@hius07 hius07 commented Jul 1, 2021

Seems that enabling Current page in chapter prevents updating total book/chapter pages count.
On the screenshots: opened a book with font size 22, then changed font size to 24.

1

2

190 on the second screenshot is a number of pages in book with font size 22 (was valid before changing to 24).
Current page in book (230) and page in chapter (6) are correct.

@hius07
Copy link
Member

@hius07 hius07 commented Jul 1, 2021

The wrong total number of pages I see in the Go to dialog as well (in the hint).

@hius07
Copy link
Member

@hius07 hius07 commented Jul 2, 2021

The issue is in

local next_chapter = self:getNextChapter(pageno) or self.ui.document:getPageCount() + 1

getPageCount returns previous number of pages (before changing the font size, in my example).

Or

local total = footer.ui.toc:getChapterPageCount(footer.pageno) or footer.pages

is called before the page/footer is recalculated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants