ShellExt: Support MTP files - #1289
Conversation
|
Also added debugging so it can be enabled and we can see what is going on. |
There was a problem hiding this comment.
Pull request overview
Updates the Windows Shell Extension so File Explorer context-menu integration works for media files on non-filesystem locations (e.g., MTP devices), aligning behavior with prior versions and addressing issue #1286.
Changes:
- Adds an MTP/non-local fallback in
GetState()by readingPKEY_FileExtensionviaIShellItem2whenSIGDN_FILESYSPATHis unavailable. - Adds an MTP/non-local fallback in
Invoke()by usingSIGDN_DESKTOPABSOLUTEPARSINGwhen a filesystem path cannot be retrieved. - Introduces optional debug logging (guarded by
MEDIAINFO_DEBUG) and addspropkey.hto the PCH.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| Source/WindowsShellExtension/pch.h | Adds propkey.h include for PKEY_FileExtension support. |
| Source/WindowsShellExtension/dllmain.cpp | Adds non-local (MTP) handling for extension detection and invocation; introduces optional debug logging. |
Comments suppressed due to low confidence (1)
Source/WindowsShellExtension/dllmain.cpp:700
- In the multi-select command builder, if both SIGDN_FILESYSPATH and SIGDN_DESKTOPABSOLUTEPARSING fail for an item,
filepathremains empty but is still appended to the command line. This can add empty arguments (or a trailing space) and potentially change MediaInfo behavior; skip appending when no usable path/parsing name was obtained.
else {
// It may be an MTP or other item with parsing name relative to the desktop
wil::unique_cotaskmem_string parsingName;
HRESULT hrParsing = item->GetDisplayName(SIGDN_DESKTOPABSOLUTEPARSING, &parsingName);
if (SUCCEEDED(hrParsing)) {
filepath = parsingName.get();
DebugPrintW("[MediaInfoShellExt] Non-local path: %s\n", filepath.wstring().c_str());
}
}
// Append the item path to the existing command, adding quotes and escapes as needed
command = wil::str_printf<std::wstring>(LR"-(%s %s)-", command.c_str(), QuoteForCommandLineArg(filepath.wstring()).c_str());
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (SUCCEEDED(hrParsing)) { | ||
| filepath = parsingName.get(); | ||
| } | ||
| } |
There was a problem hiding this comment.
@JeromeMartinez What do you prefer? launch MediaInfo with empty so that user knows something gone wrong (MediaInfo usually is empty when failed to open file) or do not launch and user may wonder what happened, "did I click properly", "do I have to wait longer" etc. or if only one fail, "why I select 5 files but only 4 windows open".
Lower down with the loop for single instance, even if we do not append when fail, it will end up launching as well even if not a single file succeeds.
So far I have never seen it fail.
There was a problem hiding this comment.
launch MediaInfo with empty so that user knows something gone wrong
Is fine for me.
Revert part of commit 7db573c as it has no much benefit and causes regression on some systems.
Resolve #1286 and at the same time fix #1290