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

Progressbar in Mosaic View #8915

Merged
merged 28 commits into from
Mar 19, 2022
Merged

Progressbar in Mosaic View #8915

merged 28 commits into from
Mar 19, 2022

Conversation

uroybd
Copy link
Contributor

@uroybd uroybd commented Mar 16, 2022

This PR implements a progressbar in Mosaic View.


This change is Reviewable

plugins/coverbrowser.koplugin/mosaicmenu.lua Outdated Show resolved Hide resolved
plugins/coverbrowser.koplugin/mosaicmenu.lua Outdated Show resolved Hide resolved
plugins/coverbrowser.koplugin/mosaicmenu.lua Outdated Show resolved Hide resolved
@Frenzie Frenzie linked an issue Mar 16, 2022 that may be closed by this pull request
@Frenzie Frenzie added this to the 2022.04 milestone Mar 16, 2022
plugins/coverbrowser.koplugin/mosaicmenu.lua Outdated Show resolved Hide resolved
plugins/coverbrowser.koplugin/mosaicmenu.lua Outdated Show resolved Hide resolved
plugins/coverbrowser.koplugin/mosaicmenu.lua Outdated Show resolved Hide resolved
plugins/coverbrowser.koplugin/mosaicmenu.lua Outdated Show resolved Hide resolved
plugins/coverbrowser.koplugin/mosaicmenu.lua Outdated Show resolved Hide resolved
plugins/coverbrowser.koplugin/mosaicmenu.lua Outdated Show resolved Hide resolved
plugins/coverbrowser.koplugin/mosaicmenu.lua Outdated Show resolved Hide resolved
plugins/coverbrowser.koplugin/mosaicmenu.lua Outdated Show resolved Hide resolved
@poire-z
Copy link
Contributor

poire-z commented Mar 17, 2022

