@poire-z

But your changes looks the same on the emulator, but are way to big on my kobo:

That means that it's finally scaling as it should. To make them smaller one should look in TouchMenu or possibly IconButton imo.

This video review recently drew my attention to this issue, although of course I've always been aware that the buttons are a lot smaller on my H2O than on most other devices: https://www.youtube.com/watch?v=i2oSOeAnD10 But on the One it starts to verge into too small territory.

Edit: I actually like the size on my H2O, which is quite a bit smaller than the originally intended size.

Kindle 2, 3, 4, 5, Touch, 7, 8: 6 in diagonal, 600 × 800 pixels (0.48 megapixels),
167 PPI density, 16-level grayscale

Look familiar? That's also why the emulator defaults to what it does. ;-)

Quoting @poire-z

Well, I don't really get dpi arithmetics :)
But your changes looks the same on the emulator, but are way to big on my kobo:

(it looks more monstruous on the device that it does on this reduced screenshot)
Original, which I prefer:

I too prefer it more like this (screenshot from my H2O).

OK, I get it, you're just doing "scaling for dpi" rightly.

But scale_for_dpi = true is just used (for now) in iconbutton, and I think previously, the only thing it was doing was scaling the image by two if DPI > 330 or something, letting it as is if < 330.
So, may be ImageWidget needs an other attribute: scale_by_two_if_dpi_more_than_330 = true :)

Not sure about my current feeling: most people, like me, will get surprised with bigger buttons.
On the other hand, I sometimes miss the current buttons, and I have to tap a few times for them to react, so big buttons may help. But they take quite some room and make koreader looks like a kid's toy :|

Btw, which device (+resolution) do you have?

I'm thinking that if we simply stick something like a FrameContainer with some padding around the image (not yet sure whether in TouchMenu or IconButton), we can simply size it down to roughly how it looks on a Kobo Aura or a Kobo Aura HD without giving up on the extra touch space.

Incidentally, here's a link to relevant Android info for icons: https://developer.android.com/guide/practices/screens_support.html#qualifiers

• mdpi | Resources for medium-density (mdpi) screens (~160dpi). (This is the baseline density.)
• hdpi | Resources for high-density (hdpi) screens (~240dpi).
• xhdpi | Resources for extra-high-density (xhdpi) screens (~320dpi).
• xxhdpi | Resources for extra-extra-high-density (xxhdpi) screens (~480dpi).
• xxxhdpi | Resources for extra-extra-extra-high-density (xxxhdpi) uses (~640dpi).

The idea being that you generate icons in those sizes and then you scale down from the nearest one.

(Glo HD: 1072 x 1448 , 300 dpi.)
Yes, like divide available width / nb of icons, remove some padding => icon expected width ?
edit: or start with icon height = a % of available height (better with landscape mode I guess)
And may be the whole container should be tap'able, not just the icon, so we have less change missing them :)

I included a partial fix for SkimToWidget scaling issues.

Before

After

Edit: fixed up ProgressWidget as well. I imagine tick_width also be scaled though.

Boy, once you start paying attention…

Mentionning it here:
Latest nightly has this change you suggested 85e2140#diff-0201d559740bcbe905c8de5a73c4e0e3R32
which makes border of some widgets (confirmbox, infomessage, notification) bigger on my Kobo, and the whole balance of widget borders feels disrupted (some widgets are thin, others are thick).

May be it's a question of habbit, and I could get used to the bigger borders, but I kinda dislike them :) Too thick = feel like some kid's toy.
It feels like home reducing it : bordersize = Screen:scaleBySize(1), (scaleBySize(N) => N*1.8 on on Kobo), but then, it's on the emulator that things looks thiner :| (but the emulator is not my home, just my work place :).

Personally, when chosing sizes for my devs, I of course code on the emulator, but check and tweak in the end the look on my kobo. Dunno if others do the same, but, may be, if there is any absolute size that needs to be scaleBySize()'ed, may be it should be adjusted a bit lower than the original size (so scaleBySize() gets it back near the original value) ?

