-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
BottomMenu: show real sizes for margins and font size #9205
Conversation
Fine by me. |
Indeed, for margins cm (or I suppose inch) would be more natural.
I think you mean something like (convert)ToPt. |
On the other hand, something a bit abstract like ( |
This name is a leftover from my previous PR, scaleBySize and scaleByPt do a smilar job. But the name can be changed. |
It'll be equally inaccurate. :-)
I don't think we have access to that — at least not over there in the framebuffer code at this time? |
Anyway if we support cm someone will request inch and vice versa. |
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. ;-) |
@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. |
@poire-z So far I have tested it on the emulator with the "-d dpi" option, to assure the correctness of the calculated sizes. |
"koscale" is the intended display size it's scaled to. It can change completely tomorrow. |
Looks like we do, as 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: So, i guess @zwim new formula should use this one if we want to show a value in absolute units like
Yes, but with |
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. |
No dpinion :) |
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. |
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. |
text = _("Metric system"), | ||
checked_func = function() | ||
return G_reader_settings:readSetting("metric_system", true) | ||
end, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have a setting for it, unless we just show both. Default to metric please. ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?)
@zwim: has my #9205 (comment) been considered ? (about using the hardcoded/real device dpi instead of the user set?) |
First answer the question: Then ask a question: |
@Frenzie mentionned using 160 in the post just after mine. |
It needs to be both. |
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. |
I will go with
as |
I'm not sure if we want to read the property directly without a getter? But yes, that should be fine besides that. |
frontend/device/generic/device.lua
Outdated
@@ -238,6 +238,10 @@ function Device:setScreenDPI(dpi_override) | |||
self.input.gesture_detector:init() | |||
end | |||
|
|||
function Device:getDisplayDPI() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be it should be named getScreenRealDPI()
(or something else instead of "real", @Frenzie ?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getDeviceScreenDPI? Assuming it doesn't relate to the setScreenDPI function right above.
Ready? |
frontend/ui/data/optionsutil.lua
Outdated
if is_size == 1 then | ||
unit = G_reader_settings:nilOrTrue("metric_length") and "mm" or "\"" | ||
else | ||
unit = "pt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
frontend/ui/data/optionsutil.lua
Outdated
if is_size == 1 then | ||
unit = G_reader_settings:nilOrTrue("metric_length") and "mm" or "\"" | ||
else | ||
if is_size == "pt" then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this will make the code shorter and more readable
Ready? |
@zwim : you're the scientist, but are you really sure a contrast can be expressed in millimeters ? :) |
This should fix #9205 (comment)
Pictures say more than words:
This change is