Tested on my Kobo:

  • it's a bit too much noise for me, so I really want the old dogear as an alternative (dunno which should be the default, this progress bar or the dogear)
  • dunno if it would look better with a thiner border size, the same size as the thumbnail border - with possibly superposition of the bottom and right borders (dunno if it's already finely positionned like or not, or if it's the border thickness difference that makes it feel like its inside the cover. And/or without the rounder corners.

@uroybd
Copy link
Contributor Author

uroybd commented Mar 17, 2022

Tested on my Kobo:

* it's a bit too much noise for me, so I really want the old dogear as an alternative (dunno which should be the default, this progress bar or the dogear)

* dunno if it would look better with a thiner border size, the same size as the thumbnail border - with possibly superposition of the bottom and right borders (dunno if it's already finely positionned like or not, or if it's the border thickness difference that makes it feel like its inside the cover. And/or without the rounder corners.

We can definitely play with styling a little more. Can you please give me some screenshots. Emulators are not entirely faithful to the reality, right?

@poire-z
Copy link
Contributor

poire-z commented Mar 17, 2022

You can just copy the single file that you are modifying on your device and see for yourself :)
https://raw.githubusercontent.com/koreader/koreader/3b0684633817752ba50c4793d1acb0ee2b3ad219/plugins/coverbrowser.koplugin/mosaicmenu.lua

This is what I get:
Reader_2022-03-17_194239

Differences of thickness:
image

The good thing is that it doesn't overlap with the text at the bottom with our fake covers.

Also, you might need to tweak the position or width depending on the cover size (large but not tall covers could get it distant from the cover bottom edge/border and tall but not large covers could get it overflowing the right edge (I have no such picture on my Kobo to test).

image

@uroybd
Copy link
Contributor Author

uroybd commented Mar 17, 2022

I have to employ the same logic I used for x position to y position also.

And yes, with no border-radius, same thickness and overlapping border might make this thing look great.

@uroybd
Copy link
Contributor Author

uroybd commented Mar 18, 2022

@poire-z I've incorporated some of your UI feedback. Here's the result:

Screenshot from 2022-03-18 10-38-18

However, I couldn't set the radius to 0. It makes ProgressWidget look weird. Extra border in left and right and all that. Like this:

Screenshot from 2022-03-18 10-38-59

plugins/coverbrowser.koplugin/mosaicmenu.lua Outdated Show resolved Hide resolved
plugins/coverbrowser.koplugin/mosaicmenu.lua Outdated Show resolved Hide resolved
Comment on lines 909 to 911
-- if progress_widget then
-- progress_widget:free()
-- end
Copy link
Contributor

Choose a reason for hiding this comment

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

Why comment out this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What happened is, if I try to resize emulator, this part actually throws an error saying free is nil. So, I assumed free is unavailable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right :) it seems ProgressWidget doesn't have and need any :free().

Copy link
Contributor

Choose a reason for hiding this comment

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

So, you can remove this commented out stuff.

@poire-z
Copy link
Contributor

poire-z commented Mar 18, 2022

However, I couldn't set the radius to 0. It makes ProgressWidget look weird.

Yep, dunno. May be you need some inner spacing (which I guess only radius provide), so it's noticable over a black bckground cover.
Try with radius = Size.border.thin (0.5, smallest we can get).

Extra border in left and right and all that. Like this:

Possible margin_h=3 that you could reduce or make 0 or 1 ? There are other tweakable things:

local ProgressWidget = Widget:new{
width = nil,
height = nil,
margin_h = Screen:scaleBySize(3),
margin_v = Screen:scaleBySize(1),
radius = Screen:scaleBySize(2),

@uroybd
Copy link
Contributor Author

uroybd commented Mar 18, 2022

margin_h = Screen:scaleBySize(3),

Tweaking margin_h didn't help much. However, the thin radius is much better. Now the corner roundness is almost unnoticable.

plugins/coverbrowser.koplugin/mosaicmenu.lua Outdated Show resolved Hide resolved
plugins/coverbrowser.koplugin/mosaicmenu.lua Outdated Show resolved Hide resolved
plugins/coverbrowser.koplugin/mosaicmenu.lua Outdated Show resolved Hide resolved
plugins/coverbrowser.koplugin/mosaicmenu.lua Show resolved Hide resolved
Comment on lines 909 to 911
-- if progress_widget then
-- progress_widget:free()
-- end
Copy link
Contributor

Choose a reason for hiding this comment

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

So, you can remove this commented out stuff.

@poire-z
Copy link
Contributor

poire-z commented Mar 18, 2022

Looks rather neat.

The little blank space on the left & right feels a bit odd. May be you can add
margin_h = Screen:scaleBySize(1) , same as margin_v, it would give this:

image
or may be Screen:scaleBySize(1.5) or Screen:scaleBySize(2) as it feels too small on this screenshot once zoomed in :) (might be some optical illusion).

One other case you might want (or not) to solve, or be sure it doesn't overflow too much over the book on the left (which I think it may if you use a really really thin image - if you made the progress bar 2/3, if the right is at 1/2, it may overflow):

image

Now that it's nearly ok, you may think about a new option in there :)
image

[ ] Show progress % in mosaic mode (or some better wording?)

And, if enabled, you would not show the dogear (you can keep it being sized/built, just not show it in paintTo).
And you would make these last 2 other options disabled (with enabled_func = function() return true if new setting not enabled end): May be not, they could apply to Detailed list mode if one doesn't use Mosaic mode everywhere.
image

@uroybd
Copy link
Contributor Author

uroybd commented Mar 19, 2022

@poire-z added the margin as you've suggested, and the settings also.

For progress bar width issue, My current strategy is to take min of 60% of the container and cover image width.

Copy link
Contributor

@poire-z poire-z left a comment

Choose a reason for hiding this comment

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

Looks quite neat.

plugins/coverbrowser.koplugin/mosaicmenu.lua Outdated Show resolved Hide resolved
@Frenzie Frenzie modified the milestones: 2022.04, 2022.03 Mar 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FR: Reading Progress in Mosaic view
3 participants