and the whole balance of widget borders feels disrupted (some widgets are thin, others are thick).

Are there any that I missed in this PR?

It feels like home reducing it : bordersize = Screen:scaleBySize(1), (scaleBySize(N) => N*1.8 on on Kobo), but then, it's on the emulator that things looks thiner :| (but the emulator is not my home, just my work place :).

Only the output needs to be an integer number of pixels; the input can be a float. I suspect you would like 1.5. And actually, so do I.

Are there any that I missed in this PR?

Many (with different values, 0, 1, 2, 3)...

frontend $ rgrep 'bordersize =' | grep -v '= 0' | grep scaleBySize | wc -l
5
frontend $ rgrep 'bordersize =' | grep -v '= 0' | grep -v scaleBySize | wc -l
51

But those that I saw with the latest nightly are just those that inherit the FrameContainer bordersize (changed from 2 to scaleBySize(2) in #3194).

But I feel no problem with the previous border sizes :)
If you're on the journey to homogenize them all, it will be a long journey :| First, we would need to decide which outside border size is the most pleasant on devices. And then, you'd have widgets with border against border, that would be too thick and need adjustment.

I suspect you would like 1.5. And actually, so do I.

1.5 looks fine on the emulator. Can't check how it looks on kobo before this evening.

If you're on the journey to homogenize them all, it will be a long journey :| First, we would need to decide which outside border size is the most pleasant on devices. And then, you'd have widgets with border against border, that would be too thick and need adjustment.

I don't see this PR as deciding anything. It's just fixing a few long-standing bugs. :-)

Edit: ah yes, something like grep -r --include=*.lua --exclude-dir=*arm* 'bordersize =' | grep -v '= 0' | grep -v scaleBySize is helpful.

1.5 looks ~ok on my Kobo (I'm sensitive to change :) so it's not a full OK! but I guess I could get used to that).

I don't see this PR as deciding anything. It's just fixing a few long-standing bugs. :-)

Well, by adding scaleBySize( original_value ), you're actually doubling bordersize on most devices, which is not fixing a long standing bug :)
I really fear your constrait to have them all scaleBySized :| (if I understood that correctly) cause it will obviously change a bit the look of most things.
I understand the will to have everything scaleBySize()'d by principle, but it's a big work. And if you're going to undertake it, may be you should define possible values elsewhere, and not use scaleBySize(2) in the widgets.
For example, in some ui/elements/commonui.lua:

MODAL_OUTSIDE_BORDER = scaleBySize(1.5)
MODAL_OUTSIDE_PADDING = scaleBySize(3)
NOTIFICATION_BORDER = scaleBySize(1)
CLASSIC_BUTTON_BORDER = scaleBySize(1)
IMPORTANT_BUTTON_BORDER = scaleBySize(3)

and use them directly in the widgets, so confirmbox, infomessage... would all use MODAL_OUTSIDE_*, so they would have a similar look.
Huge work ! But if we ever want to tweak some, we won't have to go thru each widget. And we could have different commonui.lua depending on dpi, sizes... and one could hack it to fit its liking.

I really fear your constrait to have them all scaleBySized :| (if I understood that correctly) cause it will obviously change a bit the look of most things.

Everything will finally look how it's supposed to look. And let me be clear: I'm not always the biggest fan of how it's supposed to look.

I understand the will to have everything scaleBySize()'d by principle, but it's a big work. And if you're going to undertake it, may be you should define possible values elsewhere, and not use scaleBySize(2) in the widgets.

Yes, that's one of the stretch goals. Kind of like the fonts in #2814. There are still a few remaining thorns in my eyevigilante fonts.

That being said, why should these be different at all?

MODAL_OUTSIDE_BORDER = scaleBySize(1.5)
NOTIFICATION_BORDER = scaleBySize(1)

Now that I've got all the problem areas tagged in this series of commits it's a lot easier to gain an overview of the whole shebang. I suppose I can do it all as one PR.

