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.4k stars 160 forks source link

Windows GUI: Windows 11 File Explorer Context Menu #960

Open cjee21 opened 1 week ago

cjee21 commented 1 week ago

Integrate with Windows 11 File Explorer modern context menu by implementing required items:

New shell extension also has the benefit of opening multiple selected files in the same MediaInfo instance instead of in multiple windows.

Resolves #671


Explanation of approach chosen for this new context menu implementation

Enabled security mitigations for new context menu

The following are enabled in the project file for the Explorer context menu shell extension DLL.

GS (Buffer Security Check), sdl (Additional Security Checks), NXCOMPAT (Data Execution Prevention), DYNAMICBASE (Address space layout randomization), HIGHENTROPYVA (64-Bit ASLR), guard:cf (Control Flow Guard), guard:ehcont (EH Continuation Metadata), Qspectre (Spectre variant 1 mitigation), CETCOMPAT (CET Shadow Stack)

References


Todo for MediaInfo team:

cjee21 commented 1 week ago

Note: Open source code licensed under GPL and MIT from Microsoft, NanaZip and Npp were referenced in the making of this PR. Although code in this PR is not identical to any of the referenced code, it should be ensured that there are no licensing issues. I am not a legal expert.

cjee21 commented 1 week ago

MSVC2019 to MSVC2022 stuff

A question, the project files for the new DLLs are only in MSVC2022. In that case how to go about in the installer script?

Then for this PR: PNG should go in \Source\Resource\Image\Assets.

I put it there so that is is easier to re-generate the .pri file because everything has to be in the correct folder structure as what is in a MSIX. Do you still want to move it?

cjee21 commented 1 week ago

Manifest.rc/xml deletion (not used?)

It is not currently used by C++Builder, a default manifest from C++Builder's directory is used. I deleted it in the same commit as adding Manifest.manifest so that there remains only the in-use manifest to prevent confusion.

$(BDS) to ..\..

This is replacing the default manifest from C++Builder's directory with the new Manifest.manifest for package identity.

cjee21 commented 1 week ago

Done most of the changes discussed. I've totally removed changes with the GUI. I still have no clear idea how to go about it so you can give it a look/try.

JeromeMartinez commented 1 week ago

I deleted it in the same commit as adding Manifest.manifest so that there remains only the in-use manifest to prevent confusion.

If I understand correctly this is independent from the Win11 context menu and useful as standalone. If so please a separate PR.

A question, the project files for the new DLLs are only in MSVC2022. In that case how to go about in the installer script?

It seems that we have a coherency problem, we should go with MSVC2022 everywhere, we check. cc @g-maxime.

I put it there so that is is easier to re-generate the .pri file because everything has to be in the correct folder structure as what is in a MSIX. Do you still want to move it?

I prefer to have all MediaInfo logs at the same place when not to complicated, and also I don't like to have the .pri (a binary file?) if not source in the source code. How is built the .pri? Would it make sense to have the logo files in the Source\Resource\Image\Assets and have a cmd (referenced in the project) which copy the file to the expected place and build the .pri? Not a big deal if you prefer to keep it as is, we will check later if we can improve the build if you prefer to keep as is.

cjee21 commented 1 week ago

I deleted it in the same commit as adding Manifest.manifest so that there remains only the in-use manifest to prevent confusion.

If I understand correctly this is independent from the Win11 context menu and useful as standalone. If so please a separate PR.

The manifest is for granting package identity to MediaInfo GUI. It goes along with the sparse package.

Would it make sense to have the logo files in the Source\Resource\Image\Assets and have a cmd (referenced in the project) which copy the file to the expected place and build the .pri? Not a big deal if you prefer to keep it as is, we will check later if we can improve the build if you prefer to keep as is.

The .pri only needs to be rebuilt when there is a change to the resources. If you prefer to rebuild it everytime, I will let you make the change. Guide: https://learn.microsoft.com/en-us/windows/msix/desktop/desktop-to-uwp-manual-conversion#generate-a-package-resource-index-pri-file-using-makepri

