BottomMenu: show real sizes for margins and font size · Pull Request #9205 · koreader/koreader

ghost · 2022-06-12T17:16:01Z

Pictures say more than words:

This change is

poire-z · 2022-06-12T17:50:27Z

Fine by me.
(I must say that pt for font size may talk to me because of reading about fonts - but for margins it will feel a bit abstract. May be it will talk more to people with a designer/illustration background.)

Frenzie · 2022-06-12T18:08:41Z

Indeed, for margins cm (or I suppose inch) would be more natural.

@zwim

scaleByPt

I think you mean something like (convert)ToPt.

poire-z · 2022-06-12T18:15:48Z

On the other hand, something a bit abstract like pt feels better than "cm" if the user has changed its Screen DPI.
@zwim: are you using the user-set Screen DPI ? Or the device.lua harcoded value for each device (and I don't know if this is always accurate, like on Android - do we get the correct real value)? in which case, cm will be real centimeters that the user can measure to confirm we're not lying :)

(inch wouldn't talk to me at all :)

ghost · 2022-06-12T18:26:59Z

I think you mean something like (convert)ToPt.

This name is a leftover from my previous PR, scaleBySize and scaleByPt do a smilar job. But the name can be changed.

Frenzie · 2022-06-12T18:27:04Z

On the other hand, something a bit abstract like pt feels better than "cm" if the user has changed its Screen DPI.

It'll be equally inaccurate. :-)

Or the device.lua harcoded value for each device

I don't think we have access to that — at least not over there in the framebuffer code at this time?

ghost · 2022-06-12T18:28:55Z

(inch wouldn't talk to me at all :)

Anyway if we support cm someone will request inch and vice versa.

Frenzie · 2022-06-12T18:29:03Z

This name is a leftover from my previous PR, scaleBySize and scaleByPt do a smilar job. But the name can be changed.

I'm not entirely sure what you're saying here. If they do a similar job then there should be no such thing as scaleByPt, ever. ;-)

ghost · 2022-06-12T18:34:50Z

@Frenzie I meant both take a px value and calculate the result. scaleBySize converts a px to your koscale(tm), scaleByPt converts a px value to a pt value.

ghost · 2022-06-12T18:36:56Z

@poire-z So far I have tested it on the emulator with the "-d dpi" option, to assure the correctness of the calculated sizes.

Frenzie · 2022-06-12T19:17:05Z

"koscale" is the intended display size it's scaled to. It can change completely tomorrow.

poire-z · 2022-06-12T19:24:21Z

I don't think we have access to that — at least not over there in the framebuffer code at this time?

Looks like we do, as self.device.display_dpi in base/ffi/framebuffer.lua.
And on Android, it's:

frontend/device/kobo/device.lua:    display_dpi = 300,
frontend/device/kobo/device.lua:    display_dpi = 300,
frontend/device/android/device.lua:    display_dpi = android.lib.AConfiguration_getDensity(android.app.config),
frontend/device/pocketbook/device.lua:    display_dpi = 200,
frontend/device/pocketbook/device.lua:    display_dpi = 212,
frontend/device/pocketbook/device.lua:    display_dpi = 167,

In the code, there's a check that hints it's possible there is no self.device:
https://github.com/koreader/koreader-base/blob/7548930c384227139ca506ab87ff006e50e84634/ffi/framebuffer.lua#L340-L356

So, i guess @zwim new formula should use this one if we want to show a value in absolute units like pt ( or pt for fonts and cm for margins?) that won't change when the user tweak Screen dpi to just get bigger/smaller stuff - with something to pick another value when no self.device.

On the other hand, something a bit abstract like pt feels better than "cm" if the user has changed its Screen DPI.

It'll be equally inaccurate. :-)

Yes, but with cm, anybody can realize it is innacurate - with pt only professionals will know :)

Frenzie · 2022-06-12T19:39:37Z

Looks like we do, as self.device.display_dpi in base/ffi/framebuffer.lua.

Indeed you're right. But then should we also default that to 160 if it doesn't exist?

Edit: I mean you'd need something like:

local display_dpi = self.device and self.device.display_dpi or 160

In this convertToPt function.

poire-z · 2022-06-12T19:47:36Z

But then should we also default that to 160 if it doesn't exist?