Everything will finally look how it's supposed to look.

We currently have some bordersize =1, 2, 3. I'm not sure the people who chose to put there 1, 2 or 3 really though the one they choose among 1, 2 or 3 was the one fitting in their context.
I came in late in the game :) so houqp and you never let me put a bordersize=2, you asked me to use scaleBySize(2), and well, that was too big on my kobo, so I changed it to scaleBySize(1), and that was fine enough, no discussion about why 1 and not 2.
People who put 1, 2, 3 didn't think much I guess, they choosed what looked best for them on their device or on the emulator. I'm a "i like it thin" person, but others may have like their thick.
I don't consider ScalingBySiz'ing them all "how it's supposed to look" :) I consider it "doubling how it actually looks" :)
So, it actually needs some thinking and rules, and thiking each bordersize/padding..., if the goal is to have a one-style coherent homogene UI.

That being said, why should these be different at all?
MODAL_OUTSIDE_BORDER = scaleBySize(1.5)
NOTIFICATION_BORDER = scaleBySize(1)

Personal feeling :) (I could get used to border=3 for real middle-of-the-screen modals like confirmbox and infomessage on my kobo, but it still looked too thick for the notification with 3 or 4 words in it at top of screen - edit: or may be it just need its padding=5 scaleBySized too). Anyway, I'll be more willing to adapt to any change if it's due to some thought rules (even if i disagree with them), than just to scaleBySize(what was already there).
There also may be some stuff that are fine with a thin line of 1 pixel width whatever the size of the screen is (for example, the border around the real or text covers in coverbrowser plugin - 2 looked too thick on kobo). A 1 pixel line is visible enough on my kobo.

Personal feeling :) (I could get used to border=3 for real middle-of-the-screen modals like confirmbox and infomessage on my kobo, but it still looked too thick for the notification with 3 or 4 words in it at top of screen - edit: or may be it just need its padding=5 scaleBySized too).

I don't mean "why should they be different than whatever they currently are" but rather "why should window border sizes be different than each other, these two in particular, but probably period."

@poire-z I've pushed a Size module proposal. Are there any objections or suggestions pertaining to the architecture I have in mind before I get started?

I don't think

Size.border -- returns default

is possible. Lua evaluates the stuff from left to right, so I don't think a metatable.__index function could know that another thing is going to be evaluated on the right by the time it has to decide what to do with Size.border.

But sure, having to explicitly specify

Size.border.default

is probably much simpler and preferable. When you put it like that, I'm not even sure why I bothered writing that much more complicated function. Perhaps mainly because it was fun. :-P

(Maybe Size.border() and Size.border.default would be simultaneously possible with __call or something but I doubt there's any upside to that.)

to get a default properties if property not found ?

Wait, what do you mean by that exactly?

If you mean

Size.border.supercalifragilisticexpialidocious

Then it'd probably be trivial to print a

logger.warn("The Spanish grain did not stand", k)

But would there be any upside in doing that?

Thought the main reason for your code was that Size.border("supercalifragilisticexpialidocious") would return Size.border.default , something rathen than nil.
But I guess may be we should have an error, or just let it be nil, like it is already for any unexistant attribute, dunno.
But a simple table like your size_def could indeed be enough :)

Well no, the point is the opposite (debug guard forced crash rather than regular mode nil) :p

Not entirely sure if I can sensibly implement that in metatable but it may not be too important.

@poire-z I do believe this to be quite a bit more elegant. Thank you.

if dbg.is_on then
    local logger = require("logger")
    local mt = {
        __index = function(t, k)
            local prop_value = rawget(t, k)
            local prop_exists = prop_value ~= nil
            if not prop_exists then logger.warn("Size: this property does not exist:", k) end
            assert(prop_exists)
            return prop_value
        end
    }
    setmetatable(Size, mt)
    for el, _ in pairs(Size) do
        setmetatable(Size[el], mt)
    end
end

