[UX] Smaller menu icons by Frenzie · Pull Request #3253 · koreader/koreader

Frenzie · 2017-09-23T19:01:12Z

This is to get discussion started but it does reflect my input.

Small screen before/after

Kobo Aura One before/after

Edited to add screenshots after @poire-z's comment.

poire-z · 2017-09-23T19:24:19Z

    local spacing_width = (self.width - content_width)/(#self.icons*2)
-    local icon_padding = math.min(spacing_width, Screen:scaleBySize(20))
-    self.height = tmp_ib:getSize().h + Screen:scaleBySize(10)
+    local icon_padding = math.min(spacing_width, Size.span.horizontal_default)


Would look fine (= as before) to me on kobo with a Size.span.horizontal_menuicons = scaleBySize(16) (instead of horizontal_default = Screen:scaleBySize(10))

I thought 20 looked on the large side but I suppose half that might indeed be a bit small.

Should it be in Size? This seems like pretty much a one-off value to me. For the bottom menu perhaps?

Dunno, depend if that same value plays well in the bottom menu too. Same for your icon_width = Screen:scaleBySize(40).
And if it happens it works as well on the InfoMessage icon and others alike, it may well fit in Size.
May be with a specific key (but it mess with your existing hierarchy padding > icons):

Size.icons = { horizontal_pading = Screen:scaleBySize(16), width = Screen:scaleBySize(40), ... }

robert00s · 2017-09-23T19:34:28Z

I think that larger icons (before) are better. It will be easier to tap.

Frenzie · 2017-09-23T19:39:31Z

My screenshots were a bit lazily executed and therefore deceptive. As @poire-z pointed out I should just keep the padding pretty much the way it is, with scaleBySize(16).

Current Kobo Aura One

After Kobo Aura One (=actually larger icons, this time with padding 16)

poire-z · 2017-09-23T19:51:39Z

I think that larger icons (before) are better. It will be easier to tap.

Actually, since a PR i made 1 or 2 weeks ago, they should already be easier to tap: you can now tap on the padding, the blank space in between them, and the nearest one should react.
But you should check with tomorrow's nightly how these big icons feel on your device (they feel less monstruous to me on the screenshots i made on the device and posted here, than they do on the device itself :)

But I dunno, may be some people will actually like this big look.

Frenzie · 2017-09-23T19:59:05Z

This feels right to me now (after the padding adjustment).

And yes, while fairly insignificant on the surface, for me #3213 is one of the biggest improvements of the year. I hadn't even realized how often I occasionally mistapped.

Edit:

But you should check with tomorrow's nightly how these big icons feel on your device (they feel less monstruous to me on the screenshots i made on the device and posted here, than they do on the device itself :)

We can do both. Merge this change for the top menu, keep the bottom menu as is (i.e., monstrous) for comparison.

Edit 2: Alright, I merged it. Since this isn't a monster change it'll be easy to adjust if desired. :-)

poire-z · 2017-09-23T20:05:19Z

What about bottom menu (widget/configdialog.lua) ? :)
edit: oups, missed your edit

Frenzie · 2017-09-23T20:07:40Z

I don't have anything prepared. Feel free to create a quick PR, or like I said in my edit we could keep it for the next nightly so it'd be easy for everyone to see the two sizes side by side.

poire-z · 2017-09-23T20:21:38Z

