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

Update UI layout code for use of SVG icons #6977

Merged
merged 2 commits into from
Dec 19, 2020

Conversation

poire-z
Copy link
Contributor

@poire-z poire-z commented Dec 11, 2020

Don't trust the icons' native sizes: replace scale_for_dpi=true with width = height= DGENERIC_ICON_SIZE, so all icons get the same (tunable) size - except in a few specific use cases.

I've put DGENERIC_ICON_SIZE = 50 (updated to 48) to provoke a reaction :) as we need to decide a value for this.

More context around #6937 (comment)


This change is Reviewable

@poire-z poire-z added this to the 2021.01 milestone Dec 11, 2020
@Frenzie
Copy link
Member

Frenzie commented Dec 11, 2020 via email

@johnbeard
Copy link
Contributor

I recommend 48 as that's 1) a standard icon size and 2) easier to scale up and down to other common sizes.

@poire-z
Copy link
Contributor Author

poire-z commented Dec 11, 2020

  1. easier to scale up and down to other common sizes.

Does that matter at this point ? It's the target size, so no scale up/down (unless scaleBySize would magically round things nicely with 48 as input?).
You could keep your source at 144px - or do you plan to resize them at 48 ?

@johnbeard
Copy link
Contributor

You could keep your source at 144px - or do you plan to resize them at 48 ?

Well, if we're going to actually render at 48px, it makes more sense to edit the SVGs at that size because then they'll be sharper (4px at 144px rendered at 48px will always be anti-aliased and fuzzy, and any lines not on a multiple of 3 pixel grid will be fuzzy too).

Whether that's an issue depends on the device I think. On my Clara HD, it's pretty sharp anyway (might just be too small for my eyes). On the emulator (and Android maybe??) it's pretty fuzzy.

Plus, if we go to 48px and round up from 4/3 to 2px, then we get thicker lines.

@NiLuJe
Copy link
Member

NiLuJe commented Dec 11, 2020

The issue here is that the 48 @poire-z is talking about is not the 48px @johnbeard is talking about ^^.

That 48 is fed through scaleBySize, which will then spit out an actual pixel value, which will most likely never ever be 48px ;).

@johnbeard
Copy link
Contributor

Oh right, I though it was going to change to be actually 48px (or some other value). Ignore me then, it doesn't matter what the SVG size is :-D

@poire-z
Copy link
Contributor Author

poire-z commented Dec 11, 2020