Indeed :) (and a first time for me to see the how and why of using setmetatable)

TimeVal that we looked at the other day has some pretty fancy metatable stuff going on. :-P

Actually I've been vaguely curious to see if there's any performance difference in how the widgets are constructed.

We do stuff like this:

local MultiConfirmBox = InputContainer:new{
    modal = true,
-- yada yada
}

But it'll also work like this:

local MultiConfirmBox = {
    modal = true,
--yada yada
}
setmetatable(MultiConfirmBox, {__index = InputContainer})

Then it'll look in InputContainer for any property it doesn't have.

@poire-z This is basically finished but it'd be nice if you could take a quick look.

Quick looking good :) and thank you for the huge work (which means that we can start negotiating :), and there is only size.lua to update :)

Tested on my Kobo, and not as much visual changes as I feared: no real surprise related to padding or margin changes (am I right still thinking you did multiply by 2 most of the sizes on most devices, if you kept the original values and just added scaleBySize() to them ?).

But still, for my taste:

• Icons in top menu and bottom menu are too fat (seems even fatter than in my screenshot above)
  
  edit: may be the space between last menu item and bottom line with chevron and time could be smaller?
• The "circled i" image (that you had now scaled for dpi) in InfoMessage looks also too big, it takes useless width that is thus taken from the text, and the mass of black is distracting.
  
  edit: somehow, it adds some vertical padding to one-line text, which is nice - may be we could add an option to InfoMessage to display or not this "circled i" image, so it can be disabled for InfoMessage with lots of text (I use a few for personnal stuff)
• The window border (framecontainer.lua : bordersize = Size.border.window = Screen:scaleBySize(2)) feels too big to me (1.5 looked ok). It's a question of taste, but there is one thing more: when displayed on top of book page, the horizontal border, depending on how it aligns on a line of text, makes sometimes for some kind of inkblot, which does not look nice (never had that feeling for the week I spend with 1.5).
  
  
• The "chevron" (?) << < ^ ... in menus looks also big like a mass of black, but they are easier to tap than before (even if I seldom use them).
• The ◀◀ and ▶▶ in dictquicklookup also looks massive black, and a bit distracting, and they may contribute to having their button a bit highter than the others
  
• Dunno if it's a side effect of this, or just me confused: on the Table of Contents page, the page numbers on the right seems too near the right border, may be some right padding missing.
  • May be the same in Bookmarks, when you had highlighted a long text, after the truncated text, the ellipsis touch the right of the screen: some right padding missing.
  • Same in File browser in classic mode "4 items" and "3.5 MB" are too near the right of the screen
• Virtual Keyboard looks nicer.
• Not sure you changed something related to that, so it may be something else: when checking/unchecking some menu with checkmarks, like one of the items in Links, the refresh make the text or image under the menu appears for a fraction of second (was probably there before, just hadn't strike me...)

Apart from these personnal taste remarks, no problem noticed :)

Tested on my Kobo, and not as much visual changes as I feared: no real surprise related to padding or margin changes (am I right still thinking you did multiply by 2 most of the sizes on most devices, if you kept the original values and just added scaleBySize() to them ?).

I don't think you're quite right about "most devices" but insofar as they were legacy values from the early Kindle days that would be a logical result of the bug fix on your 300 DPI Kobo Aura I-forget-which and on my ~265 DPI H2O.

Icons in top menu and bottom menu are too fat (seems even fatter than in my screenshot above)

They are, but I think I'd still rather see such a big visual change as a separate PR than as part of this bug fix.

Btw, what would be the proper way to scale these images in ImageWidget? 'cause right now the assumption is that 64x64 at 800x600-ish (~160 DPI) is the proper size, but basically you and I want 64x64 to be the size at something closer to 250-300 DPI, which means downscaling to 40x40 (same for InfoMessage). But if I'm not mistaken, something as simple as passing scaleBySize(40) won't take aspect ratio into account. Now I think all of our icons are square, but still…