(no quick PR either from me this evening, so let's keep it)

robert00s · 2017-09-24T10:10:03Z

Thanks for great work. But icons only in scaleBySize(72) looks nice (shape)
local icon_width = Screen:scaleBySize(72)

local icon_width = Screen:scaleBySize(48)

local icon_width = Screen:scaleBySize(40)

Frenzie · 2017-09-24T10:13:57Z

Indeed, the sizing down algorithm isn't exactly the nicest. I only "intended" the sized up icons to look suboptimal in the short term.

robert00s · 2017-09-24T11:11:51Z

Is there any chance to fix algorithm? Or maybe we should use unicode symbols instead of icons...

Frenzie · 2017-09-24T11:38:32Z

The path I've had in mind for the past half year or so is to bump MuPDF. In that case it should hopefully be more or less as simple as having the MuPDF renderImageFile() function work directly on the SVG files.

But even without that I'm sure MuPDF must have all kinds of fancy pixmap scaling algorithms.

As you can see this icon scaled down with a cubic algorithm from 64x64 to 40x40 in Gimp looks just fine:

(For that matter, sizing it up to 120x120 looks better, too.)

And linear:

I'm not sure where things go "wrong". The default (?) algorithm might be optimized for text legibility or some such.

poire-z · 2017-09-24T12:05:33Z

Looks like the scaling is not done by mupdf...

koreader/frontend/ui/widget/imagewidget.lua

Line 137 in 88aba42

bb = Mupdf.renderImageFile(self.file, self.width, self.height),

Even if one provides self.width and self.height = 40 to this, we get a bb of size 64x64.
And the scaling is done with:

koreader/frontend/ui/widget/imagewidget.lua

Line 206 in 88aba42

new_bb = self._bb:scale(self.width, self.height)

which "uses very simple nearest neighbour scaling":
https://github.com/koreader/koreader-base/blob/82d9eab8bae9e3e7737a01827362f9a063bd4a83/ffi/blitbuffer.lua#L763-L766
(I don't get why sometimes github includes the code snippet, and sometimes does not...)

edit: mupdf may ignore the width and height given to the internal function we are using (except not may be for svg)
https://github.com/robamler/mupdf-nacl/blob/c0eaa04dfda9ac8db8e879101a0a0fe5b6f8630b/include/mupdf/fitz/image.h#L22-L37 (well, that's not our project, but our mupdf code has the same comment)

Frenzie · 2017-09-24T12:44:56Z

@poire-z Thanks for the closer look. I misremembered and/or assumed incorrectly. In that case the fix should be rather simple, at least theoretically because I haven't looked at the how or what: get a scaled image out of MuPDF.

MuPDF uses "an algorithm based on a Graphics Gem when downscaling images, and simple bilinear interpolation when magnifying images" (source).

(I don't get why sometimes github includes the code snippet, and sometimes does not...)

Looks to me like the code snippets that work are from this repo and the ones that don't are from another (such as base).

poire-z · 2017-09-24T12:48:11Z

Simplest would be to implement bilinear or such in pure lua in ffi/blitbuffer.lua BB_mt.__index:scale().
But I can not find such existing code to just cut & paste, and I hate that kind of algorithmic mathematical coding, so I won't be the one doing it :)
ps: working on bottom menu smaller icons

Frenzie · 2017-09-24T13:04:22Z

(well, that's not our project, but our mupdf code has the same comment)

Upstream MuPDF has changed around a bit there.

Simplest would be to implement bilinear or such in pure lua in ffi/blitbuffer.lua BB_mt.__index:scale().

For some value of simple. I'm not sure if writing a half-performant bilinear interpolation filter is particularly simple, even setting aside future concerns. :-P

ps: working on bottom menu smaller icons

Cool, thanks. :-)

poire-z · 2017-09-24T14:45:10Z

If you guys could check for this koreader-base koreader/koreader-base#517 (2 lua files patchable manually in ffi/), if you actually get better icons at various sizes/dpi.
(I'm not really sure of what I was doing, but the function in mupdf was called fz_scale_pixmap, so I just tried it :)

[UX] Smaller menu icons

ca7e489

Frenzie added the UX label Sep 23, 2017

poire-z reviewed Sep 23, 2017

View reviewed changes

larger padding

5e221c2

Frenzie changed the title ~~[WIP, UX] Smaller menu icons~~ [UX] Smaller menu icons Sep 23, 2017

Frenzie merged commit 3647826 into koreader:master Sep 23, 2017

Frenzie deleted the smaller_menu_buttons branch September 23, 2017 20:01

poire-z mentioned this pull request Sep 24, 2017

[UX] Smaller bottom menu icons #3257

Merged

poire-z mentioned this pull request Sep 24, 2017

Add proper scaling to mupdf.renderImage() koreader/koreader-base#517

Merged

Uh oh!

Conversation

Frenzie commented Sep 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Small screen before/after

Kobo Aura One before/after

Uh oh!

poire-z Sep 23, 2017

Choose a reason for hiding this comment

Uh oh!

Frenzie Sep 23, 2017

Choose a reason for hiding this comment

Uh oh!

poire-z Sep 23, 2017

Choose a reason for hiding this comment

Uh oh!

robert00s commented Sep 23, 2017

Uh oh!

Frenzie commented Sep 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current Kobo Aura One

After Kobo Aura One (=actually larger icons, this time with padding 16)

Uh oh!

poire-z commented Sep 23, 2017

Uh oh!

Frenzie commented Sep 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poire-z commented Sep 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Frenzie commented Sep 23, 2017

Uh oh!

poire-z commented Sep 23, 2017

Uh oh!

robert00s commented Sep 24, 2017

Uh oh!

Frenzie commented Sep 24, 2017

Uh oh!

robert00s commented Sep 24, 2017

Uh oh!

Frenzie commented Sep 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poire-z commented Sep 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Frenzie commented Sep 24, 2017

Uh oh!

poire-z commented Sep 24, 2017

Uh oh!

Frenzie commented Sep 24, 2017

Uh oh!

poire-z commented Sep 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Frenzie commented Sep 23, 2017 •

edited

Loading

Frenzie commented Sep 23, 2017 •

edited

Loading

Frenzie commented Sep 23, 2017 •

edited

Loading

poire-z commented Sep 23, 2017 •

edited

Loading

Frenzie commented Sep 24, 2017 •

edited

Loading

poire-z commented Sep 24, 2017 •

edited

Loading