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

Ensure minimum height for the in-fill in progress bar #6878

Merged
merged 1 commit into from
Nov 18, 2020

Conversation

Jellby
Copy link
Contributor

@Jellby Jellby commented Nov 14, 2020

With some screen sizes, the footer progress bar (in thick style) may be empty, because margins and borders take the full height. This should ensure that does not happen, by reserving at least 1 pixel for the actual progress bar.

Left: before the PR. Right: after
Screenshot_20201114_115606 Screenshot_20201114_115723
with ./kodev run -h=720 -w=1024, for sizes 5 to 9 (which get scaled to 6 to 10, and the default border and margin are scaled to 2)


This change is Reviewable

Comment on lines 55 to 56
margin_v_orig = nil,
bordersize_orig = nil,
Copy link
Member

Choose a reason for hiding this comment

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

These seem to be for internal purposes only.

@Frenzie
Copy link
Member

Frenzie commented Nov 14, 2020

Sounds good. "Some screen sizes" is very vague though. What kind of screen sizes? :-)

Comment on lines 18 to 19
* margin_v_orig -- margin_v before adjusting
* bordersize_orig -- bordersize before adjusting
Copy link
Contributor

Choose a reason for hiding this comment

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

If there are just internal variables, and they shouldn't ever need to be provided by caller, may be don't document them here.
And name them _orig_margin_v, with a leading underscore as a kind of convention these are private.

@Jellby
Copy link
Contributor Author

Jellby commented Nov 14, 2020

Sounds good. "Some screen sizes" is very vague though. What kind of screen sizes? :-)

I don't know, whatever causes Screen:scaleBySize(1) > 1 and Screen:scaleBySize(5) < 9, I guess. I gave an example in the top message: 720×1024

@@ -660,7 +660,7 @@ function ReaderFooter:resetLayout(force_reset)
else
bar_height = self.settings.progress_style_thick_height or PROGRESS_BAR_STYLE_THICK_DEFAULT_HEIGHT
end
self.progress_bar.height = Screen:scaleBySize(bar_height)
self.progress_bar:setHeight(bar_height)
Copy link
Member

@NiLuJe NiLuJe Nov 14, 2020

Choose a reason for hiding this comment

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

There's another instance of this @ L1836

(Which I'm not quite sure is actually necessary at all, because it has nothing whatsoever to do with what that function's supposed to do... It feels like a leftover from a c/p from this resetLayout, actually)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought I had searched for other instances, obviously I missed at least one.

@poire-z poire-z added this to the 2020.12 milestone Nov 18, 2020
@poire-z poire-z merged commit dba7112 into koreader:master Nov 18, 2020
NiLuJe added a commit that referenced this pull request Nov 22, 2020
Some stuff was still hacked on manually instead of trusting the widget system to do things right, which it does, if you update the right stuff at the right time the right way ;). *This Is The Way*.

Fix #6893 (and address #6878 (comment), because it was indeed redundant ^^).

Includes a bonus fix for a number of (footer) settings not being applied immediately in PDFs.
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.

None yet

4 participants