The window border (framecontainer.lua : bordersize = Size.border.window = Screen:scaleBySize(2)) feels too big to me (1.5 looked ok).

The funny thing about your remark is that it should actually be 3. :-P But yes, I'd like probably all borders to be thinner myself.

It's a question of taste, but there is one thing more: when displayed on top of book page, the horizontal border, depending on how it aligns on a line of text, makes sometimes for some kind of inkblot, which does not look nice (never had that feeling for the week I spend with 1.5).

Not really sure what you mean.

The "chevron" (?) << < ^ ... in menus looks also big like a mass of black, but they are easier to tap than before (even if I seldom use them).

I dislike them, and as indicated above I still think think it's besides the point. :-P These things should be torpedoed as far as I'm concerned, probably to be replaced by some very regular → type arrows. But like I said, I don't want to make a bug fix PR into a more contentious PR that actually changes things. Just look at your reaction to the one actual change I stuck in there. :-P

(I consider getting rid of all such typographical abuses bug fixes as well, but the specific way to go is less obvious and more open to interpretation.)

Did you give some of the related symbols a try? E.g., ◁◁ ▷▷, ◅◅ ▻▻.

they were legacy values from the early Kindle days that would be a logical result of the bug fix on your 300 DPI Kobo

It's a bug if you consider "all things should scale" a requirement :) (which may be it was and is, but it wasn't implemented fully, and that bug didn't feel like one). There may be a few that don't need to scale linearly.
Let's say I had a 600x800 device (I never had), and I bought a 1000x1400 one: ok, koreader looked fat and toyish on the 600x800, it now will look the same on my 1000x1400.
Before this bug fix, it looked thiner and more elegant, I considered for years this to be a feature, as with 1000x1400, we can do thiner stuff that we couldn't on 600x800.
(And we could do that just by using .5 values in scaleBySize)
So, ok for merging this PR.
But there should be soon another one to fix the visual changes on high resolution devices introduced by this bug fix :)

Btw, what would be the proper way to scale these images.

Dunno, I suck at DPI arithmetic. With that fact known and forgiven :) I would go that way:
we have at most 7 icons, we want some space between the last to last and the last one in the top menu. So let's say we need to fit 7.5 to 8 such icons in Screen.width. Get some scaling factor, do some bit of rounding so it scales well. Applies this factor to all original image sizes.

Not really sure what you mean.

By inkblot: when the 4px border happens to align with the baseline of the book text, it appears (like it does a bit on the screenshots above) like they melt, and that the border is not a straight line with a fixed stroke size and was drawn with a bad pencil without a ruler... (may depend on font size, but I never had that feeling when borders were 2 px or 3 px).

Did you give some of the related symbols a try? E.g., ◁◁ ▷▷, ◅◅ ▻▻.

◁ and ▷ look indeed nicer, their weight goes better with the weight of text in other buttons (but they may still have a bit too much height). But no real other alternative arrows available in Noto Sans.
Even ⬅ U+2b05 looks too black in Noto Sans.

Alright, I'll stick in the other arrows and merge this. Feel free to start proposing any changes immediately in PRs as I don't yet have anything concrete lined up.

it looked thiner and more elegant, I considered for years this to be a feature, as with 1000x1400, we can do thiner stuff that we couldn't on 600x800.

It is. One of the biggest crimes perpetrated in present-day GUIs from Apple to Microsoft to GNOME is this odd notion that all interface elements should scale equally. The only reason I had such ridiculously large fonts in the taskbar and menus and such was that otherwise it would be illegible. On the UHD monitor I've had since 2014 I've been using my GUI fonts as ever so slightly larger (let's say 1.4-1.5) and content fonts at 2x. That looks like this:

But there should be soon another one to fix the visual changes on high resolution devices introduced by this bug fix :)

I prefer to call it reasonable DPI. We've been suffering low DPI; now we have reasonable DPI. ;-)

https://techcrunch.com/2017/05/25/feast-your-eyes-on-this-600-dpi-e-paper-screen/