Program Plugin: Refactor ShellLinkReader and fix argument/description retrieval#4490
Open
DavidGBrett wants to merge 14 commits into
Open
Program Plugin: Refactor ShellLinkReader and fix argument/description retrieval#4490DavidGBrett wants to merge 14 commits into
DavidGBrett wants to merge 14 commits into
Conversation
…uments retrieval in ShellLinkHelper
…_PATH in ShellLinkHelper MAX_PATH can crop the description - INFOTIPSIZE is larger and the documented maximum length after Windows 2000
…tore access for arguments This is the recommended method from win 7 onwards, as the old method could truncate the string fix retrieveArguments param
…with those from PInvoke
…n immutable ShellLinkReadResult from Read ShellLinkReadResult is a record that contains the target path, description, and arguments - this is better and more consistent than getting target path as return value and the others as mutable fields on the helper
This better reflects its purpose as the only thing it does is parse the shell link and get information from it - a single public read method
… by CsWin32 in ShellLinkReader
DavidGBrett
force-pushed
the
fix-shell-link-buffer-issue
branch
from
722068a to
08df0ca
Compare
DavidGBrett
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactors .lnk handling: adds ShellLinkReadResult and native symbols, implements ShellLinkReader (uses IShellLinkW + IPropertyStore to extract path/description/arguments), replaces ShellLinkHelper usage in Win32.cs. ChangesShell Link Reader Refactoring
Sequence Diagram(s)sequenceDiagram
participant LnkProgram as LnkProgram
participant Reader as ShellLinkReader
participant IShellLinkW as IShellLinkW
participant IPropertyStore as IPropertyStore
LnkProgram->>Reader: Read(path)
Reader->>IShellLinkW: IPersistFile.Load & Resolve (SLR_NO_UI)
Reader->>IShellLinkW: GetPath() -> TargetPath
Reader->>IShellLinkW: GetDescription() -> Description
Reader->>IPropertyStore: GetValue(PKEY_Link_Arguments)
IPropertyStore->>Reader: PROPVARIANT (vt, pwszVal)
Reader->>LnkProgram: ShellLinkReadResult(TargetPath, Description, Arguments)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b9744b13-d357-46b6-ae43-4ae6ac072992
📒 Files selected for processing (5)
Plugins/Flow.Launcher.Plugin.Program/NativeMethods.txtPlugins/Flow.Launcher.Plugin.Program/Programs/ShellLinkHelper.csPlugins/Flow.Launcher.Plugin.Program/Programs/ShellLinkReadResult.csPlugins/Flow.Launcher.Plugin.Program/Programs/ShellLinkReader.csPlugins/Flow.Launcher.Plugin.Program/Programs/Win32.cs
💤 Files with no reviewable changes (1)
- Plugins/Flow.Launcher.Plugin.Program/Programs/ShellLinkHelper.cs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Plugins/Flow.Launcher.Plugin.Program/Programs/ShellLinkReader.cs (2)
88-93: ⚡ Quick winKeep
GetDescriptionfailures on the existing known-error path.
ProgramLogger.IsKnownWinProgramErroronly treatsGetDescriptionexceptions as known when the logged calling method isLnkProgram. Logging this asretrieveDescriptionwill reclassify the long-standingMiracastView.lnkfailure asUNKNOWNand add noise. Either preserve the old token here or widen the logger mapping.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Plugins/Flow.Launcher.Plugin.Program/Programs/ShellLinkReader.cs` around lines 88 - 93, The logged calling token must remain the same so GetDescription exceptions stay classified as known; change the ProgramLogger.LogException call in ShellLinkReader.GetDescription to use the original "LnkProgram" (or the exact token used by ProgramLogger.IsKnownWinProgramError) instead of "retrieveDescription", keeping the existing message structure and passing the exception e so IsKnownWinProgramError continues to recognize MiracastView.lnk failures.
32-38: ⚡ Quick winReturn an explicit empty result instead of
default.
return default;makes the failure contract depend onShellLinkReadResult's declaration and default-state semantics.Win32.LnkProgramdereferences the result immediately, so returning an initialized empty result is safer and keeps this API stable.Suggested change
catch (COMException e) { ProgramLogger.LogException( $"|IShellLinkW|Read|{path}|Error occurred while loading or resolving shell link", e ); - return default; + return new ShellLinkReadResult(string.Empty, string.Empty, string.Empty); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Plugins/Flow.Launcher.Plugin.Program/Programs/ShellLinkReader.cs` around lines 32 - 38, The catch block for COMException in ShellLinkReader.cs currently returns default which makes caller behavior fragile; replace the return default with an explicit, initialized ShellLinkReadResult instance (e.g., a failure/empty result) so Win32.LnkProgram doesn't dereference an unset value—construct and return a ShellLinkReadResult populated with safe defaults (Success=false or equivalent, empty/nullable target/path fields set appropriately, and any error metadata) instead of relying on default(T); update the return in the COMException handler that surrounds ProgramLogger.LogException to return that explicit empty result.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Plugins/Flow.Launcher.Plugin.Program/Programs/ShellLinkReader.cs`:
- Around line 88-93: The logged calling token must remain the same so
GetDescription exceptions stay classified as known; change the
ProgramLogger.LogException call in ShellLinkReader.GetDescription to use the
original "LnkProgram" (or the exact token used by
ProgramLogger.IsKnownWinProgramError) instead of "retrieveDescription", keeping
the existing message structure and passing the exception e so
IsKnownWinProgramError continues to recognize MiracastView.lnk failures.
- Around line 32-38: The catch block for COMException in ShellLinkReader.cs
currently returns default which makes caller behavior fragile; replace the
return default with an explicit, initialized ShellLinkReadResult instance (e.g.,
a failure/empty result) so Win32.LnkProgram doesn't dereference an unset
value—construct and return a ShellLinkReadResult populated with safe defaults
(Success=false or equivalent, empty/nullable target/path fields set
appropriately, and any error metadata) instead of relying on default(T); update
the return in the COMException handler that surrounds ProgramLogger.LogException
to return that explicit empty result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d261c827-570a-4957-ba37-29944f5491b2
📒 Files selected for processing (1)
Plugins/Flow.Launcher.Plugin.Program/Programs/ShellLinkReader.cs
Not necessary currently but defensive addition in case something goes wrong on the com side or no exception is thrown
DavidGBrett
force-pushed
the
fix-shell-link-buffer-issue
branch
from
7eb3adb to
1f1d233
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #4486
Bug Fixes
Rework
Summary by cubic
Refactors .lnk parsing into a new
ShellLinkReaderusingCsWin32interop and separate, correctly sized buffers. Fixes truncated descriptions and arguments, simplifies usage inWin32.cs, and corrects logger class names.Summary of changes
IPropertyStore/PKEY_Link_Arguments(replacesIShellLinkW.GetArguments); description usesPInvoke.INFOTIPSIZE; target usesPInvoke.MAX_PATH; separatestackallocbuffers per field and cleared before use; resolve withSLR_NO_UI; improvedCOMExceptionhandling;LogExceptionmessages use the correct class name.ShellLinkReader.Read(path)returning immutableShellLinkReadResult; dedicated methods for target/description/arguments;NativeMethods.txtentries forShellLink,STGM,IPropertyStore,PROPERTYKEY,PROPVARIANT,PKEY_Link_Arguments,PropVariantClear,VARENUM,INFOTIPSIZE,MAX_PATH.ShellLinkHelper, oldIShellLinkW.GetArgumentspath, and ad-hocStringBuilder/constant handling inWin32.cs.stackallocbuffers, explicitPInvoke.PropVariantClear, and COM releases.SLR_NO_UIavoids UI prompts; better cleanup reduces interop risk.Release Note
Shortcuts now show full descriptions and command-line options without being cut off.
Written for commit 1de6d75. Summary will update on new commits. Review in cubic