JeromeMartinez commented 1 week ago

The manifest is for granting package identity to MediaInfo GUI. It goes along with the sparse package.

OK.

The .pri only needs to be rebuilt when there is a change to the resources. If you prefer to rebuild it everytime, I will let you make the change.

OK, let's keep as is for the moment, You already do a lot and we'll manage the rebuild on our side (no need to be automatic, usually it is done by a check if the source files are younger than the binary, anyway we'll manage that).


I remark only now that we still build with MSVC2019 on our build farm, we'll migrate in order to be able to test this PR.

JeromeMartinez commented 1 week ago

The manifest is for granting package identity to MediaInfo GUI. It goes along with the sparse package.

I change my mind: my understanding now is that it does not hurt to switch to this manifest even without building the sparse package. If so, please a separate PR with the BCB project modification + manifest, and I merge and build with this change, so this part is ready without waiting for the merge of this PR.

cjee21 commented 1 week ago

Okay, tested the current state of this branch:

* = effect of updated manifest ^ = effect of removing the GUI changes

JeromeMartinez commented 1 week ago

The old context menu is still shown so duplicate.^ The GUI Preferences only controls the old context menu.^

Theses ones were not expected. I see code in WindowsShellExtension handling the registry key. Isn't it possible to just launch WindowsShellExtension when the preference window is closed? My concern is to avoid creating more registry keys and touch as few things as possible, not to remove everything.

cjee21 commented 1 week ago

Theses ones were not expected. I see code in WindowsShellExtension handling the registry key. Isn't it possible to just launch WindowsShellExtension when the preference window is closed? My concern is to avoid creating more registry keys and touch as few things as possible, not to remove everything.

Let me try to explain in simple way how it works.

  1. The new DLL for shell extension needs to be registered as a context menu handler.

    • This can be done either by manually adding registry entries or by declaring it in the manifest of a MSIX.
    • MSIX method requires Windows build 17763 and above but since this is an unpackaged app, it requires sparse package which needs Windows build 19000 and above.
    • Registry manual method should work on Windows 7 or maybe even Vista onwards.
    • MSIX method is a must for the context menu entry to appear in the Windows 11 modern style context menu.
    • For now in this PR, only implement the MSIX method and only for Windows 11.
  2. Once registered, when files are right-clicked in Explorer, Explorer will check list of registered handlers for that file extension. It will load the DLL in COM Surrogate / DLLHost (because it does not trust third party DLLs that may crash). This can be observed using Process Explorer from Sysinternals.

  3. Then it calls the functions in our DLLMain.cpp after calling the entrypoint to get the COM interface.

    • GetState is called to determine if the menu entry should appear
      • In here, we check the registry that was set by GUI. If registry key indicates that it is disabled, we set cmdState = ECS_HIDDEN and Explorer will hide our entry. Else setting to ECS_ENABLED will enable the entry. Take note we only read registry here, no writing. Also code in this function should be fast so that Explorer menu does not hang or timeout.
    • GetIcon and GetTitle for providing the icon and title shown in the menu
    • For others like GetTooltip, we return E_NOTIMPL as we didn't implement
  4. When we click on the menu entry, Invoke function is called by Explorer.

    • In here, we obtain from Explorer and parse the list of files selected. It is converted to command line and then we use that command line to start-up MediaInfo.exe and then exit this function.
  5. About 10 seconds after the context menu is dismissed, our DLL will be unloaded by File Explorer from DLLHost. This can be observed using Process Explorer from Sysinternals.

So what else needs to be done in this PR? For the Qt version, there is no other shell extensions to deal with and the Qt GUI already writes all its Preferences settings to the registry. So all is well and working properly. We can ignore Qt one now. For the VCL one, this is where things have to be done. What needs to be done that I not yet do is...

cjee21 commented 1 week ago

Took a look at the codes for handling old context menu again and came up with a better solution. Appears to work properly on Windows 11 now. No duplicate entries and can control appearance of the new shell extension. Just need to test on Windows 10 and portable to ensure there is no change to existing behaviour there.

cjee21 commented 1 week ago

Note: we should consider adding notifying Windows Shell to refresh list of context menu upon installation/uninstallation. Else both the new and old menu entries will not appear after installation until Windows refreshes itself or something triggers a refresh althoough they are enabled by default. It will prevent issues like: https://sourceforge.net/p/mediainfo/support-requests/51/

Also, this bug: https://sourceforge.net/p/mediainfo/bugs/1161/ is reproducible and is now causing duplicate entries in 'classic' context menu for mp3 files etc. UPDATE: Seems this is caused by MediaInfo still associated with audio and video in registry. Something in the old codes is not deleting properly. So basically, all these years, the existing context menu actually cannot be disabled completely. It is weird that it affects only some files like mp3 though because others like flac is also classified as audio in Windows.


Seems there is some registry left behind for some file extensions even after uninstall Screenshot 2024-11-18 142236


There are issues with existing (old) context menu implementation. In Preferences.cpp ShellExtension there are 144 extensions listed. In Preferences.cpp InfoTip there are 114 extensions listed. In MediaInfo_Extensions_Install there are 145 extensions listed. In MediaInfo_Extensions_Uninstall there are 141 extensions listed.

cjee21 commented 1 week ago

Force push adds avif, avs, heic, heif, iamf, ico, jxl and webp to extensions list in manifest and changes webm to lower case.

JeromeMartinez commented 1 week ago

Took a look at the codes for handling old context menu again and came up with a better solution.

Thank you, it seems less complicated than the previous proposal.

Also, this bug: https://sourceforge.net/p/mediainfo/bugs/1161/ is reproducible and is now causing duplicate entries in 'classic' context menu for mp3 files etc. UPDATE: Seems this is caused by MediaInfo still associated with audio and video in registry. Something in the old codes is not deleting properly. So basically, all these years, the existing context menu actually cannot be disabled completely. It is weird that it affects only some files like mp3 though because others like flac is also classified as audio in Windows.

Ha, I see! But so weird, indeed. I don't see an easy solution... Maybe just removing video & audio if it does not work well enough, and we manage our own list completely.

cjee21 commented 1 week ago

I don't see an easy solution... Maybe just removing video & audio if it does not work well enough, and we manage our own list completely.

I agree with this. But issue now is making sure all unwanted registry entries are correctly removed on uninstall or upgrade of MediaInfo.

JeromeMartinez commented 1 week ago

But issue now is making sure all unwanted registry entries are correctly removed on uninstall or upgrade of MediaInfo.

Would you mind to add a PR about it? It seems that you see better than me the issue there.

cjee21 commented 1 week ago

But issue now is making sure all unwanted registry entries are correctly removed on uninstall or upgrade of MediaInfo.

Would you mind to add a PR about it? It seems that you see better than me the issue there.

Screenshot 2024-11-18 203519

Looks like the GUI is trying the wrong key that's why fail to disable for audio and video. There is no such thing as HKCU\SystemFileAssociations

cjee21 commented 1 week ago

Also I think the old shell will be re-enabled everytime MediaInfo is updated for those who disabled it until they open MediaInfo.

cjee21 commented 1 week ago

@JeromeMartinez I merged all my currently open PRs to a branch and made a test build. Everything seems to be in order now. Both context menu entry appears automatically after install, old one gets disabled on first launch of MediaInfo, no duplicate entries after that, can enable/disable shell extension and after uninstall, no registry entries that are written by MediaInfo are left behind.

cjee21 commented 1 week ago

Just a rebase, so that can test properly without the other bugs.

cjee21 commented 1 week ago

If we were to use the same DLL shell extension for older Windows versions, this is probably the way to do it: https://learn.microsoft.com/en-us/windows/win32/shell/reg-shell-exts#example-of-an-extension-handler-registration

JeromeMartinez commented 1 week ago

If we were to use the same DLL shell extension for older Windows versions, this is probably the way to do it:

Thank you. I am not sure I want to spend the time on that but I would be happy to get an update if you wish to do so. For the moment, the important part is Win11 integration (including the tooltip), it is less important for older versions.

cjee21 commented 1 week ago

Thank you. I am not sure I want to spend the time on that but I would be happy to get an update if you wish to do so.

I do not want to spend time on Windows versions that will soon be end-of-support too and risk breaking things. The only benefit for older Windows versions is opening multiple files in the same instance and maybe some simplification of old codes.

For the moment, the important part is Win11 integration (including the tooltip), it is less important for older versions.

Context menu is working fine so far and can now be independently controlled for folders as well after the last commit. ToolTip is already working when I tested after seeing issue 809 that day.

PS No idea how Windows decides the order to show menu items in the modern context menu but MediaInfo seems to always be the first entry on my PC.

cjee21 commented 1 week ago

btw it is possible to sign the NSIS uninstaller too to prevent yellow Windows warning when executing uninstall https://nsis.sourceforge.io/Docs/Chapter5.html#uninstfinalize

There's also a note at https://nsis.sourceforge.io/Signing_an_Uninstaller that says:

Note 2: Running a dummy command with !uninstfinalize may also be useful if only the installer will be signed. This could prevent bogus header fields in the uninstaller, see feature request ticket #577

JeromeMartinez commented 1 week ago

btw it is possible to sign the NSIS uninstaller too to prevent yellow Windows warning when executing uninstall

It was the case several years ago and at some point it disappeared, I was too lazy for finding the reason :(.

cjee21 commented 1 week ago

btw it is possible to sign the NSIS uninstaller too to prevent yellow Windows warning when executing uninstall

It was the case several years ago and at some point it disappeared, I was too lazy for finding the reason :(.

I mean currently, the uninstaller is not signed so when user execute uninstall, this appears instead of what we see during install: Untitled

If we sign it and add MediaInfo icon then the same UAC window should appear like the one during install with publisher name and icon.

JeromeMartinez commented 1 week ago

I mean currently, the uninstaller is not signed so when user execute uninstall

I was speaking about that too :).

cjee21 commented 1 week ago

I mean currently, the uninstaller is not signed so when user execute uninstall

I was speaking about that too :).

So from what I read, to sign it we need use macro (https://nsis.sourceforge.io/Docs/Chapter5.html#uninstfinalize) to execute the signing script. This will make NSIS call the signing script after it generate uninstaller then script signs the uninstaller before NSIS package it into the installer. Can also use MUI_UNICON to change the uninstaller exe icon.

cjee21 commented 1 week ago

@JeromeMartinez managed to get a signed uninstaller.

Just need to add this to the .nsi files:

!define MUI_UNICON "..\..\Source\Resource\Image\MediaInfo.ico"

; Sign uninstaller
!uninstfinalize 'sign.cmd "%1" "MediaInfo Uninstaller"'

Then create a bat/cmd file named sign with the following:

set /P CodeSigningCertificatePass= < %USERPROFILE%\CodeSigningCertificate.pass
if "%NOSIGN%"=="" (
    signtool sign /f %USERPROFILE%\CodeSigningCertificate.p12 /p %CodeSigningCertificatePass% /fd sha256 /v /tr http://ts.ssl.com /td sha256 /d %2 /du http://mediaarea.net %1
)
set CodeSigningCertificatePass=

After that just build as usual and the uninstaller should be signed. When user uninstalls, UAC will show MediaInfo Uninstaller with MediaInfo logo and verified publisher instead of uninst.exe with unknown publisher.

If want to sign the installer here instead of from build script, can also do:

; Sign installer
!finalize 'sign.cmd "%1" "MediaInfo Installer"'
cjee21 commented 20 hours ago

Rebased to master.

NSIS needs to be updated to 3.10 (at least 3.09) to support the ${AtLeastWin11} check used in this PR. Should consider updating the 8-year-old 7-Zip while at it. Maybe update libcurl too if there are security fixes. https://github.com/MediaArea/MediaArea-Utils-Binaries/tree/master/Windows