Only on the emulator we'll render at 48px (may be only on my small one?). On my GloHD, I know my scale ratio: x 1.78667, so I'll have them at 85px.
(But I don't understand much about DPI and the scaling stuff, so I let you and @Frenzie take the right decisions :)

@poire-z
Copy link
Contributor Author

poire-z commented Dec 11, 2020

(But but but... can I still get thicker lines ? :)

@Frenzie
Copy link
Member

Frenzie commented Dec 11, 2020

I don't think it's a relevant concern outside of computers with low-res monitors and very old devices, and it's not very bad on those either.

(But but but... can I still get thicker lines ? :)

Something like 6px at 144 or 2 at 48 is fine by me. However, I wouldn't worry too much about what it looks like at 48 unless that happens to be exactly the value we like. Worry more about how it looks at 96 or 144. ;-)

@johnbeard
Copy link
Contributor

Are we talking thicker for all icons or just the appbar icons which render smaller (currently?)

Because if it's the latter, we should consider trying to figure out what the actual size ratio is so we can thicken those icons relative to the "normal" icons (e.g. zoom to page) so they come out at the same actual thickness on screen.

@Frenzie
Copy link
Member

Frenzie commented Dec 11, 2020 via email

@poire-z
Copy link
Contributor Author

poire-z commented Dec 11, 2020

which render smaller (currently?)

Smaller than what? Currently when ?
Because this PR is about making them all the same size: DGENERIC_ICON_SIZE (=48 currently).
(The zoom and direction one also, but these are allowed to have their size decreased to fit, only when needed.)

So, we all should test from now on with this PR applied on our emulator and devices.

@NiLuJe
Copy link
Member

NiLuJe commented Dec 11, 2020

Assuming you've not set a custom UI DPI, the scaleBySize formula is:

ceil( input * ( ( min(viewWidth, viewHeight) / 600 ) + ( min(viewWidth, viewHeight) / 600 ) ) / 2 )

Or, if you've set a custom UI dpi and it doesn't match the device's:

ceil( input * ( ( min(viewWidth, viewHeight) / 600 ) + ( dpi / 160 ) ) / 2 )

On a H2O (1080x1429, 265 DPI):

echo $(( 48 * ( (1080/600.) + (265/160.) ) / 2. )) = 82.95px -> 83px
echo $(( 48 * ( (1080/600.) + (1080/600.) ) / 2. )) = 86.4px -> 87px

On a Forma (1440x1920, 300 DPI):

echo $(( 48 * ( (1440/600.) + (300/160.) ) / 2. )) = 102.6px -> 103px
echo $(( 48 * ( (1440/600.) + (1440/600.) ) / 2. )) = 115.20px -> 116px

EDIT: Fixed the maths xD.

EDIT²: Again.

@NiLuJe
Copy link
Member

NiLuJe commented Dec 11, 2020

Now that I've stopped mangling the maths, the explanation: it was designed to be 1:1 on the original 6" Vizplex screens: 600x800 @ 167dpi.

(Especially compared to the 10.3" DX: 825x1200 @ 150dpi).

@poire-z
Copy link
Contributor Author

poire-z commented Dec 11, 2020

Or, if you've set a custom UI dpi and it doesn't match the device's:

Is there a logic about this distinction ?
So, if I use the default of 300, or if I set a custom DPI of 301, I would get really different results ?

GloHD or ClaraHD (1448 x 1072 @ 300dpi):

> print( 48 * (1072/600 + 1072/600) / 2 ) -- with device dpi ("auto")
85.76
> print( 48 * (1072/600 + 301/160) / 2 ) - custom dpi of 301
88.03

> print( 48 * (1072/600 + 300/160) / 2 ) -- what we'd get with using device dpi of 300
87.88

@NiLuJe
Copy link
Member

NiLuJe commented Dec 11, 2020

Or, if you've set a custom UI dpi and it doesn't match the device's:

Is there a logic about this distinction ?

I'm as puzzled as you are by that bit of logic every time I look at it ^^. (Which is why I almost always get it wrong each time I try to explain it ^^).

It possibly coulda maybe have made sense at the time, but it's certainly fishy nowadays.

@NiLuJe
Copy link
Member

NiLuJe commented Dec 11, 2020

Rabbit hole:

@poire-z
Copy link
Contributor Author

poire-z commented Dec 11, 2020

So, I guess it's time to make it right, if we are going to resize things around a bit :)

@NiLuJe
Copy link
Member

NiLuJe commented Dec 11, 2020

Let's see what @Frenzie has to say about it, but I'd tend to agree, yeah ;).

@poire-z
Copy link
Contributor Author

poire-z commented Dec 11, 2020

There's a little top vs bottom menu bar height difference, because of this span:

self.height = icon_height + Size.span.vertical_large

image

With:
image

Without:
image

So, we have to decide whether to kill that span at top, or add it at bottom.

Also, this PR does not include the line thichness reduction from #6937 (comment) .
That's another option to decide.

@Frenzie
Copy link
Member

Frenzie commented Dec 11, 2020

So, if I use the default of 300, or if I set a custom DPI of 301, I would get really different results ?

While quite unlikely for a minor step like the example given, it's certainly possible that all kinds of things could suddenly gain an extra pixel. Your 2px line turns into a 3px line at some point.

Setting aside the scaleBySize part of the equation:

  • 1px at 160
  • turns into 2px at 240 DPI
    1*240/160 = 1.5 → 2px
  • 2px at 320
  • turns into 3px at… (and so forth)

So, I guess it's time to make it right, if we are going to resize things around a bit :)

Make what right, precisely?

@Frenzie
Copy link
Member

Frenzie commented Dec 11, 2020

Note that we purposefully play with the above. 0.5px at 160 = 1px, all the way up to far above 320 dpi. It then makes 160 DPI a bit fatter than the look we're actually going for.

@Frenzie
Copy link
Member

Frenzie commented Dec 11, 2020

Make what right, precisely?

Oh, I see what you mean. No, I think it's probably an oversight in custom DPI handling unless comments/git blame have anything pertinent to say.

@Frenzie
Copy link
Member

Frenzie commented Dec 11, 2020

Without:

Looks a bit unbalanced to me.

@NiLuJe
Copy link
Member

NiLuJe commented Dec 11, 2020

So, we have to decide whether to kill that span at top, or add it at bottom.

I'd be moderately inclined to have it in both menus, but no super strong opinion about it (e.g., keeping the bottom menu as small as possible has its merits, although we are talking about a very few pixels here, so, eh ;p).

@poire-z
Copy link
Contributor Author

poire-z commented Dec 11, 2020

There's a little top vs bottom menu bar height difference

OK, fixed. We can have both menu bars have balanced similar padding, here with Size.padding.default (that we can reduce if too much):
image

@poire-z
Copy link
Contributor Author

poire-z commented Dec 11, 2020

I think, now with the sample blacker icons at 6px strokes, that I prefer our current smaller size of DGENERIC_ICON_SIZE = 40 to the 48 that I put to get larger and thicker icons and counteract their grayness.
image
(we just get them in the Zoom panel at 40, vs 50 before)

But as it's now configurable, one can have them to his taste. We just have to decide for a nice default.

@Frenzie
Copy link
Member

Frenzie commented Dec 12, 2020

Yup, let's just stick to the same old size. ;-)

@poire-z
Copy link
Contributor Author

poire-z commented Dec 19, 2020

OK. Now other comment, can I merge it?

@poire-z
Copy link
Contributor Author

poire-z commented Dec 19, 2020

Oups, some small cleanup in Makefile needed (I only rgrep'ed for stuff in subdirectories...).

@Frenzie
Copy link
Member

Frenzie commented Dec 19, 2020

Now other comment, can I merge it?

Actually I noticed you're also removing the source files for the old icons. Please don't! ^_^

@Frenzie
Copy link
Member

Frenzie commented Dec 19, 2020

I'd like to git mv them into an appropriate classic folder (and replace the seemingly random YAMLs by the relevant SVGs).

@poire-z
Copy link
Contributor Author

poire-z commented Dec 19, 2020

Actually I noticed you're also removing the source files for the old icons. Please don't! ^_^

You really want me to let them?
(I've been just removing the exclusion from zip and rm of icons/src :)

git mv them into an appropriate classic

You'll have to rename a few of them to give them their new names.

@Frenzie
Copy link
Member

Frenzie commented Dec 19, 2020

You really want me to let them?

Yup! When I said PNG I meant PNG. :-)

@poire-z
Copy link
Contributor Author

poire-z commented Dec 19, 2020

OK, done.

@Frenzie
Copy link
Member

Frenzie commented Dec 19, 2020

For example, here's what it looks like in GNOME Shell (I think the icon's drawn at ~ 300 × 300):
Screenshot_2020-12-19_13-43-59

Copy link
Member

@Frenzie Frenzie left a comment

Choose a reason for hiding this comment

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

I didn't read/test the alpha stuff in detail but seems fine while skimming.

@poire-z
Copy link
Contributor Author

poire-z commented Dec 19, 2020

Not much alpha stuff changed, but @NiLuJe will soon re tweak all that :)

