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

loading bar thicker #5581

Merged
merged 4 commits into from Nov 10, 2019
Merged

loading bar thicker #5581

merged 4 commits into from Nov 10, 2019

Conversation

@yparitcher
Copy link
Contributor

yparitcher commented Nov 8, 2019

the current thickness for the loading bar when opening a document is to thin to really see on my KT4,
i don't have screenshots as they only trigger once koreader finishes loading.

i would like to make it a little thicker, it still does not fully cover the top menu bar.

let me know if this is the wrong place to change the number, and if it has negative effects on other devices. (my emulator loads books too fast to get the bar)

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Nov 8, 2019

i don't have screenshots as they only trigger once koreader finishes loading.

You could post a photo.

i would like to make it a little thicker, it still does not fully cover the top menu bar.

I see how 5 could feel too thin. Tested on my device, and I find 15 quite big. 10 would be fine for me, 12 would be the max I would not find ugly. (Somehow, the taller it is, the less black it feels, so it looks like some grey/dirty mass on my GloHD - and at 15, it slightly overrides Kobo Start Menu (kobo launcher) first text line.)

Anyway, I wouldn't really mind it, I see it quite rarely :)

let me know if this is the wrong place to change the number

I don't see any other place :) But you could define it as some module variable, like those a few lines above (would make it easier for me to patch it back to 5 in my own patch :)
local ENGINE_PROGRESS_HEIGHT = Screen:scaleBySize(15)

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Nov 9, 2019

if it has negative effects on other devices

It should have roughly the same physical size on all devices. Personally I rather like that it's an elegant line rather than some distracting blob that just flashes for half a second.

I don't see any other place :)

I thought it was intended to be more or less the same as any progressbar (like at the bottom), which however happens to use a value of 3 or 7 right now.

local PROGRESS_BAR_STYLE_THICK_DEFAULT_HEIGHT = 7
local PROGRESS_BAR_STYLE_THIN_DEFAULT_HEIGHT = 3

In regard to which I'll quote myself:

http://koreader.rocks/doc/modules/ui.size.html

There are values for borders, margins, paddings, radii, and lines. Have a look at the code for full details. If you are considering to deviate from one of the defaults, please take a second to consider:

  1. Why you want to differ in the first place. We consciously strive toward consistency in the UI, which is typically valued higher than one pixel more or less in a specific context.
  2. If there really isn't anything close to what you want, whether it should be added to the arsenal of default sizes rather than as a local exception.

So I'd say standardize that thing on 7, Size.line.progress or something.

line = {
thin = Screen:scaleBySize(0.5),
medium = Screen:scaleBySize(1),
thick = Screen:scaleBySize(1.5),
},

@yparitcher

This comment has been minimized.

Copy link
Contributor Author

yparitcher commented Nov 9, 2019

i can see how the thickness is a little ugly, so i made it the same size as the progress bar (7) but added a margin of 3 so it still can be seen.

how does that look?

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Nov 9, 2019

but added a margin of 3 so it still can be seen.

That margin adds a thin line of white above it, which I find less clean that without (without, the bar merges into the black plastic of the device).
If you need that extra height to see it, I'd rather like it to be part of the height, instead of that margin.

Care to show a pic?
Do you use a small Screen DPI?
I would have suspected some pixels are not shown and the KT4 needs to have a viewport to account for that, but I guess if that wass, it would already be known :)
Do you see a varying part of the white margin you added? Depending on how you tilt the device? (I do on my GloHD - when tilted top up 20° from horizontal, I don't see it any more.)

@yparitcher

This comment has been minimized.

Copy link
Contributor Author

yparitcher commented Nov 10, 2019

Care to show a pic?

i have no way of taking one. sorry.

Do you use a small Screen DPI?

i use the default, KT4 is 167 PPI

I would have suspected some pixels are not shown and the KT4 needs to have a viewport to account for that, but I guess if that wass, it would already be known :)
Do you see a varying part of the white margin you added? Depending on how you tilt the device? (I do on my GloHD - when tilted top up 20° from horizontal, I don't see it any more.)

yes this is due to my screen being slightly recessed so a small section at the edge is hard to tell apart from the case, especially if the device is tilted. (this is why i was not seeing the original bar)

like everything this need to have some distance from the edge in order to be seen on my device, it is the same to me if it is black or has a white margin.

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Nov 10, 2019

You mean you don't see at all the 3px white margin? Even when tilting bottom up?

@yparitcher

This comment has been minimized.

Copy link
Contributor Author

yparitcher commented Nov 10, 2019

You mean you don't see at all the 3px white margin? Even when tilting bottom up?

if i look straight i see it, however as i tilt the top towards me i see less, until i don't see it at all

i need the margin to be able to see it easily (when looking at a glance), and at an angle.

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Nov 10, 2019

Ok, I get it: when changing some layout option, and it is taking 2mn, you might not keep your device in your hands while doing other stuff, so the need to see more of it at a distance/angle, to know when it is done to get back to it ?
So, some margin or more height ? (Both feels ugly when you're just waiting with your device in your hand with no tilt :)

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Nov 10, 2019

Having seen it a few times today, I think I'll be ok with the added small margin. It makes it slightly more obvious it is a progress bar. May be use local y = Size.margin.small then.

@@ -1032,7 +1033,7 @@ function ReaderRolling:showEngineProgress(percent)
local x = 0
local y = 3
local w = Screen:getWidth() / 3
local h = Screen:scaleBySize(7)
local h = Size.line.progress

This comment has been minimized.

Copy link
@Frenzie

Frenzie Nov 10, 2019

Member

Ideally also applied to the other progressbar but not strictly necessary. :-)

@Frenzie Frenzie added this to the 2019.11 milestone Nov 10, 2019
@Frenzie Frenzie added the UX label Nov 10, 2019
@Frenzie Frenzie merged commit 816b197 into koreader:master Nov 10, 2019
1 check passed
1 check passed
ci/circleci: build Your tests passed on CircleCI!
Details
@yparitcher yparitcher deleted the yparitcher:progress_bar branch Nov 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.