MediaArea / MediaInfo

Convenient unified display of the most relevant technical and tag data for video and audio files.
https://MediaArea.net/MediaInfo
BSD 2-Clause "Simplified" License
1.26k stars 149 forks source link

Windows GUI: Enable DPI Awareness #835

Closed cjee21 closed 1 month ago

cjee21 commented 1 month ago

Add high DPI support to Windows GUI by setting dpiAwareness to PerMonitorV2 in manifest settings as well as adding high DPI scalable image assets to Main and About windows. The button icons now have transparent backgrounds as well.

Resolves #750

To take note:

Let me know if any changes are needed.

Screenshot 2024-05-20 222336

JeromeMartinez commented 1 month ago

Thank you a lot for the 2 PRs. They are not small and we need to check the modification for being sure that nothing is broken, we'll handle them as soon as possible compared to some other hot things we have to do, please be a bit patient (maybe a week or 2).

cjee21 commented 1 month ago

Would you mind to create another PR with only the icon changes?

For enabling high DPI support, other than the icon changes, the only other thing I changed was to toggle this: Screenshot 2024-05-20 155437 So a PR with only icon changes would probably only be a one line different compared to existing PR.

And replacing the current images rather than adding new ones (or at least removing all unused stuff if you can not replace directly, so removal of old images when you add new images) "_HiDPI" should not be needed, we keep only one instance of the images

I can remove those and the '_HiDPI'. I left it there because I wasn't sure if it is okay to remove them (in case someone still needs the old ones). Screenshot 2024-05-20 160229 This icon number 6 cannot be found in this repo and doesn't seem to be used anywhere so it will not be present after this.

.h should have spaces rather than tabs as in the other part of the files.

This file is auto edited by the IDE. For this PR I did not manually edit any source code. I can manually change the tab to spaces.

If not too difficult, try to reduce as much as possible the changes in the project files, I know that the software always try to update completely the project but we try to have a changelog with changes we understand. If not possible, maybe a first commit with only load and save of the project so it is the changes from the software, then another commit with your actual changes?

I'll see if I can split the commit.

cjee21 commented 1 month ago

Changes mentioned in above comment were made. The old icons were only removed from the Windows GUI project and those in Source\Resource\Image\Menu of this repo remain untouched.

JeromeMartinez commented 1 month ago

This icon number 6 cannot be found in this repo and doesn't seem to be used anywhere so it will not be present after this.

it is an former sponsor, no more used.

cjee21 commented 1 month ago

the binary ico is away, maybe the logo cion need to stay somewhere

It was removed by C++Builder when it made that big change to the project file by itself on first load. I have restored it.

maybe our C++ Builder version on the build farm is too old

If that is the case then it may explain why DEP and ASLR were not enabled on the official release. I found that it is enabled by default here and therefore is not in the project file. If I try to disable it then it gets written to project file. Looks like only non-default values are written to the file so I can't make a patch to enable DEP and ASLR.

A version that is later than mentioned by embarcadero blog post quoted below is needed for this PR.

Delphi and C++Builder added support for high DPI in the VCL (Windows UI framework) in version 10.3, released in November 2018.

JeromeMartinez commented 1 month ago

Delphi and C++Builder added support for high DPI in the VCL (Windows UI framework) in version 10.3, released in November 2018.

This is the version we currently use in our CI.

It was removed by C++Builder when it made that big change to the project file by itself on first load. I have restored it.

OK, I will do some tests with merge of the 2 commits of removal then revert, and try more minimal changes and see if it is OK. We'll also check if it is easy to update C++ Builder to a new version (11 something currently for the community version) cc @g-maxime.

cjee21 commented 1 month ago

I tried the changes with our build farm

and also the binary does not launch, so weird

I tried this build and found that it crashed on start before anything appear:

Problem Event Name: APPCRASH
Application Name:   MediaInfo.exe
Application Version:    24.4.0.0
Application Timestamp:  00000000
Fault Module Name:  KERNELBASE.dll
Fault Module Version:   10.0.22621.3527

No idea why.

cjee21 commented 1 month ago

I minimized the changes by manually editing the project file after getting a better diff using Visual Studio Code. Reduced to two commits. Tested that opening then immediately building the project using C++Builder IDE works. Don't know if it will work on your build farm.

JeromeMartinez commented 1 month ago

I minimized the changes by manually editing the project file after getting a better diff using Visual Studio Code.

Greatly appreciated, it helps to understand the actual changes.

Don't know if it will work on your build farm.

Unfortunately no change, this build has your change whereas this build was done from main branch this morning with the same setup and works. I'll try to push changes one per one e.g. changing the icons only for seeing which one is faulty, and in parallel we'll test an upgrade to the latest C++ builder version (11.3, few years younger than what we currently have).

cjee21 commented 1 month ago

we'll test an upgrade to the latest C++ builder version (11.3, few years younger than what we currently have).

This should enable DEP and ASLR by default. (source)

JeromeMartinez commented 1 month ago

Don't know if it will work on your build farm.

We updated it to C++ Builder 11.3 and it works better :-p. An incompatibility with one of the used features, but it is fine for us to use this C++ Builder. @cjee21 please test this build.

This should enable DEP and ASLR by default

I guess that the default is for new projects but not old ones, this would be for another PR.

cjee21 commented 1 month ago

This force push is just to fix tab/space indent issue in the project file and change the commit names to better match existing commits of this repo.

We updated it to C++ Builder 11.3 and it works better :-p. An incompatibility with one of the used features, but it is fine for us to use this C++ Builder. @cjee21 please test this build.

Looks good. 😃

This should enable DEP and ASLR by default

I guess that the default is for new projects but not old ones, this would be for another PR.

No need for another PR. They are enabled now as expected. Screenshot 2024-05-21 202051

JeromeMartinez commented 1 month ago

@cjee21 please rebase (CI fail fixed by https://github.com/MediaArea/MediaInfo/pull/837).

cjee21 commented 1 month ago

@cjee21 please rebase (CI fail fixed by #837).

Ok Done.