@NiLuJe : one little useless thing to check anyway for your blit/alpha stuff changes: the alphacontainer, that is only used with PDF by readerview for its arrow - when Page overlap > Arrow is used instead of Gray out:

image

It's quite crappy, I remember I spent quite some time last week as the SVG stuff made it no more transparent. But well, rarely used, not important.

@pazos
Copy link
Member

pazos commented Dec 19, 2020

@poire-z: I tried to submit a PR against your branch but I cannot locate your fork as a base.

Anyway, it was to move platform specific resources outside shared icons:

diff --git a/Makefile b/Makefile
index 453757ef..17722647 100644
--- a/Makefile
+++ b/Makefile
@@ -383,7 +383,6 @@ androidupdate: all
 		-xr!*NOTES.txt$ \
 		-xr!*NOTICE$ \
 		-xr!*README.md$ \
-		-xr!*koreader.icns$ \
 		-xr'!.*'
 
 	# make the android APK
@@ -427,7 +426,7 @@ macosupdate: all
 		$(INSTALL_DIR)/bundle/Contents/MacOS \
 		$(INSTALL_DIR)/bundle/Contents/Resources
 
-	cp resources/koreader.icns $(INSTALL_DIR)/bundle/Contents/Resources/icon.icns
+	cp $(MACOS_DIR)koreader.icns $(INSTALL_DIR)/bundle/Contents/Resources/icon.icns
 	cp -LR $(INSTALL_DIR)/koreader $(INSTALL_DIR)/bundle/Contents
 	cp -pRv $(MACOS_DIR)/menu.xml $(INSTALL_DIR)/bundle/Contents/MainMenu.xib
 	ibtool --compile "$(INSTALL_DIR)/bundle/Contents/Resources/Base.lproj/MainMenu.nib" "$(INSTALL_DIR)/bundle/Contents/MainMenu.xib"
diff --git a/platform/mac/do_mac_bundle.sh b/platform/mac/do_mac_bundle.sh
index 02fcb400..933ab349 100755
--- a/platform/mac/do_mac_bundle.sh
+++ b/platform/mac/do_mac_bundle.sh
@@ -160,8 +160,7 @@ rm -rf cache clipboard history ota \
     l10n/.git l10n/.tx l10n/templates l10n/LICENSE l10n/Makefile l10n/README.md \
     plugins/SSH.koplugin plugins/hello.koplugin plugins/timesync.koplugin \
     plugins/autofrontlight.koplugin resources/fonts resources/icons/src \
-    resources/kobo-touch-probe.png resources/koreader.icns rocks/bin \
-    rocks/lib/luarocks screenshots spec tools
+    rocks/bin rocks/lib/luarocks screenshots spec tools
 
 # adjust reader.lua a bit.
 
diff --git a/resources/kobo-touch-probe.png b/resources/kobo-touch-probe.png
deleted file mode 100644
index d34260f7..00000000
Binary files a/resources/kobo-touch-probe.png and /dev/null differ
diff --git a/resources/koreader.icns b/resources/koreader.icns
deleted file mode 100644
index 97dd63f0..00000000
Binary files a/resources/koreader.icns and /dev/null differ
diff --git a/tools/kobo_touch_probe.lua b/tools/kobo_touch_probe.lua
index 7e2cf675..673ec778 100755
--- a/tools/kobo_touch_probe.lua
+++ b/tools/kobo_touch_probe.lua
@@ -51,7 +51,7 @@ function TouchProbe:init()
         },
     }
     self.image_widget = ImageWidget:new{
-        file = "resources/kobo-touch-probe.png",
+        file = "tools/kobo-touch-probe.png",
     }
     local screen_w, screen_h = Screen:getWidth(), Screen:getHeight()
     local img_w, img_h = self.image_widget:getSize().w, self.image_widget:getSize().h

@poire-z
Copy link
Contributor Author

poire-z commented Dec 19, 2020

OK, I'm applying it, thanks.

@poire-z poire-z requested a review from pazos December 19, 2020 13:20
@poire-z
Copy link
Contributor Author

poire-z commented Dec 19, 2020

(Just changed your cp $(MACOS_DIR)koreader.icns to cp -pv $(MACOS_DIR)/koreader.icns - and renamed the files to their new locations.)

Also move Mac specific resource in platform/mac/.
@poire-z poire-z merged commit daefdc9 into koreader:master Dec 19, 2020
@poire-z poire-z deleted the icon_layout_tweaks_for_svg branch December 19, 2020 13:49
@poire-z
Copy link
Contributor Author

poire-z commented Dec 19, 2020