No dpinion :)
(But may be the -d emulator option should then be favored ? as it may only be used on the emulator/appimage - with no garantee it's the real dpi of the screen)

Frenzie · 2022-06-12T19:49:51Z

The emulator -d option maps to display_dpi, doesn't it? I'm talking about the scenario where one uses the base framework without the KOReader frontend.

ghost · 2022-06-14T16:21:35Z

Updated the screenshots in the initial commit, got rid of the helper in framebuffer.

Calculations for size are tested with a hardcopy (document with the same font and size in pt shown by KOReader) on two documents.

Not tested on Android yet.

poire-z · 2022-06-14T18:17:44Z

+    text = _("Metric system"),
+    checked_func = function()
+        return G_reader_settings:readSetting("metric_system", true)
+    end,


Do we really have to have a setting for this ? for the 2 nearly hidden places that show some size ?
I'd say that if I don't know how long is a "inch", for lack of ever seeing this unit, I guess all our users over the world (but may be not their blind grandma) should have a grasp of mm, cm, meters and understand what it means/sizes when seeing it).

I can comment out the menu entry. We can delete it if nobody requests it in lets say two releases :)
(As a physicist the Système international d’unité is the inly one to take care of :) )

'Murica!

(Snark aside, yeah, it might make sense to have a toggle available somewhere ;o). It's not quite as widespread and opinionated as the 12H vs. 24H clock issue, but, still ;p).

Let's @Frenzie have the final word.
(If kept, may be move the menu item nearer to the other unit one/ones like 12/24h.)

We should have a setting for it, unless we just show both. Default to metric please. ;-)

Yepp, default to metric is clear.
Should I put the menu entry into "Time and Date" (as the last entry there) and rename that entry to "Time, date and units"?

