Enable toolbar scaling (DPI)

[ProgerXP]

trafficstars

Notepad 2e is now way better than Notepad2 in non-100% DPI (see #154). However, this does not apply to the toolbar: at any scaling, its size remains the same in absolute pixels despite the rest of the window (window frame, menu, text area, etc.) being properly scaled.

Can we scale the toolbar as well? I can provide larger toolbar bitmap(s) for various sizes if needed. Or the existing bitmap can be scaled to match the DPI.

[Anton-V-K]

PR #392 is ready for reviewing.

[ProgerXP]

@cshnik Check #392 please.

[cshnik]

@Anton-V-K Please consider the following:

1. Dynamic graphics scaling for UI controls (when using pixel images for buttons/toolbars) is not a good practice since it does not allow to provide clear and stable UI when working on different configurations. The better way would be to support max standard scaling 200% for toolbar and get rid of the implemented image scaling proc. This way we provide a less flexible but much more predictable representation for UI.
2. Newly committed toolbar images are not usable AS IS since scale factor is drawn over it. I'm sure it was made for testing purposes (during the work on the feature) but it was not expected to be the part for the PR.
3. Current value for NUMTOOLBITMAPS is incorrect and should be set to 33. The issue can be noticed when testing toolbar in high DPI mode (200% scaling).

[ProgerXP]

Dynamic graphics scaling for UI controls (when using pixel images for buttons/toolbars) is not a good practice since it does not allow to provide clear and stable UI when working on different configurations. The better way would be to support max standard scaling 200% for toolbar and get rid of the implemented image scaling proc. This way we provide a less flexible but much more predictable representation for UI.

1. What do you mean by "clear and stable UI when working on different configurations"?
2. Windows allows up to 400% scaling (on a 4K display). I don't have a 8K display but I guess it may allow up to 800%.
3. Do you mean we should include images for all standard scaling % (around 16 or 32) and not use dynamic scaling if user's DPI is non-standard? This will make the DPI issue present for cases we cannot predict (like 8K displays) meaning some people will still have small icons. Current implementation allows a fallback for such users.

That said, the if/else block testing user's DPI doesn't look ideal so if we add more bitmaps it should be converted for a loop testing if the resource bitmap exists (by name). But in principle I think the implementation is good.

Newly committed toolbar images are not usable AS IS since scale factor is drawn over it. I'm sure it was made for testing purposes (during the work on the feature) but it was not expected to be the part for the PR.

I will replace the images. It's okay for daily builds (this PR).

[ProgerXP]

@Anton-V-K We have discussed this and decided that scaling should only occur if the window's DPI is >=20% larger than the last (largest) resource in the EXE. In other cases the nearest resource is used without scaling. Also, to avoid looping or if/else blocks, it's better to move the bitmap generation code (either standard or scaled) from Notepad2.c (leaving a single-line [2e] comment) to DPIHelper.cpp and create a std::map with all supported DPIs.

Please update the implementation and correct NUMTOOLBITMAPS.

[Anton-V-K]

[...] scaling should only occur if the window's DPI is >=20% larger than the last (largest) resource in the EXE [...] [...] move the bitmap generation code (either standard or scaled) from Notepad2.c (leaving a single-line [2e] comment) to DPIHelper.cpp and create a std::map with all supported DPIs [...]

Well, the approach with pre-created bitmap looks a bit inefficient - the scale factor is hardly changeable on a daily basis, but the app will allocate bitmaps for all supported DPI => this will slightly increase memory footprint (not a big issue on modern hardware though). I doubt it will be possible to pre-create the scaled bitmap (for DPI 168+20% and up), because the scale factor isn't know at the moment of the map initialization, so probably this bitmap will be created on demand.

[ProgerXP]

Well, the approach with pre-created bitmap looks a bit inefficient - the scale factor is hardly changeable on a daily basis, but the app will allocate bitmaps for all supported DPI => this will slightly increase memory footprint (not a big issue on modern hardware though).

Are you talking about storing variants of Toolbar.bmp for all supported DPI (100, 150, 200, etc.) inside the EXE? If so, then there is no way around this because there is no way for automatic scaling to produce nice and crisp icons, so we should supply them.

It's true that Toolbar.bmp at 400% will have the size of about 500 KiB and that will be added to all processes' memory usage but again, this can't be avoided if we want good-looking icons and keep single-file distribution.

I doubt it will be possible to pre-create the scaled bitmap (for DPI 168+20% and up), because the scale factor isn't know at the moment of the map initialization, so probably this bitmap will be created on demand.

Yes, of course. The map is only for bitmaps stored in resources. If there is no match for the window's DPI in the map and that DPI is by 20% larger than the larger bitmap then create it on demand, like you have already done.

[Anton-V-K]

[...] to avoid looping or if/else blocks, [...] and create a std::map with all supported DPIs

I'm not sure I got the idea how to rewrite the code where no loops/ifs are used %) There will be a function like HBITMAP DPICreateMainToolbar(const HWND hwnd) (in DPIHelper.*) with a C-style array like this:

  static const struct
  {
    DWORD dpi;
    UINT  toolbar_id;
  } map[] =
  {
    // We provide toolbars for standard DPI only: 96 DPI (100%), 120 DPI (125%), 144 DPI (150%), 168 DPI (175%)
    { 168,  IDB_TOOLBAR_175 },
    { 144,  IDB_TOOLBAR_150 },
    { 120,  IDB_TOOLBAR_125 },
    { 96,   IDR_MAINWND }, // default
  };

The rest of code will scan through this array to select proper toolbar id, load the bitmap from resources and optionally stretch it if DPI is over limit.

[ProgerXP]

The rest of code will scan through this array to select proper toolbar id

Use std::map with range() keys.

[Anton-V-K]

Which range() do you mean? I see no such class in the current code base. Is it a kind of range class from C++20? If the range is supposed to be something like std::pair<int, int>, I can't imagine which benefits it will give. Anyway a map with ranges seems to be an overkill for this task - ordinary array seems to be enough. The ranges can also be used in it this way:

{ 151, 200,  IDB_TOOLBAR_175 },
{ 131, 150,  IDB_TOOLBAR_150 },
{ 111, 130,  IDB_TOOLBAR_125 },
{   0, 110,  IDR_MAINWND },    // default

This can further be simplifed by dropping first column as redundant.

[ProgerXP]

I mean something like this:

std::map<int, int> images = {
  {range(0, 100), IDB_...},
};

return images.at(dpi);

Anyway a map with ranges seems to be an overkill for this task - ordinary array seems to be enough. The ranges can also be used in it this way:

Well, it's just one at() call, what can be simpler?

[Anton-V-K]

Could you please elaborate what is range(0, 100)? I'm also very eager to find out how std::map can squeeze a range into int :)

[ProgerXP]

I'm also very eager to find out how std::map can squeeze a range into int :)

I'm sorry, I got a little bit carried away here.

You can go ahead with a function in DPIHelper.cpp with regular C array with two columns (min DPI and resource ID) and for. The array will be easier to maintain in case we decide to shift bitmap DPIs, and more compact than a dozen of if/else branches.

[Anton-V-K]

I've submitted my changes in PR #392

[cshnik]

Done.