@NiLuJe : I haven't thought/I forgot about the inverted colors in nightmode:
image image
I hope your yesterday patch takes care of that :)

@Frenzie
Copy link
Member

Frenzie commented Dec 19, 2020

@poire-z Did you delete one too many? False alarm, it came from a "private" plugin. (Nothing to it, just a quick investigation into a DB search interface.)

12/19/20-16:02:21 DEBUG cache image|resources/icons/dogear.png|23|23
./luajit: frontend/ui/widget/imagewidget.lua:191: attempt to index field 'bb' (a nil value)
stack traceback:
	frontend/ui/widget/imagewidget.lua:191: in function '_loadfile'
	frontend/ui/widget/imagewidget.lua:208: in function '_render'
	frontend/ui/widget/imagewidget.lua:313: in function 'getSize'
	plugins/coverbrowsersearch.koplugin/listmenu.lua:807: in function 'paintTo'
	frontend/ui/widget/verticalgroup.lua:48: in function 'paintTo'
	frontend/ui/widget/verticalgroup.lua:52: in function 'paintTo'
	frontend/ui/widget/overlapgroup.lua:82: in function 'paintTo'
	frontend/ui/widget/container/framecontainer.lua:96: in function 'paintTo'
	frontend/ui/widget/container/inputcontainer.lua:85: in function 'paintTo'
	frontend/ui/widget/verticalgroup.lua:48: in function 'paintTo'
	frontend/ui/widget/container/framecontainer.lua:96: in function 'paintTo'
	frontend/ui/widget/container/inputcontainer.lua:85: in function 'paintTo'
	frontend/ui/uimanager.lua:1089: in function '_repaint'
	frontend/ui/uimanager.lua:1235: in function 'handleInput'
	frontend/ui/uimanager.lua:1312: in function 'run'
	./reader.lua:306: in main chunk
	[C]: at 0x55fd5e4de791

@NiLuJe
Copy link
Member

NiLuJe commented Dec 19, 2020

@poire-z: Nope, colors have to be inverted or the B&W stuff doesn't work ;).

(You'd need to single out actually color icons, don't composite 'em, and treat them almost like classic images, but not quite, i.e., with an extra invert in NM before blending. Which would then break highlighting for those ;). And needs a copy to not affect the cache).

@poire-z
Copy link
Contributor Author

poire-z commented Dec 19, 2020

Did you delete one too many?

You probably did git pull and forgot to ./kodev something like build to have plugins/* copied into koreader-emulator/koreader/plugins/ (which happens to me quite often - and is a bit bothering when modifying plugins).

@Frenzie
Copy link
Member

Frenzie commented Dec 19, 2020

You probably did git pull and forgot to ./kodev something like build to have plugins/* copied into koreader-emulator/koreader/plugins/ (which happens to me quite often - and is a bit bothering when modifying plugins).

I always use ./kodev run (which does build) actually. It takes only a split second. :-) It's only for Android that I might avoid it. But it's a false alarm because it's just local due to the giant icon name/calling changes.


-- Directories to look for icons by name, with any of the accepted suffixes
local ICONS_DIRS = {}
local user_icons_dir = DataStorage:getSettingsDir() .. "/icons"
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 guess this should be DataStorage:getDataDir() instead of DataStorage:getSettingsDir().
(koreader/icons/ instead of koreader/settings/icons/)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it doesn't seem to have much to do with settings.

Frenzie pushed a commit that referenced this pull request Dec 20, 2020
Should be `DataStorage:getDataDir()` instead of `DataStorage:getSettingsDir()`.
(`koreader/icons/` instead of `koreader/settings/icons/`) #6977 (comment)
roygbyte pushed a commit to roygbyte/koreader-base that referenced this pull request Mar 3, 2022
* Make sure scaleBySize *always* honors a DPI override, even if it's set to
the native DPI...

As discussed in koreader/koreader#6977

* Allow clearing a DPI override

Useful when switching from a custom DPI to the native one.

* Plug clearDPI into setDPI if it gets a nil

(As that's what front does)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants