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

Windows GUI: Windows 11 File Explorer Context Menu #960

Merged
merged 9 commits into from
Nov 29, 2024

Conversation

cjee21
Copy link
Collaborator

@cjee21 cjee21 commented Nov 16, 2024

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

  • Shell extension DLL which implements IExplorerCommand interface
  • Sparse package which grants package identity
  • Package helper DLL used by installer to add and remove the sparse package
  • Updated installer script to handle everything

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

  • The sparse package is provisioned for all users by the installer during install and removed for all users during uninstall using a helper dll. This should ensure all users will have the context menu on login after install and a clean removal after uninstall.

  • The 'Assets' folder and resources.pri file are needed because when the app runs with app identity, the assets referenced by the sparse package manifest is used for the app icon. Since it is a sparse package, it itself does not contain assets and instead references the assets that are in the declared external location. The resources.pri is needed for the 'unplated' icons which are needed to prevent the icon from having accent-coloured plating as the background.

  • In order to minimize the size of the DLL while ensuring it runs on clean Windows installations without Microsoft Visual C++ Redistributable installed, vcruntime is statically linked while ucrt is dynamically linked. This results in something between using /MT and /MD. It is known as Hybrid CRT and is supported according to the CRT maintainer.

  • /PDBALTPATH:%_PDB% is added to linker in release mode so that the PDB file path is not contained in the DLL. This ensures no path information leakage and reduces the size of DLL slightly while still enabling analysis that requires PDB files to be done, for example SizeBench.

  • The shell extension is re-written from WRL to C++/WinRT. See 'Note' at https://learn.microsoft.com/en-us/cpp/cppcx/wrl/windows-runtime-cpp-template-library-wrl?view=msvc-170 for details on why.

References


Todo for MediaInfo team:

  • Update version update script for:

    • Source\GUI\VCL\Manifest.manifest
    • Source\WindowsSparsePackage\MSIX\AppxManifest.xml
    • Source\WindowsShellExtension\Resource.rc
    • Source\WindowsPackageHelper\Resource.rc
  • Update build script

    • make sparse package
      makeappx pack /d "Source\WindowsSparsePackage\MSIX" /p "Project\MSVC2022\x64\Release\MediaInfo_SparsePackage.msix" /nv

    • build DLLs

      MSBuild /t:MediaInfo_WindowsShellExtension /restore /p:RestorePackagesConfig=true;Configuration=Release;Platform=x64 Project\MSVC2022\MediaInfo.sln
      MSBuild /t:MediaInfo_PackageHelper /restore /p:RestorePackagesConfig=true;Configuration=Release;Platform=Win32 Project\MSVC2022\MediaInfo.sln
      
    • sign sparse package MSIX and DLLs generated by above

  • Test on other system configurations besides Windows 11 Home single account.

@cjee21
Copy link
Collaborator Author

cjee21 commented Nov 16, 2024

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.

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.

Thank you for you hard work on that.

In general this is OK but I would like some changes:

First I would like another PR with .gitignore, $(BDS) to ..\.. stuff, MSVC2019 to MSVC2022 stuff, BCB22 to BCB23 stuff (if relevant? we don't use version 12), Manifest.rc/xml deletion (not used?), Exec regsvr32 to ExecWait regsvr32.exe, Delete "$INSTDIR\MediaInfo_InfoTip.dll" to UnInstallLib, Refresh File Explorer.
It seems not related to the Win11 integration and I could independently validate them.

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

Please add the following header to all (non automatically generated and not tiny helper) .cpp & .h files:

/*  Copyright (c) MediaArea.net SARL. All Rights Reserved.
 *
 *  Use of this source code is governed by a BSD-style license that can
 *  be found in the License.html file in the root of the source tree.
 */

Source/Common/Preferences.cpp Outdated Show resolved Hide resolved
Source/GUI/VCL/GUI_Preferences.cpp Outdated Show resolved Hide resolved
Source/GUI/VCL/GUI_Preferences.cpp Outdated Show resolved Hide resolved
Source/GUI/VCL/GUI_Preferences.cpp Outdated Show resolved Hide resolved
Source/GUI/VCL/GUI_Preferences.cpp Outdated Show resolved Hide resolved
Source/WindowsShellExtension/dllmain.cpp Show resolved Hide resolved
Source/WindowsShellExtension/dllmain.cpp Show resolved Hide resolved
Source/WindowsShellExtension/dllmain.cpp Show resolved Hide resolved
@cjee21
Copy link
Collaborator Author

cjee21 commented Nov 16, 2024

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
Copy link
Collaborator Author

cjee21 commented Nov 16, 2024

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 cjee21 force-pushed the Windows11-contextmenu branch from d298b47 to ad12639 Compare November 16, 2024 15:08
@cjee21
Copy link
Collaborator Author

cjee21 commented Nov 16, 2024

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
Copy link
Member

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
Copy link
Collaborator Author

cjee21 commented Nov 16, 2024

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
Copy link
Member

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

This comment was marked as duplicate.

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.

Fine with that for the moment, but blocked because we don't have MSVC2022 on our build farm.

@JeromeMartinez
Copy link
Member

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
Copy link
Collaborator Author

cjee21 commented Nov 16, 2024

Okay, tested the current state of this branch:

  • The menu now only appears on the specified extensions + folders.*
  • If multiple combinations are selected such as media + non-media + folder etc, it will appear if the file you right click on is a specified extension even if non-media are among the selected.*
  • The old context menu is still shown so duplicate.^
  • The GUI Preferences only controls the old context menu.^

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

@JeromeMartinez
Copy link
Member

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
Copy link
Collaborator Author

cjee21 commented Nov 17, 2024

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...

  • to write Software\\MediaArea\\MediaInfo - ShellExtension DWORD = 0 when Preferences for shell is disabled by user and delete ShellExtension DWORD if enabled. Delete instead of setting to 1 since it is enabled by default so most people likely have it enabled so we avoid leaving registry keys behind since this is per user and can't be removed by uninstaller.
  • to disable/remove the old context menu when new one is installed so that there are no duplicate entries in the 'classic' Explorer context menu,
  • do something about the difference in behaviour of the two shell extensions
    • old one can toggle directories separately while new one can't
    • new one can open multiple files in same MediaInfo instance while old one launches one instance per file selected.

@cjee21
Copy link
Collaborator Author

cjee21 commented Nov 18, 2024

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 cjee21 force-pushed the Windows11-contextmenu branch from 3e75f45 to b7ce8ff Compare November 18, 2024 04:46
@cjee21
Copy link
Collaborator Author

cjee21 commented Nov 18, 2024

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 cjee21 force-pushed the Windows11-contextmenu branch from b7ce8ff to 29f90e2 Compare November 18, 2024 07:08
@cjee21
Copy link
Collaborator Author

cjee21 commented Nov 18, 2024

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

@JeromeMartinez
Copy link
Member

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
Copy link
Collaborator Author

cjee21 commented Nov 18, 2024

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
Copy link
Member

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 cjee21 force-pushed the Windows11-contextmenu branch from 29f90e2 to 13642e6 Compare November 18, 2024 12:15
@cjee21
Copy link
Collaborator Author

cjee21 commented Nov 18, 2024

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
Copy link
Collaborator Author

cjee21 commented Nov 28, 2024

@g-maxime
I tested https://mediaarea.net/download/snapshots/binary/mediainfo-gui/20241128-2/MediaInfo_GUI_24.11.20241128_Windows_x64.exe

MediaInfo fails to start because Source/GUI/VCL/Manifest.manifest does not support capital V for Version. Other than that the new context menu installs and uninstalls fine. Also, for some reason the version numbers for the two new DLLs does not seem to be updated successfully.

@cjee21
Copy link
Collaborator Author

cjee21 commented Nov 28, 2024

Also, for some reason the version numbers for the two new DLLs does not seem to be updated successfully.

Likely because the resource files were generated by Visual Studio and uses UTF-16 LE encoding.

@JeromeMartinez
Copy link
Member

@cjee21 thank you for the debugging.

@JeromeMartinez
Copy link
Member

Likely because the resource files were generated by Visual Studio and uses UTF-16 LE encoding.

Does it hurt if we switch to UTF-8?

@cjee21
Copy link
Collaborator Author

cjee21 commented Nov 28, 2024

Likely because the resource files were generated by Visual Studio and uses UTF-16 LE encoding.

Does it hurt if we switch to UTF-8?

Thinking if I should just manually edit it to be like the one used for MediaInfoLib DLL.

@cjee21
Copy link
Collaborator Author

cjee21 commented Nov 28, 2024

Okay let's try this. Converted to UTF-8 and simplified.

@JeromeMartinez
Copy link
Member

Okay let's try this. Converted to UTF-8 and simplified.

Thank you. Small thing but could you merge the commits adding the new files in UTF-16 and the UTF-8 switch, so we have only one commit for the addition of the new file and no UTF-16 intermediate.

Also, please remove the <SpectreMitigation>Spectre</SpectreMitigation> lines, for the moment we don't have it in other place, I think we would add it everywhere in one shot when we decide to switch to it.

@g-maxime
Copy link
Contributor

I've already relaunched a build, so the DLL version will not be updated in the next snapshot either.

@cjee21
Copy link
Collaborator Author

cjee21 commented Nov 28, 2024

Thank you. Small thing but could you merge the commits adding the new files in UTF-16 and the UTF-8 switch, so we have only one commit for the addition of the new file and no UTF-16 intermediate.

Also, please remove the <SpectreMitigation>Spectre</SpectreMitigation> lines, for the moment we don't have it in other place, I think we would add it everywhere in one shot when we decide to switch to it.

Time for an interactive rebase......

@JeromeMartinez
Copy link
Member

Time for an interactive rebase......

Thank you.
We are not far from the end for this PR, no other change request expected.

@cjee21 cjee21 force-pushed the Windows11-contextmenu branch from 6989cb8 to cbc3df3 Compare November 28, 2024 14:26
@cjee21
Copy link
Collaborator Author

cjee21 commented Nov 28, 2024

Also, please remove the <SpectreMitigation>Spectre</SpectreMitigation> lines, for the moment we don't have it in other place, I think we would add it everywhere in one shot when we decide to switch to it.

There is also EH Continuation Metadata (EHCONT) that is not yet enabled elsewhere in MediaInfo.

Instruction Pointer Validation section of https://techcommunity.microsoft.com/blog/windowsosplatform/developer-guidance-for-hardware-enforced-stack-protection/2163340

@JeromeMartinez
Copy link
Member

There is also EH Continuation Metadata (EHCONT) that is not yet enabled elsewhere in MediaInfo.

I learn new option every day.
But I prefer to have a common set of options in all project, and debate about options in separate PRs.

@cjee21
Copy link
Collaborator Author

cjee21 commented Nov 28, 2024

@JeromeMartinez
Copy link
Member

@cjee21 now that we can build it, I am testing the latest snapshot and I remark that after the first install without launching the app the right click does not show the new MediaInfo menu, only the old one with "show more options", and after I launch the app I have both MediaInfo with the right click (great!) and with "show more options".
Wasn't it expected that the new MediaInfo menu shows up immediately after install, without app launch?

I am fine with that if it is fine for you, just let me know that this was your target and I merge this PR.

@JeromeMartinez
Copy link
Member

@cjee21 forget about my previous comment, it is working as expected, there is just a delay between the install and the menu displayed.

@JeromeMartinez JeromeMartinez merged commit a1eea67 into MediaArea:master Nov 29, 2024
3 checks passed
@cjee21 cjee21 deleted the Windows11-contextmenu branch November 29, 2024 18:11
@cjee21
Copy link
Collaborator Author

cjee21 commented Nov 29, 2024

there is just a delay between the install and the menu displayed.

There is a shell refresh triggered after provisioning of package by installer but at that point the package is not installed by Windows yet. Since it is uncertain when Windows will finish installing the provisioned package, I decide to just let File Explorer refresh itself which seems to happen within about 30 seconds of any change in installed packages. There will be duplicate entries in the "show more options" after install as the legacy entry will be removed on first launch of MediaInfo when it checks that the new one is successfully registered.

@cjee21
Copy link
Collaborator Author

cjee21 commented Nov 29, 2024

https://mediaarea.net/download/snapshots/binary/mediainfo-gui/20241129-2/

The 32/64 combined installer has a bug. Some files are wrongly placed in assets folder. Think I overlooked something in the script regarding SetOutPath.

Other things look good. Version numbers are correct and updating from previous snapshot correctly updates the package.

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.

Compatible with Win11 new context menu (suggestion)
3 participants