Skip to content
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

Qt GUI Improvements #876

Merged
merged 11 commits into from
Jun 19, 2024
Merged

Qt GUI Improvements #876

merged 11 commits into from
Jun 19, 2024

Conversation

cjee21
Copy link
Collaborator

@cjee21 cjee21 commented Jun 19, 2024

A collection of improvements made while I was exploring the Qt version of MediaInfo.

  • Use MSVC2022 for building the x86_64 Windows build
    • Changes to project file to successfully build using MSVC2022 including fixes for linker errors due to path issues
  • Enable Control Flow Guard (CFG) for x86_64 Windows build
    • Enables CFG as strongly encouraged by Microsoft.
    • For full protection, MediaInfoLib, ZenLib and zlib should be built with /guard:cf enabled in Project | Properties | Configuration Properties | C/C++ | Code Generation of their respective Visual Studio Projects.
  • Reverts commit 18b6e3c
    • This commit it is not cross-platform and causes missing icons in both Windows and Ubuntu.
  • Use newer 'Close' icon when available
    • Uses newer QIcon::fromTheme(QIcon::ThemeIcon::WindowClose) when built with Qt > 6.7
    • This newer icon is visible in dark mode on Windows unlike previous one.
  • Prefer Cascadia Mono font when available
    • For systems with Cascadia Mono font (the default font for Windows Terminal) installed, it will be used else old font selection behaviour is maintained.
    • Cascadia Mono is more readable than the previously selected font on Windows and is also consistent with current VCL version of MediaInfo.
  • Prevent toolbar from being moved
    • Previously, it was possible to detach the toolbar.
  • Prevent clipped text in easy view
    • Make it resize properly to prevent clipped text in easy view when viewing certain files with multi-line metadata due to text wrap.
  • Other improvements
    • Remove unused includes in mainwindow.cpp.
    • Fix inconsistent indents in mainwindow.cpp.

Tested with Qt 6.7.2 and MSVC2022.

Windows 11 screenshots:
Screenshot 2024-06-19 192245
Process Explorer showing 64-bit Qt version with DEP, ASLR, CFG and Per-monitor DPI Awareness enabled
Screenshot 2024-06-19 190541
Screenshot 2024-06-19 190557
Screenshot 2024-06-19 190618

Ubuntu screenshots:
Screenshot from 2024-06-19 19-10-41
Screenshot from 2024-06-19 19-10-58
Screenshot from 2024-06-19 19-11-10

Previous screenshots for comparison:
#857 (comment)
#857 (comment)

cjee21 added 9 commits June 19, 2024 01:02
MediaInfoLib, ZenLib and zlib should be built with Control Flow Guard (CFG) enabled as well.
This reverts commit 18b6e3c because it is not cross-platform.
Use newer theme icon for 'Close' buttons when on Qt > 6.7 so that it is visible in Windows dark mode.
Prefer Cascadia Mono font as monospaced font for text view and sheet view.
Copy link
Member

@JeromeMartinez JeromeMartinez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A collection of improvements made while I was exploring the Qt version of MediaInfo.

Thank you so much, we didn't yet have the time to focus on such improvement and it is great that a third party uses the possibilities of open source for improving MediaInfo, and that you take the time to deeply test the results, we know that it is a lot of time.

Use MSVC2022 for building the x86_64 Windows build

I prefer to keep a coherence so to change that everywhere.
About $$PWD, what is the issue without it? It seems to work well without it.

Enable Control Flow Guard (CFG) for x86_64 Windows build

Beside Qt project, if you don't mind, please check, enable if needed, test and send another PR for VS2022 as well as VS2019 (we didn't yet upgraded VS on our build farm), for all repositories. Nothing mandatory if you don't want to do that, just a wish.

As we are there, you may be curious about CET, and try CET option. As we don't have a CPU supporting CET, we didn't try but it would be great if it is doable.

@cjee21
Copy link
Collaborator Author

cjee21 commented Jun 19, 2024

I prefer to keep a coherence so to change that everywhere.

I can change for others as well but it will be untested.

About $$PWD, what is the issue without it? It seems to work well without it.

Without this I get many linker errors of things not found during the linking stage.

Beside Qt project, if you don't mind, please check, enable if needed, test and send another PR for VS2022 as well as VS2019 (we didn't yet upgraded VS on our build farm), for all repositories. Nothing mandatory if you don't want to do that, just a wish.

I'll see what I can do when I'm free.

As we are there, you may be curious about CET, and try CET option. As we don't have a CPU supporting CET, we didn't try but it would be great if it is doable.

My CPU seems supported. I'll check it out.

There is also Spectre mitigations but I'm not sure if relevant for MediaInfo. This one is known to affect performance. This one is enabled in my Windows 11 context menu PoC.

@JeromeMartinez
Copy link
Member

I can change for others as well but it will be untested.

Similar update so I take the risk :).

Without this I get many linker errors of things not found during the linking stage.

Weird, Fedora uses it and seems fine, @g-maxime can you check that mediainfo-qt package still compiles on Fedora?

There is also Spectre mitigations but I'm not sure if relevant for MediaInfo. This one is known to affect performance.

IMO the "cost" of this option is higher than the benefit we may have with it, so I wouldn't add it until they find a less impacting mitigation.

@cjee21
Copy link
Collaborator Author

cjee21 commented Jun 19, 2024

Weird, Fedora uses it and seems fine, @g-maxime can you check that mediainfo-qt package still compiles on Fedora?

Only affects Windows MSVC as far as I know. It built fine on Ubuntu without any changes.

@JeromeMartinez
Copy link
Member

Only affects Windows MSVC as far as I know. It built fine on Ubuntu without any changes.

Oops, true, so @g-maxime no need to test.

Enable Control-flow Enforcement Technology (CET) / Hardware-enforced Stack Protection on Windows.
@cjee21
Copy link
Collaborator Author

cjee21 commented Jun 19, 2024

It works! :D
Screenshot 2024-06-19 212106
Tested on AMD CPU.

@cjee21
Copy link
Collaborator Author

cjee21 commented Jun 19, 2024

Beside Qt project, if you don't mind, please check, enable if needed, test and send another PR for VS2022 as well as VS2019 (we didn't yet upgraded VS on our build farm), for all repositories. Nothing mandatory if you don't want to do that, just a wish.

PRs created for MediaInfoLib, ZenLib and zlib.

Also for MediaInfo CLI. There are probably other MSVC Projects which I did not touch because I do not know their purpose.

@JeromeMartinez JeromeMartinez merged commit 7dd7434 into MediaArea:master Jun 19, 2024
3 checks passed
@cjee21 cjee21 deleted the Qt branch June 20, 2024 04:52
@cjee21
Copy link
Collaborator Author

cjee21 commented Jun 20, 2024

FYI The HTML view in Qt version already has proper dark mode support on Windows.

Screenshot 2024-06-20 124639

EDIT: This is before the HTML patch for MediaInfoLib. It has extra cells on the first row. Can't find why it displays so many borders.

@cjee21
Copy link
Collaborator Author

cjee21 commented Jun 20, 2024

PRs created for MediaInfoLib, ZenLib and zlib.

Also for MediaInfo CLI. There are probably other MSVC Projects which I did not touch because I do not know their purpose.

@JeromeMartinez
Confirmed that most recent dev build has CFG and CET enabled for MediaInfo DLLs and CLI
Screenshot 2024-06-20 230358
Screenshot 2024-06-20 230420
Screenshot 2024-06-20 230500
Screenshot 2024-06-20 230620

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants