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

lvdocview: fix and expose functionality for displaying author and book title separately in the status bar #575

Merged
merged 3 commits into from
Jul 20, 2024

Conversation

trash-pandy
Copy link
Contributor

@trash-pandy trash-pandy commented Jul 2, 2024

I enjoy using KOReader, and I especially like using the alt status bar, but the author and title switching every page was distracting on my Kindle PW2's e-ink display. The functionality seemed to be mostly in place already, but wasn't entirely exposed. I have made an implementation of the feature in KOReader and plan to make a pull request to that repo as well, if this pull request gets merged. I have tested the feature in the emulator and it appears to be working properly, but a second opinion would be much appreciated.

koreader alt status menu


This change is Reviewable

@poire-z
Copy link
Contributor

poire-z commented Jul 2, 2024

Looks alright (minus the minor indentation issues we see at the bottom of the Files change pane on GitHub).
Looks like setStatusMode is only used internally so there should be no problem with its signature change.

@trash-pandy
Copy link
Contributor Author

There seems to be a lot of whitespace issues in the project. I felt it was mildly out of scope for this PR. I can make some minor, localized edits to more closely match the code around them, if that's the only blocker to this being merged.

@poire-z
Copy link
Contributor

poire-z commented Jul 5, 2024

There seems to be a lot of whitespace issues in the project. I felt it was mildly out of scope for this PR. I can make some minor, localized edits to more closely match the code around them

Yes, there are mixed and ugly whitespace mismatchs.
But please don't fix the whitespace on lines you're not touching.
Just be sure the lines you touch or add do match the code around - so you don't add to the mess.

@trash-pandy
Copy link
Contributor Author

Anything else I can do?

@poire-z
Copy link
Contributor

poire-z commented Jul 11, 2024

Looks ok, will merge in a few days - we're still waiting for a stable release of koreader to bump crengine, as it includes some possibly risky commits.

@poire-z poire-z merged commit 39b0673 into koreader:master Jul 20, 2024
1 check passed
@poire-z
Copy link
Contributor

poire-z commented Jul 20, 2024

No change in koreader-base I assume?
So, you'll be able to PR the frontend part after this gets bumped into koreader (tomorrow or the day after).
Will it work ok before you add the frontend part? Or do we need to be sync'ed?

@trash-pandy
Copy link
Contributor Author

If it isn't synced, then the author will be always displayed in the top left with the alt status bar.

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

Successfully merging this pull request may close these issues.

2 participants