No strong opinion. But I'd say leave Time and date> as-is, and add a Units> submenu, with only this one for now (I can't rule out that you will one day come up with the idea of showing frontlight levels in lux or lumens, and device temperature in °C or °F :)
(Is [x] Metric system unambigous enough to indicates it's only about sizes/distances ? Is "metric" about meters and distances, or about metrics and other general units ?)

ghost · 2022-06-22T16:27:09Z

The menu looks like that now:

poire-z · 2022-06-25T20:57:47Z

@zwim: has my #9205 (comment) been considered ? (about using the hardcoded/real device dpi instead of the user set?)

ghost · 2022-06-26T07:28:01Z

@zwim: has my #9205 (comment) been considered ? (about using the hardcoded/real device dpi instead of the user set?)

First answer the question:
Not now?

Then ask a question:
What shall we do on devices, where there is no Device.display_dpi? Do we want to use Screen:getDPI() as a fallback?

poire-z · 2022-06-26T07:30:45Z

@Frenzie mentionned using 160 in the post just after mine.
I have no idea which is best, between 160 or using the user set dpi.

Frenzie · 2022-06-26T07:33:03Z

It needs to be both.

Frenzie · 2022-06-26T07:33:54Z

But technically speaking I suppose the user-set DPI overrides the fallback of 160? In that case it doesn't need to be both. But in any case it needs to be 160 as a fallback; it can't be nil.

ghost · 2022-06-26T07:36:49Z

I will go with

local display_dpi = Device.display_dpi or Screen:getDPI()

as Screen:getDPI() last fallback is 160.

Frenzie · 2022-06-26T07:38:34Z

I'm not sure if we want to read the property directly without a getter? But yes, that should be fine besides that.

poire-z · 2022-06-26T08:07:38Z

    self.input.gesture_detector:init()
 end

+function Device:getDisplayDPI()


May be it should be named getScreenRealDPI() (or something else instead of "real", @Frenzie ?)

getDeviceScreenDPI? Assuming it doesn't relate to the setScreenDPI function right above.

ghost · 2022-06-28T19:59:27Z

Ready?

poire-z · 2022-06-28T20:14:21Z

+        if is_size == 1 then
+            unit = G_reader_settings:nilOrTrue("metric_length") and "mm" or "\""
+        else
+            unit = "pt"


May be here and in the callers, except of 1 and 2, use "pt" or "mm" and if size_type == "mm" elseif size_type == "pt" (event if you end up using inch because of the preference).

poire-z · 2022-06-29T15:50:03Z

-        if is_size == 1 then
-            unit = G_reader_settings:nilOrTrue("metric_length") and "mm" or "\""
-        else
+        if is_size == "pt" then


May be the variable should be renamed to "unit" or "size_type" now (as is_size sounds like it should be a boolean true/false).

Yes, this will make the code shorter and more readable

ghost · 2022-07-04T20:16:03Z

Ready?

poire-z · 2022-07-30T10:22:55Z

@zwim : you're the scientist, but are you really sure a contrast can be expressed in millimeters ? :)

This should fix #9205 (comment)

This should fix koreader#9205 (comment)

poire-z reviewed Jun 14, 2022

View reviewed changes

aoarta added 5 commits June 24, 2022 22:41

Show pt values in bottom menu hold functions

dc75177

Now for real

2961c18

missed one

c827a70

Submenu for units

1810bf3

x

ea53523

Use display_dpi if available

a93eed3

poire-z reviewed Jun 26, 2022

View reviewed changes

Rename

9f7db5b

poire-z reviewed Jun 28, 2022

View reviewed changes

poire-z reviewed Jun 29, 2022

View reviewed changes

aoarta added 4 commits June 29, 2022 17:50

is_size use "pt" and "mm" instead of numbers

1d77c2b

Do the translations right now

6b9f573

Rename is_size to unit

126b064

Context: "Font size"

7898ce8

poire-z approved these changes Jul 4, 2022

View reviewed changes

Frenzie added this to the 2022.07 milestone Jul 5, 2022

Frenzie approved these changes Jul 5, 2022

View reviewed changes

ghost merged commit 2c952ec into koreader:master Jul 5, 2022

ghost deleted the showRealSizes branch July 5, 2022 13:39

ghost mentioned this pull request Jul 30, 2022

Bottom Menu: contrast has no units #9392

Merged

Frenzie pushed a commit that referenced this pull request Jul 30, 2022

Bottom Menu: fix contrast has no units (#9392)

70642ba

This should fix #9205 (comment)

ghost mentioned this pull request Jun 20, 2024

Q: "Screen DPI" vs physical font size #12058

Closed

NiLuJe mentioned this pull request Sep 11, 2024

Fix page gap inconsistencies between doc open & config change #12507

Merged

0xstillb pushed a commit to 0xstillb/koreader-thai that referenced this pull request May 9, 2026

BottomMenu: show real sizes for margins and font size (koreader#9205)

45f246a

0xstillb pushed a commit to 0xstillb/koreader-thai that referenced this pull request May 9, 2026

Bottom Menu: fix contrast has no units (koreader#9392)

3ddf2d0

This should fix koreader#9205 (comment)

This pull request was closed.

Uh oh!

Uh oh!

Conversation

ghost commented Jun 12, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poire-z commented Jun 12, 2022

Uh oh!

Frenzie commented Jun 12, 2022

Uh oh!

poire-z commented Jun 12, 2022

Uh oh!

ghost commented Jun 12, 2022

Uh oh!

Frenzie commented Jun 12, 2022

Uh oh!

ghost commented Jun 12, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Frenzie commented Jun 12, 2022

Uh oh!

ghost commented Jun 12, 2022

Uh oh!

ghost commented Jun 12, 2022

Uh oh!

Frenzie commented Jun 12, 2022

Uh oh!

poire-z commented Jun 12, 2022

Uh oh!

Frenzie commented Jun 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poire-z commented Jun 12, 2022

Uh oh!

Frenzie commented Jun 12, 2022

Uh oh!

ghost commented Jun 14, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NiLuJe Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Jun 22, 2022

Uh oh!

poire-z commented Jun 25, 2022

Uh oh!

ghost commented Jun 26, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poire-z commented Jun 26, 2022

Uh oh!

Frenzie commented Jun 26, 2022

Uh oh!

Frenzie commented Jun 26, 2022

Uh oh!

ghost commented Jun 26, 2022

Uh oh!

Frenzie commented Jun 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Jun 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Jun 12, 2022 •

edited by ghost

Loading

ghost commented Jun 12, 2022 •

edited by ghost

Loading

Frenzie commented Jun 12, 2022 •

edited

Loading

ghost commented Jun 14, 2022 •

edited by ghost

Loading

NiLuJe Jun 14, 2022 •

edited

Loading

ghost commented Jun 26, 2022 •

edited by ghost

Loading