Replace DllImport and flags with CSWin32 & Remove useless DllImport and flags & Fix right Win key click issue #3122
Conversation
|
🥷 Code experts: no user matched threshold 10 Yusyuriv has most 🧠 knowledge in the files. See details
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame: To learn more about /:\ gitStream - Visit our Docs |
This comment has been minimized.
This comment has been minimized.
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant changes across multiple files in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (14)
Flow.Launcher/Flow.Launcher.csproj (1)
93-96: Document the package's purpose in a comment.Consider adding a comment explaining that this package is used for generating P/Invoke code, similar to the existing comment for ModernWpfUI below.
+ <!-- Microsoft.Windows.CsWin32 provides source generators for Windows API P/Invoke code --> <PackageReference Include="Microsoft.Windows.CsWin32" Version="0.3.106"> <PrivateAssets>all</PrivateAssets> <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets> </PackageReference>Flow.Launcher.Plugin/NativeMethods.txt (1)
1-3: Consider adding complementary Windows API functions.For robust window enumeration and text handling, consider adding these complementary functions:
IsWindow: Verifies if a window handle is validIsWindowVisible: Checks window visibilityGetClassName: Gets window class name for filteringPlugins/Flow.Launcher.Plugin.Sys/NativeMethods.txt (1)
4-5: Consider adding more error handling constants.While S_OK and E_UNEXPECTED are good baseline constants, consider adding other relevant HRESULT constants for more specific error handling:
- ERROR_ACCESS_DENIED
- ERROR_CANCELLED
- ERROR_INSUFFICIENT_BUFFER
Plugins/Flow.Launcher.Plugin.Sys/Main.cs (1)
249-249: UseHWND.Nullinstead ofnew()when passing a null window handleFor clarity and best practices, consider using
HWND.Nullwhen passing a null window handle toSHEmptyRecycleBin.Apply this diff to improve code clarity:
-var result = PInvoke.SHEmptyRecycleBin(new(), string.Empty, 0); +var result = PInvoke.SHEmptyRecycleBin(HWND.Null, string.Empty, 0);Flow.Launcher.Infrastructure/Image/ThumbnailReader.cs (4)
30-30: Consider removing or refining the external URL in the commentThe comment includes a direct link to an external website (StackOverflow). While it's acceptable to reference sources, embedding URLs in the code may not be ideal as they might become inaccessible over time.
Consider summarizing the relevant information from the source directly in the comment or moving the reference to the documentation or a README file.
32-32: SimplifySHCreateItemFromParsingNamecall by specifying the interface directlyYou can streamline the code by specifying the interface type directly in the
SHCreateItemFromParsingNamecall, eliminating the need forGUID_IShellItem.Apply the following changes:
- Remove the declaration of
GUID_IShellItem:-private static readonly Guid GUID_IShellItem = typeof(IShellItem).GUID;
- Modify the
SHCreateItemFromParsingNamecall:-var retCode = PInvoke.SHCreateItemFromParsingName( +IShellItem nativeShellItem = PInvoke.SHCreateItemFromParsingName<IShellItem>( fileName, null, - GUID_IShellItem, - out var nativeShellItem);This leverages the generic overload provided by
CsWin32, improving readability and simplifying error handling.Also applies to: 53-57
59-60: AllowSHCreateItemFromParsingNameto handle exceptionsInstead of manually checking the
HRESULTand throwing an exception, consider using the version ofSHCreateItemFromParsingNamethat throws an exception on failure. This simplifies the code and ensures consistent error handling.Modify the code as follows:
-var retCode = PInvoke.SHCreateItemFromParsingName<IShellItem>( +IShellItem nativeShellItem = PInvoke.SHCreateItemFromParsingName<IShellItem>( fileName, null, - out var nativeShellItem); -if (retCode != HRESULT.S_OK) - throw Marshal.GetExceptionForHR(retCode);This change assumes that
SHCreateItemFromParsingNameis configured to throw exceptions on failure in theCsWin32settings.
Line range hint
14-26: UseSIIGBFdirectly instead of definingThumbnailOptionsSince
ThumbnailOptionsmirrors theSIIGBFenum from the Windows API, you can simplify the code by usingSIIGBFdirectly. This reduces duplication and potential inconsistencies.Apply the following changes:
- Remove the
ThumbnailOptionsenum:-/// <summary> -/// Subclass of <see cref="Windows.Win32.UI.Shell.SIIGBF"/> -/// </summary> -[Flags] -public enum ThumbnailOptions -{ - None = 0x00, - BiggerSizeOk = 0x01, - InMemoryOnly = 0x02, - IconOnly = 0x04, - ThumbnailOnly = 0x08, - InCacheOnly = 0x10, -}
- Update method signatures to use
SIIGBF:-public static BitmapSource GetThumbnail(string fileName, int width, int height, ThumbnailOptions options) +public static BitmapSource GetThumbnail(string fileName, int width, int height, SIIGBF options) -private static unsafe HBITMAP GetHBitmap(string fileName, int width, int height, ThumbnailOptions options) +private static unsafe HBITMAP GetHBitmap(string fileName, int width, int height, SIIGBF options)
- Update method calls:
-imageFactory.GetImage(size, (SIIGBF)options, &hBitmap); +imageFactory.GetImage(size, options, &hBitmap);
- Update the fallback option in the catch block:
-imageFactory.GetImage(size, (SIIGBF)ThumbnailOptions.IconOnly, &hBitmap); +imageFactory.GetImage(size, SIIGBF.ICONONLY, &hBitmap);This change uses the existing
SIIGBFenum, enhancing code clarity and maintainability.Also applies to: 79-80, 84-84
Flow.Launcher/Helper/DWMDropShadow.cs (1)
47-52: Enhance code readability and maintainability
Use
sizeof(int)instead of hardcoding the size: Replace the hardcoded value4withsizeof(int)to ensure correctness and portability.Specify types explicitly in
newexpressions: Instead of usingnew(helper.Handle), explicitly specify the type to improve readability.Apply this diff:
int val = 2; -int ret1 = PInvoke.DwmSetWindowAttribute(new(helper.Handle), DWMWINDOWATTRIBUTE.DWMWA_NCRENDERING_POLICY, &val, 4); +int ret1 = PInvoke.DwmSetWindowAttribute(new HWND(helper.Handle), DWMWINDOWATTRIBUTE.DWMWA_NCRENDERING_POLICY, &val, (uint)sizeof(int)); var m = new MARGINS { cyBottomHeight = 0, cxLeftWidth = 0, cxRightWidth = 0, cyTopHeight = 0 }; -int ret2 = PInvoke.DwmExtendFrameIntoClientArea(new(helper.Handle), &m); +int ret2 = PInvoke.DwmExtendFrameIntoClientArea(new HWND(helper.Handle), &m);Plugins/Flow.Launcher.Plugin.Program/Programs/ShellLinkHelper.cs (1)
Line range hint
25-75: Ensure availability of removed enums and structsThe enums
SLGP_FLAGSand structWIN32_FIND_DATAWhave been removed from the class but are still referenced in theretrieveTargetPathmethod. Ensure that these types are imported from the appropriate namespaces inWindows.Win32, such asWindows.Win32.UI.ShellandWindows.Win32.Storage.FileSystem, or redefine them if necessary to prevent compilation errors.Flow.Launcher.Infrastructure/Hotkey/GlobalHotkey.cs (1)
40-56: Simplify calls toGetKeyStateby removing unnecessary castsThe
PInvoke.GetKeyStatemethod acceptsVIRTUAL_KEYdirectly without the need to cast toint. Removing the casts improves code readability.Apply this diff:
if ((PInvoke.GetKeyState((int)VIRTUAL_KEY.VK_SHIFT) & 0x8000) != 0) +if ((PInvoke.GetKeyState(VIRTUAL_KEY.VK_SHIFT) & 0x8000) != 0) { // SHIFT is pressed state.ShiftPressed = true; } if ((PInvoke.GetKeyState((int)VIRTUAL_KEY.VK_CONTROL) & 0x8000) != 0) +if ((PInvoke.GetKeyState(VIRTUAL_KEY.VK_CONTROL) & 0x8000) != 0) { // CONTROL is pressed state.CtrlPressed = true; } if ((PInvoke.GetKeyState((int)VIRTUAL_KEY.VK_MENU) & 0x8000) != 0) +if ((PInvoke.GetKeyState(VIRTUAL_KEY.VK_MENU) & 0x8000) != 0) { // ALT is pressed state.AltPressed = true; } if ((PInvoke.GetKeyState((int)VIRTUAL_KEY.VK_LWIN) & 0x8000) != 0 || (PInvoke.GetKeyState((int)VIRTUAL_KEY.VK_RWIN) & 0x8000) != 0) +if ((PInvoke.GetKeyState(VIRTUAL_KEY.VK_LWIN) & 0x8000) != 0 || + (PInvoke.GetKeyState(VIRTUAL_KEY.VK_RWIN) & 0x8000) != 0) { // WIN is pressed state.WinPressed = true; }Flow.Launcher/Helper/WindowsInteropHelper.cs (1)
Line range hint
42-67: Refactor 'IsWindowFullscreen' to eliminate 'unsafe' codeThe
IsWindowFullscreenmethod can be refactored to remove theunsafecontext by utilizing managed types likeStringBuilder. This enhances code safety and maintainability.Apply this diff to refactor the code:
- public unsafe static bool IsWindowFullscreen() + public static bool IsWindowFullscreen() { // get current active window var hWnd = PInvoke.GetForegroundWindow(); if (hWnd.Equals(HWND.Null)) { return false; } // if current active window is desktop or shell, exit early if (hWnd.Equals(HWND_DESKTOP) || hWnd.Equals(HWND_SHELL)) { return false; } - string windowClass; - int capacity = 256; - char[] buffer = new char[capacity]; - fixed (char* pBuffer = buffer) - { - PInvoke.GetClassName(hWnd, pBuffer, capacity); - int validLength = Array.IndexOf(buffer, '\0'); - if (validLength < 0) validLength = capacity; - windowClass = new string(buffer, 0, validLength); - } + int capacity = 256; + var sb = new StringBuilder(capacity); + PInvoke.GetClassName(hWnd, sb, capacity); + var windowClass = sb.ToString(); // for Win+Tab (Flip3D) if (windowClass == WINDOW_CLASS_WINTAB) { return false; } PInvoke.GetWindowRect(hWnd, out var appBounds); // for console (ConsoleWindowClass), we have to check for negative dimensions if (windowClass == WINDOW_CLASS_CONSOLE) { return appBounds.top < 0 && appBounds.bottom < 0; } // for desktop (Progman or WorkerW, depends on the system), we have to check if (windowClass is WINDOW_CLASS_PROGMAN or WINDOW_CLASS_WORKERW) { var hWndDesktop = PInvoke.FindWindowEx(hWnd, HWND.Null, "SHELLDLL_DefView", null); hWndDesktop = PInvoke.FindWindowEx(hWndDesktop, HWND.Null, "SysListView32", "FolderView"); if (!hWndDesktop.Equals(IntPtr.Zero)) { return false; } } Rectangle screenBounds = Screen.FromHandle(hWnd).Bounds; return (appBounds.bottom - appBounds.top) == screenBounds.Height && (appBounds.right - appBounds.left) == screenBounds.Width; }Plugins/Flow.Launcher.Plugin.Program/NativeMethods.txt (1)
6-10: Consider adding additional shell link constantsFor completeness, consider adding these commonly used shell link constants:
STGM_READSLGP_SHORTPATHSLGP_UNCPRIORITYFlow.Launcher/MainWindow.xaml.cs (1)
692-692: Consider error handling for GetForegroundWindow.While the code correctly accesses the handle value, consider adding error handling as GetForegroundWindow can return null in certain scenarios (e.g., no foreground window).
- var foregroundWindowHandle = PInvoke.GetForegroundWindow().Value; + var foregroundWindow = PInvoke.GetForegroundWindow(); + var foregroundWindowHandle = foregroundWindow.Value; + if (foregroundWindowHandle == IntPtr.Zero) + { + // Fallback to primary screen + return Screen.PrimaryScreen; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (27)
Flow.Launcher.Infrastructure/FileExplorerHelper.cs(2 hunks)Flow.Launcher.Infrastructure/Flow.Launcher.Infrastructure.csproj(2 hunks)Flow.Launcher.Infrastructure/Hotkey/GlobalHotkey.cs(3 hunks)Flow.Launcher.Infrastructure/Hotkey/InterceptKeys.cs(0 hunks)Flow.Launcher.Infrastructure/Hotkey/KeyEvent.cs(1 hunks)Flow.Launcher.Infrastructure/Image/ThumbnailReader.cs(3 hunks)Flow.Launcher.Infrastructure/NativeMethods.txt(1 hunks)Flow.Launcher.Plugin/Flow.Launcher.Plugin.csproj(2 hunks)Flow.Launcher.Plugin/NativeMethods.txt(1 hunks)Flow.Launcher.Plugin/SharedCommands/ShellCommand.cs(2 hunks)Flow.Launcher/Flow.Launcher.csproj(1 hunks)Flow.Launcher/Helper/DWMDropShadow.cs(2 hunks)Flow.Launcher/Helper/SingleInstance.cs(0 hunks)Flow.Launcher/Helper/WallpaperPathRetrieval.cs(1 hunks)Flow.Launcher/Helper/WindowsInteropHelper.cs(3 hunks)Flow.Launcher/MainWindow.xaml.cs(3 hunks)Flow.Launcher/NativeMethods.txt(1 hunks)Plugins/Flow.Launcher.Plugin.ProcessKiller/Flow.Launcher.Plugin.ProcessKiller.csproj(1 hunks)Plugins/Flow.Launcher.Plugin.ProcessKiller/NativeMethods.txt(1 hunks)Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessHelper.cs(2 hunks)Plugins/Flow.Launcher.Plugin.Program/Flow.Launcher.Plugin.Program.csproj(2 hunks)Plugins/Flow.Launcher.Plugin.Program/NativeMethods.txt(1 hunks)Plugins/Flow.Launcher.Plugin.Program/Programs/ShellLinkHelper.cs(3 hunks)Plugins/Flow.Launcher.Plugin.Program/Programs/ShellLocalization.cs(2 hunks)Plugins/Flow.Launcher.Plugin.Sys/Flow.Launcher.Plugin.Sys.csproj(1 hunks)Plugins/Flow.Launcher.Plugin.Sys/Main.cs(5 hunks)Plugins/Flow.Launcher.Plugin.Sys/NativeMethods.txt(1 hunks)
💤 Files with no reviewable changes (2)
- Flow.Launcher.Infrastructure/Hotkey/InterceptKeys.cs
- Flow.Launcher/Helper/SingleInstance.cs
✅ Files skipped from review due to trivial changes (1)
- Flow.Launcher.Infrastructure/NativeMethods.txt
🔇 Additional comments (24)
Flow.Launcher/Flow.Launcher.csproj (1)
93-96: Verify the CSWin32 package version selection.
The addition of Microsoft.Windows.CsWin32 aligns well with the PR objective of replacing DllImport. However, let's verify the version choice.
✅ Verification successful
Version 0.3.106 is appropriate for this project
The selected version 0.3.106 is actually the latest stable release of Microsoft.Windows.CsWin32 (published on 2024-05-10), making it a suitable choice. This version represents a progression from the beta releases and is more stable than the previously mentioned 0.3.49-beta. The version choice aligns well with production use in the Flow.Launcher project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check NuGet for latest stable and preview versions
curl -s https://api.nuget.org/v3/registration5-semver1/microsoft.windows.cswin32/index.json | jq -r '.items[0].items[].catalogEntry | select(.version) | {version: .version, published: .published}'
# Check for any known issues or breaking changes
gh search issues --repo microsoft/CsWin32 --state all --json title,url,state --query 'breaking changes in:title'
Length of output: 5319
Plugins/Flow.Launcher.Plugin.Sys/Flow.Launcher.Plugin.Sys.csproj (1)
62-62: Verify CSWin32 package version compatibility
Let's verify the compatibility and latest stable version of the Microsoft.Windows.CsWin32 package.
✅ Verification successful
CSWin32 package version is consistent and up-to-date
The Microsoft.Windows.CsWin32 package version 0.3.106 is:
- Currently the latest stable version available on NuGet
- Used consistently across all 6 project files in the codebase
- Properly configured with
PrivateAssetsin all locations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check latest version and release notes
curl -s https://api.nuget.org/v3/registration5-semver1/microsoft.windows.cswin32/index.json | jq -r '.items[0].items[-1].catalogEntry | {version, description}'
# Check if this version is used in other parts of the codebase
rg -l "Microsoft.Windows.CsWin32"
Length of output: 663
Script:
#!/bin/bash
# Check the version of Microsoft.Windows.CsWin32 used in other project files
for file in $(rg -l "Microsoft.Windows.CsWin32"); do
echo "=== $file ==="
rg -A 1 "Microsoft.Windows.CsWin32" "$file"
done
Length of output: 2530
Plugins/Flow.Launcher.Plugin.Program/Flow.Launcher.Plugin.Program.csproj (2)
56-58: LGTM! Good practice adding NativeMethods.txt as an additional file.
The explicit declaration of Win32 APIs in a separate file improves maintainability and makes the API surface clear.
68-71: Verify the CSWin32 package version.
The package configuration is correct with appropriate asset settings. However, let's verify if version 0.3.106 is the most suitable version.
✅ Verification successful
CSWin32 package version is appropriate and consistently used
The version 0.3.106 of Microsoft.Windows.CsWin32 is consistently used across all projects in the solution that reference it, including:
- Flow.Launcher
- Flow.Launcher.Infrastructure
- Flow.Launcher.Plugin
- Flow.Launcher.Plugin.ProcessKiller
- Flow.Launcher.Plugin.Program
- Flow.Launcher.Plugin.Sys
While there are newer beta versions available (up to 0.3.x-beta), using the stable 0.3.106 version is a safer choice for production code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check latest versions and compatibility of Microsoft.Windows.CsWin32
# Check NuGet for latest versions
curl -s https://api.nuget.org/v3/registration5-semver1/microsoft.windows.cswin32/index.json | jq -r '.items[0].items[].catalogEntry.version'
# Check if this version is used consistently across the solution
fd -e csproj -x grep -l "Microsoft.Windows.CsWin32"
Length of output: 940
Script:
#!/bin/bash
# Check the specific version used in each project file
echo "=== Checking CsWin32 versions across projects ==="
for proj in $(fd -e csproj); do
echo "=== $proj ==="
grep -A 2 "Microsoft.Windows.CsWin32" "$proj"
done
Length of output: 6568
Flow.Launcher.Plugin/NativeMethods.txt (2)
1-3: Verify error handling in the codebase.
Since these Windows API functions can fail, ensure proper error handling is implemented.
Let's check the error handling patterns:
✅ Verification successful
Let me gather more context about the implementation to verify the error handling patterns.
Error handling is properly implemented for these Windows API functions
The code shows appropriate error handling:
GetWindowTextreturn value is checked for 0 (failure case) and returns an empty string- The code includes documentation about possible failure cases in comments
Win32Exceptionis used throughout the codebase for Windows API error handlingEnumThreadWindowsis used in a callback pattern where errors are handled by the return value
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check error handling for Windows API calls
# Expected results: Marshal.GetLastWin32Error() or similar error checking
# Search for error handling patterns
rg -A 2 "GetWindowText|EnumThreadWindows"
# Check for Win32 error handling
ast-grep --pattern 'Marshal.GetLastWin32Error()'
Length of output: 1141
Script:
#!/bin/bash
# Let's check the full implementation context
rg -B 5 -A 10 "GetWindowText.*|EnumThreadWindows.*" --type cs
# Also check for any other error handling patterns
rg "Marshal\.GetLastWin32Error|Win32Exception|WindowsError" --type cs
Length of output: 4113
1-3: LGTM! The selected Windows API functions are appropriate for window enumeration and text retrieval.
These functions form a logical set for retrieving window titles during thread window enumeration.
Let's verify the implementation patterns in the codebase:
✅ Verification successful
The Windows API functions are correctly implemented with proper usage patterns
The implementation in ShellCommand.cs demonstrates correct usage of all three Windows API functions:
EnumThreadWindowsis properly used with a callback function (CheckSecurityThread)GetWindowTextLengthis called first to determine the required buffer sizeGetWindowTextis used with proper buffer allocation based on the length- Safe memory handling is implemented using fixed pointers and null termination checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check implementation patterns of window enumeration and text retrieval
# Expected results:
# 1. EnumThreadWindows should be used with a callback
# 2. GetWindowText should be preceded by GetWindowTextLength
# 3. Proper buffer allocation should be used
# Check for window enumeration patterns
ast-grep --pattern 'EnumThreadWindows($_)'
# Check for proper text retrieval patterns
ast-grep --pattern $'GetWindowTextLength($_);
$$$
GetWindowText($_)'
Length of output: 114
Script:
#!/bin/bash
# Let's try a different approach to find these Windows API usages
# First, let's search for any existing files that might be using these APIs
# Search for files containing these Windows API functions
echo "=== Files containing Windows API functions ==="
rg -l "EnumThreadWindows|GetWindowText|GetWindowTextLength"
# Look for any existing P/Invoke declarations or usage patterns
echo -e "\n=== Existing DllImport or usage patterns ==="
rg -A 5 "EnumThreadWindows|GetWindowText|GetWindowTextLength"
# Check for any test files or documentation mentioning these functions
echo -e "\n=== Tests or documentation ==="
fd -e cs -e md -e txt | xargs rg "EnumThreadWindows|GetWindowText|GetWindowTextLength"
Length of output: 3085
Flow.Launcher.Infrastructure/Flow.Launcher.Infrastructure.csproj (3)
38-40: Verify the content and location of NativeMethods.txt
The addition of NativeMethods.txt as an AdditionalFiles is crucial for CSWin32 source generation. This file should contain the list of Win32 APIs that need to be generated.
✅ Verification successful
NativeMethods.txt content is properly configured for CSWin32 migration
The NativeMethods.txt file exists and contains the necessary Win32 API declarations for the project's requirements:
- Shell-related APIs:
SHCreateItemFromParsingName,IShellItem,IShellItemImageFactory - Window hook APIs:
SetWindowsHookEx,UnhookWindowsHookEx,CallNextHookEx - Window message constants:
WM_KEYDOWN,WM_KEYUP,WM_SYSKEYDOWN,WM_SYSKEYUP - Other required Win32 functions:
GetModuleHandle,GetKeyState,EnumWindows
The remaining DllImport usages found are in plugin projects which are separate from the Infrastructure project and not affected by this change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify NativeMethods.txt exists and contains necessary Win32 API declarations
# Check if the file exists and show its content
if [ -f "Flow.Launcher.Infrastructure/NativeMethods.txt" ]; then
echo "File exists. Content:"
cat "Flow.Launcher.Infrastructure/NativeMethods.txt"
else
echo "Warning: NativeMethods.txt not found in expected location"
# Search for the file in case it's in a different location
fd -t f "NativeMethods.txt"
fi
# Search for DllImport usage that might need to be included in NativeMethods.txt
rg "DllImport" -t cs
Length of output: 12792
Line range hint 1-76: Verify target framework compatibility with CSWin32
The project targets net7.0-windows which should be compatible with CSWin32, but let's verify other framework-related settings.
✅ Verification successful
Target framework is compatible with CSWin32
All projects in the solution are targeting .NET 7.0 Windows, with some specifically targeting Windows 10.0.19041.0. This is fully compatible with Microsoft.Windows.CSWin32 0.3.106, which supports .NET Standard 2.0 and newer platforms including .NET 7.0.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for framework compatibility issues in other project files
# Search for target framework declarations in other project files
fd -e csproj | xargs grep -l "TargetFramework"
# Check for any framework-specific conditional compilation
rg "net7.0|netstandard" -t cs
Length of output: 1259
Script:
#!/bin/bash
# Let's check the target framework versions in all csproj files
fd -e csproj -x cat {} | grep -A 1 "<TargetFramework"
# Also check for any CSWin32 package references across all csproj files
fd -e csproj -x cat {} | grep -A 3 "CSWin32"
Length of output: 2048
63-66: Verify CSWin32 package version compatibility and security
The addition of Microsoft.Windows.CsWin32 package is aligned with the PR objective to replace DllImport with CSWin32. However, we should verify the version choice.
✅ Verification successful
Consider upgrading Microsoft.Windows.CsWin32 package to latest stable version
The current version 0.3.106 is the latest stable release available on NuGet. While there are some open issues reported for this version, they are specific to certain API functionalities (AttachConsole, EnumProcessModulesEx, MFEnumDeviceSources) and don't indicate critical security concerns.
Let's verify if the project is using any of these affected APIs:
Version 0.3.106 of Microsoft.Windows.CsWin32 is appropriate for this project
The codebase scan shows that none of the problematic APIs mentioned in the open issues are being used. The project primarily uses common Win32 APIs like window management (GetForegroundWindow, SetWindowLong), hook functions (SetWindowsHookEx), and shell interactions (SHEmptyRecycleBin), which are well-tested and stable in the current version.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for latest stable versions and known issues with Microsoft.Windows.CsWin32
# Check NuGet for latest versions
curl -s https://api.nuget.org/v3-flatcontainer/microsoft.windows.cswin32/index.json | jq -r '.versions[]' | sort -V | tail -n 5
# Check for any GitHub issues related to the specific version
gh api graphql -f query='
{
search(query: "repo:microsoft/CsWin32 is:issue 0.3.106", type: ISSUE, first: 5) {
nodes {
... on Issue {
title
state
url
}
}
}
}'
Length of output: 1178
Script:
#!/bin/bash
# Search for usage of potentially affected APIs
rg -i "AttachConsole|EnumProcessModulesEx|MFEnumDeviceSources|MFTEnumEx|CollectMetadataPaths" --type cs
# Check all Win32 API usages in the codebase
ast-grep --pattern 'PInvoke.$_($$$)'
Length of output: 7157
Plugins/Flow.Launcher.Plugin.Sys/NativeMethods.txt (2)
1-6: LGTM! Clear list of Windows API methods for CSWin32 generation.
The file correctly lists the required Windows API methods and constants needed for system operations. This aligns well with the PR objective of transitioning from DllImport to CSWin32.
1-3: Verify security implications of system-level operations.
These Windows APIs (ExitWindowsEx, LockWorkStation, SHEmptyRecycleBin, SetSuspendState) can perform powerful system-level operations. Ensure proper security checks and user confirmations are implemented in the calling code.
Also applies to: 6-6
✅ Verification successful
Let me gather more context about user confirmations and error handling.
Let me check the implementation of system operations to verify security measures.
Security measures are properly implemented for system operations
The code review shows that appropriate security measures are in place:
- All destructive operations (shutdown, restart, log off) require explicit user confirmation through warning message boxes
- Error handling is implemented for operations like emptying the recycle bin
- Sleep/Lock operations are direct OS calls that are inherently safe and don't require additional confirmation
- Operations use proper Windows API calls through PInvoke rather than unsafe alternatives
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for security checks and user confirmations in the implementation
# Look for security checks and user confirmations
rg -A 5 "ExitWindowsEx|LockWorkStation|SHEmptyRecycleBin|SetSuspendState" --type cs
# Check for UAC/elevation related code
ast-grep --pattern 'new WindowsPrincipal(WindowsIdentity.GetCurrent()).IsInRole(WindowsBuiltInRole.Administrator)'
Length of output: 2740
Script:
#!/bin/bash
# Check for user confirmations and error handling patterns
rg -B 5 -A 10 "Title = \"(Logoff|Lock|Empty Recycle Bin|Hibernate|Sleep)\"" --type cs
# Check if there's any confirmation dialog implementation
rg -A 5 "ShowMsgBox|MessageBox|Confirm" --type cs
# Look for the complete Result class implementation to understand the action execution flow
ast-grep --pattern 'class Result {
$$$
}'
Length of output: 81181
Script:
#!/bin/bash
# Look for the implementation of system operations and their security checks
rg -B 5 -A 10 "Title = \"(Shutdown|Restart|Restart Advanced|Log Off)\"" --type cs
# Check for any additional security-related code around these operations
rg -A 5 "ExitWindowsEx|LockWorkStation|SHEmptyRecycleBin|SetSuspendState" --type cs --glob "!NativeMethods.txt"
Length of output: 5842
Flow.Launcher.Plugin/Flow.Launcher.Plugin.csproj (3)
64-65: LGTM: FodyWeavers.xml inclusion
The inclusion of FodyWeavers.xml in the None ItemGroup is correct and aligns with the existing Fody package reference for property change notifications.
75-78: Consider updating to the latest stable version of CSWin32
The Microsoft.Windows.CsWin32 package version 0.3.106 is being used, but there might be a more recent stable version available. Please verify if this version aligns with other Flow Launcher projects for consistency.
Let's check the version usage across the solution:
✅ Verification successful
Version 0.3.106 of Microsoft.Windows.CsWin32 is consistently used across all projects
The version check reveals that all Flow Launcher projects are using the exact same version (0.3.106) of the Microsoft.Windows.CsWin32 package:
- Flow.Launcher.Plugin
- Flow.Launcher.Plugin.Program
- Flow.Launcher.Plugin.ProcessKiller
- Flow.Launcher.Plugin.Sys
- Flow.Launcher.Infrastructure
- Flow.Launcher
This consistent version usage across the solution indicates proper version alignment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for CSWin32 package references in all project files
echo "Checking CSWin32 versions across all projects:"
rg -A 1 "Microsoft\.Windows\.CsWin32" --glob "*.csproj"
Length of output: 1627
60-61: Verify the contents of NativeMethods.txt
The addition of NativeMethods.txt is crucial for the DllImport to CSWin32 migration. This file should contain the list of Win32 APIs that need to be generated.
Let's verify the contents and existence of this file:
✅ Verification successful
NativeMethods.txt contains the expected Win32 APIs for CSWin32 generation
The file contains the three Win32 APIs required for window enumeration and text retrieval:
EnumThreadWindows: For enumerating windows of a threadGetWindowText: For retrieving window textGetWindowTextLength: For getting the length of window text
NativeMethods.txt correctly lists the Win32 APIs used in the codebase
The file contains the three Win32 APIs that are being migrated from DllImport to CSWin32:
EnumThreadWindows: Used inShellCommand.csfor checking security windowsGetWindowTextandGetWindowTextLength: Used inGetWindowTitlemethod to retrieve window titles
The migration is properly implemented in ShellCommand.cs using the new CSWin32 approach with Windows.Win32.PInvoke instead of DllImport statements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if NativeMethods.txt exists and show its contents
if [ -f "Flow.Launcher.Plugin/NativeMethods.txt" ]; then
echo "Contents of NativeMethods.txt:"
cat "Flow.Launcher.Plugin/NativeMethods.txt"
else
echo "Warning: NativeMethods.txt not found"
# Search for it in case it's in a different location
fd -t f "NativeMethods.txt"
fi
Length of output: 220
Script:
#!/bin/bash
# Let's verify if these APIs were previously used via DllImport
rg "DllImport.*(?:EnumThreadWindows|GetWindowText|GetWindowTextLength)" -g "*.cs"
Length of output: 83
Script:
#!/bin/bash
# Let's try a different approach to find these Win32 API usages
# First, let's search for any file containing these method names
rg -l "EnumThreadWindows|GetWindowText|GetWindowTextLength"
# Then, let's also check for DllImport usage in the codebase
rg "\[DllImport" -A 1
Length of output: 20079
Script:
#!/bin/bash
# Let's check the specific file that uses these Win32 APIs
cat "Flow.Launcher.Plugin/SharedCommands/ShellCommand.cs"
Length of output: 4272
Plugins/Flow.Launcher.Plugin.Sys/Main.cs (1)
10-12: Appropriate inclusion of Windows.Win32 namespaces
The added using directives correctly include the necessary namespaces for the PInvoke calls.
Flow.Launcher.Infrastructure/Image/ThumbnailReader.cs (1)
68-93: Code implementation is correct and follows best practices
The GetHBitmap method correctly handles resource management with appropriate use of try and finally blocks. The use of unsafe code is necessary due to pointer operations required by the Windows API, and the logic handles exceptions effectively.
Plugins/Flow.Launcher.Plugin.ProcessKiller/NativeMethods.txt (1)
1-2: Verify process access security implications
The combination of QueryFullProcessImageName and OpenProcess is appropriate for process path resolution. However, ensure that:
- Process access rights are properly restricted when using
OpenProcess - Error handling is implemented for access denied scenarios
✅ Verification successful
Let me verify the security implications further by checking the complete implementation of the process handling code.
Let me check the complete context of the process handling method to ensure proper error handling and resource cleanup.
Let me try a different pattern to get the method implementation.
Let me try one final approach to get the process handling code.
Process access implementation is secure and well-implemented
The implementation in ProcessHelper.cs demonstrates secure process access practices:
- Uses
PROCESS_QUERY_LIMITED_INFORMATIONwhich is the minimum required access right - Properly handles access denied scenarios by returning empty string
- Implements proper resource cleanup using
SafeProcessHandle - Has appropriate error handling with try-catch blocks
- The combination of
QueryFullProcessImageNameandOpenProcessin NativeMethods.txt is complete for the intended functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if OpenProcess is used with minimum required access rights
rg -A 5 "OpenProcess" --type cs
Length of output: 644
Script:
#!/bin/bash
# Check the complete implementation of process handling
ast-grep --pattern 'var handle = PInvoke.OpenProcess($$$)'
Length of output: 249
Script:
#!/bin/bash
# Get the complete method implementation
ast-grep --pattern 'GetProcessPath($$$) {
$$$
}'
Length of output: 52
Script:
#!/bin/bash
# Get the complete method implementation using ripgrep
rg -U "GetProcessPath.*?\{.*?\}" -A 20 --multiline --type cs
Length of output: 62
Script:
#!/bin/bash
# Get the content of ProcessHelper.cs
cat Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessHelper.cs
Length of output: 3882
Plugins/Flow.Launcher.Plugin.Program/NativeMethods.txt (1)
1-5: Consider security implications of environment string expansion
The ExpandEnvironmentStrings API can expose sensitive environment variables. Ensure that:
- Expanded strings are properly sanitized before use
- No sensitive information is leaked through environment variables
Flow.Launcher/NativeMethods.txt (1)
3-14: Consider additional window-related constants and messages
For completeness, consider adding these commonly used window constants:
WM_ACTIVATEWM_WINDOWPOSCHANGEDWM_SETFOCUSWS_EX_TOOLWINDOW
Also, ensure proper error handling when using SetForegroundWindow as it can fail due to Windows focus assault prevention.
Flow.Launcher.Infrastructure/Hotkey/KeyEvent.cs (1)
1-1: LGTM! Good improvement in code maintainability
The changes effectively replace magic numbers with type-safe PInvoke constants from Windows.Win32, which:
- Improves code maintainability
- Reduces the risk of errors from hardcoded values
- Provides better type safety
Also applies to: 10-10, 15-15, 20-20, 25-25
Plugins/Flow.Launcher.Plugin.ProcessKiller/Flow.Launcher.Plugin.ProcessKiller.csproj (1)
53-58: Verify package version compatibility and security
The addition of Microsoft.Windows.CsWin32 aligns with the PR objective. However, let's verify:
- If this is the latest stable version
- If there are any known security issues
✅ Verification successful
Package version is consistent and up-to-date
The version 0.3.106 of Microsoft.Windows.CsWin32 is:
- The latest stable release (not beta) according to NuGet feed
- Consistently used across all projects in the codebase that reference it
- Has no known security vulnerabilities according to GitHub Security Advisories
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check latest version and security advisories for Microsoft.Windows.CsWin32
# Get latest version from NuGet
curl -s https://api.nuget.org/v3-flatcontainer/Microsoft.Windows.CsWin32/index.json | jq -r '.versions[-1]'
# Check for any security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NUGET, package: "Microsoft.Windows.CsWin32") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 527
Script:
#!/bin/bash
# Let's try a different approach to check the latest version
# Using a simpler curl command to get the raw index
curl -s "https://api.nuget.org/v3-flatcontainer/microsoft.windows.cswin32/index.json"
# Also, let's check if this version is used in other parts of the codebase
rg -l "Microsoft\.Windows\.CsWin32" --type csproj
Length of output: 850
Script:
#!/bin/bash
# Let's try searching with a different approach for csproj files
fd -e csproj -x grep -l "Microsoft.Windows.CsWin32" {}
Length of output: 440
Script:
#!/bin/bash
# Let's check the version used in other project files
for file in $(fd -e csproj); do
echo "=== $file ==="
grep -A 3 "Microsoft.Windows.CsWin32" "$file" || true
done
Length of output: 6680
Flow.Launcher.Infrastructure/FileExplorerHelper.cs (1)
5-5: LGTM! Good transition to CSWin32
The changes successfully:
- Replace DllImport with PInvoke.EnumWindows
- Update HWND handling to use the safer Value property
- Maintain existing functionality while improving type safety
Let's verify the HWND handling consistency across the codebase:
Also applies to: 69-71
✅ Verification successful
Let me gather more information to verify the HWND handling across the codebase.
Based on the search results, I can now generate the final response since I have enough information to verify the HWND handling across the codebase.
HWND handling is consistent with CSWin32 usage
The codebase shows consistent HWND handling patterns:
- All HWND comparisons use
Equals()or!=withHWND.Null - No instances of
ToInt32()found on HWND objects - Proper usage of
Valueproperty when needed (e.g., in FileExplorerHelper.cs)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining ToInt32() calls on HWND objects
# and verify consistent Value property usage
# Search for potential HWND conversions
rg "ToInt32\(\)" -A 2 -B 2
# Search for HWND usage patterns
ast-grep --pattern 'HWND.$$$'
Length of output: 742
Script:
#!/bin/bash
# Search for all HWND related code patterns
rg "HWND" -A 3 -B 3
# Search specifically for Value property access on HWND
rg "\.Value" -A 2 -B 2
Length of output: 86128
Flow.Launcher/MainWindow.xaml.cs (2)
29-29: LGTM! Using CSWin32 for improved type safety.
The change from System.Runtime.InteropServices to Windows.Win32 aligns with modern best practices for Windows API interop.
424-424: Verify handle lifetime management.
The handle wrapping looks correct, but ensure the underlying handle is properly managed.
✅ Verification successful
Let me gather more information about the handle management patterns in the codebase.
Handle management appears to be properly implemented
The handle management is correctly implemented through the following patterns:
HwndSourceis obtained viaPresentationSource.FromVisual()which is managed by WPF's lifecycle- Proper disposal pattern is used with
usingstatement whereHwndSourceis manually created - The
hwndSource.Handleusage inSetForegroundWindowis safe as the handle lifetime is tied to the WPF window
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for proper handle cleanup patterns in the codebase
rg -A 5 "SetForegroundWindow|GetForegroundWindow"
Length of output: 1924
Script:
#!/bin/bash
# Check for HwndSource usage and disposal patterns
ast-grep --pattern 'HwndSource $_'
# Check for handle related cleanup/disposal patterns
rg -A 5 "HwndSource|Handle.Dispose|Handle.Close"
Length of output: 2080
Plugins/Flow.Launcher.Plugin.Program/Programs/ShellLocalization.cs
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
Flow.Launcher.Infrastructure/Image/ThumbnailReader.cs (1)
34-34: Document the S_ExtractionFailed constantConsider adding a comment explaining what this HRESULT value represents and when it occurs.
- private static readonly HRESULT S_ExtractionFailed = (HRESULT)0x8004B200; + /// <summary> + /// HRESULT value indicating that thumbnail extraction failed. + /// Typically occurs when a thumbnail cannot be generated for a file. + /// </summary> + private static readonly HRESULT S_ExtractionFailed = (HRESULT)0x8004B200;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Flow.Launcher.Infrastructure/Image/ThumbnailReader.cs(3 hunks)
🔇 Additional comments (6)
Flow.Launcher.Infrastructure/Image/ThumbnailReader.cs (6)
4-10: LGTM: Clean transition to CSWin32 imports
The new using directives appropriately support the transition from DllImport to CSWin32, with specific imports that align with the PR objectives.
14-16: LGTM: Improved documentation clarity
The added XML documentation clearly indicates the relationship between ThumbnailOptions and the Windows API enum SIIGBF.
38-38: LGTM: Improved type safety and resource management
The changes enhance type safety by using HBITMAP instead of IntPtr, and properly manage resources using CSWin32's PInvoke.DeleteObject.
Also applies to: 47-47
62-67: LGTM: Fixed COM object release issue
The implementation now correctly handles the COM object release by setting nativeShellItem to null after release, addressing the previous review comment about potential double release.
78-86: LGTM: Well-implemented fallback behavior
The code gracefully handles thumbnail extraction failures by falling back to icon extraction when appropriate, with proper exception handling.
51-96: Verify the thumbnail extraction behavior
Let's verify that the changes maintain correct thumbnail extraction behavior across different file types.
✅ Verification successful
Thumbnail extraction behavior maintains proper fallback mechanism
The code changes maintain correct thumbnail extraction behavior:
- Proper fallback from ThumbnailOnly to IconOnly when extraction fails
- Consistent handling across different file types:
- Icon-only for directories
- Full thumbnails for image files
- Standard icons for regular files
- Appropriate error handling with COM exception catching and resource cleanup
The implementation in ImageLoader.cs shows the correct usage patterns and the error handling in ThumbnailReader.cs ensures graceful degradation when thumbnail extraction fails.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests related to thumbnail extraction
rg -l "GetThumbnail.*ThumbnailOptions" --type cs
# Look for any existing issue reports about thumbnail extraction
gh issue list --search "thumbnail extraction"
# Check for any related changes in test files
rg -l "WindowsThumbnailProvider" --type cs
Length of output: 349
Script:
#!/bin/bash
# Check the implementation in ImageLoader.cs to understand the usage
rg -A 10 "GetThumbnail.*ThumbnailOptions" Flow.Launcher.Infrastructure/Image/ImageLoader.cs
# Check test coverage
fd -e cs -e config "Test" --exec rg -l "ThumbnailReader|ImageLoader"
# Look for any error handling or logging related to thumbnail extraction
rg "catch.*ThumbnailOptions" --type cs -A 5
Length of output: 2278
|
Cool! This has been something I want to do for a while. |
|
Would you mind split this into three pr? We can probably get the two bug fix merged earlier. If too hard then let's just keep it here. I think cswin32 will take a while to review (as I want to check the specific generated type). I am a bit messed up lately with my thesis (due probably 12/20) and some research stuff, so probably I will delay the review for that. |
No worries for the time. In those bug fixes, I use PInvoke so I think I should include it here. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessHelper.cs (1)
100-100: Define buffer size as a named constantThe magic number 2000 for buffer capacity should be defined as a named constant at the class level for better maintainability and clarity.
Apply this diff at the class level:
+ private const uint MAX_PATH_LENGTH = 2000;And update the usage:
- uint capacity = 2000; + uint capacity = MAX_PATH_LENGTH;Plugins/Flow.Launcher.Plugin.Program/Programs/ShellLinkHelper.cs (1)
33-34: Extract magic number into a named constantThe value 260 (MAX_PATH) should be defined as a private constant at the class level since it's used multiple times.
+ private const int MAX_PATH = 260; - const int MAX_PATH = 260;Flow.Launcher/Helper/WindowsInteropHelper.cs (3)
Line range hint
42-97: Consider refactoring for improved maintainabilityWhile the implementation is functionally correct, consider breaking down the method into smaller, focused methods to improve readability and maintainability.
Suggested refactoring:
+ private static bool IsSpecialWindowClass(string windowClass) + { + return windowClass == WINDOW_CLASS_WINTAB || + windowClass == WINDOW_CLASS_CONSOLE || + windowClass is WINDOW_CLASS_PROGMAN or WINDOW_CLASS_WORKERW; + } + + private unsafe static string GetWindowClassName(HWND hWnd) + { + int capacity = 256; + char[] buffer = new char[capacity]; + fixed (char* pBuffer = buffer) + { + PInvoke.GetClassName(hWnd, pBuffer, capacity); + int validLength = Array.IndexOf(buffer, '\0'); + if (validLength < 0) validLength = capacity; + return new string(buffer, 0, validLength); + } + } + + private static bool IsDesktopWindow(HWND hWnd, string windowClass) + { + if (windowClass is WINDOW_CLASS_PROGMAN or WINDOW_CLASS_WORKERW) + { + var hWndDesktop = PInvoke.FindWindowEx(hWnd, HWND.Null, "SHELLDLL_DefView", null); + hWndDesktop = PInvoke.FindWindowEx(hWndDesktop, HWND.Null, "SysListView32", "FolderView"); + return !hWndDesktop.Equals(IntPtr.Zero); + } + return false; + }
58-69: Add comments explaining the string handling logicThe string handling approach using fixed buffers is correct, but it would benefit from comments explaining why this specific approach was chosen over alternatives like
new string(pBuffer)orbuffer.ToString().Add the following comments:
+ // We use a fixed buffer approach instead of new string(pBuffer) or buffer.ToString() because: + // 1. new string(pBuffer) might read beyond the null terminator if the API doesn't add one + // 2. buffer.ToString() would return the type name instead of the buffer contents string windowClass; int capacity = 256; char[] buffer = new char[capacity];
106-107: Add error handling for window handle operationsWhile the implementation is correct, consider adding error handling for cases where the window handle might be invalid.
public static void DisableControlBox(Window win) { + if (win == null) throw new ArgumentNullException(nameof(win)); var hwnd = new HWND(new WindowInteropHelper(win).Handle); + if (hwnd.Value == IntPtr.Zero) + { + throw new InvalidOperationException("Failed to get window handle."); + } PInvoke.SetWindowLong(hwnd, WINDOW_LONG_PTR_INDEX.GWL_STYLE, PInvoke.GetWindowLong(hwnd, WINDOW_LONG_PTR_INDEX.GWL_STYLE) & ~(int)WINDOW_STYLE.WS_SYSMENU); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Flow.Launcher.Plugin/SharedCommands/ShellCommand.cs(2 hunks)Flow.Launcher/Helper/WindowsInteropHelper.cs(3 hunks)Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessHelper.cs(2 hunks)Plugins/Flow.Launcher.Plugin.Program/Programs/ShellLinkHelper.cs(3 hunks)
🔇 Additional comments (12)
Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessHelper.cs (4)
1-10: LGTM! Import changes align with CSWin32 transition
The new imports properly support the transition to CSWin32 for Windows API access.
89-117: 🛠️ Refactor suggestion
Refactor to eliminate 'unsafe' code in 'TryGetProcessFilename' method
The use of unsafe code can be avoided by utilizing managed code alternatives, enhancing code safety and maintainability.
109-112:
Correct the usage of 'capacity' after 'QueryFullProcessImageName'
After calling PInvoke.QueryFullProcessImageName, the capacity parameter is updated to contain the length of the string copied into the buffer.
93-99: 🛠️ Refactor suggestion
Improve handle management using a single 'using' statement
The current implementation creates a handle and then wraps it in a SafeProcessHandle. This can be simplified to reduce the chance of handle leaks.
Apply this diff:
- var handle = PInvoke.OpenProcess(PROCESS_ACCESS_RIGHTS.PROCESS_QUERY_LIMITED_INFORMATION, false, (uint)p.Id);
- if (handle.Value == IntPtr.Zero)
- {
- return string.Empty;
- }
- using var safeHandle = new SafeProcessHandle(handle.Value, true);
+ using var safeHandle = PInvoke.OpenProcess(PROCESS_ACCESS_RIGHTS.PROCESS_QUERY_LIMITED_INFORMATION, false, (uint)p.Id);
+ if (safeHandle.IsInvalid)
+ {
+ return string.Empty;
+ }Likely invalid or redundant comment.
Plugins/Flow.Launcher.Plugin.Program/Programs/ShellLinkHelper.cs (2)
5-7: LGTM: Appropriate CSWin32 imports added
The new imports from Windows.Win32 namespaces align with the PR's objective of replacing DllImport with CSWin32, providing type-safe access to Windows APIs.
Line range hint 13-18: LGTM: Clean COM import implementation
The ShellLink class implementation is correct and follows COM interop best practices. The GUID matches the IShellLinkW interface specification.
Flow.Launcher/Helper/WindowsInteropHelper.cs (2)
7-9: LGTM: Improved type safety with CSWin32
The transition from raw IntPtr to strongly-typed HWND and the addition of CSWin32 imports enhances type safety and follows modern Windows API best practices.
Also applies to: 16-17
21-34: LGTM: Clean property implementations
The properties are well-implemented with proper null checks and lazy initialization using the new CSWin32 types and PInvoke class.
Flow.Launcher.Plugin/SharedCommands/ShellCommand.cs (4)
6-7: LGTM! Using directives align with CSWin32 transition
The addition of Windows.Win32 namespaces properly supports the transition from DllImport to CSWin32, providing better type safety for Windows API interactions.
42-42: LGTM! Proper usage of PInvoke.EnumThreadWindows
The conversion to CSWin32's PInvoke is correct, with proper type casting for the thread ID.
45-50: LGTM! Improved type safety with Windows-specific types
The signature change from generic IntPtr to specific HWND and LPARAM types enhances type safety and better represents the Windows API contract.
52-69: 🛠️ Refactor suggestion
Refactor GetWindowTitle to avoid unsafe code
While the current implementation works, using StringBuilder would be safer and eliminate the need for unsafe code and manual null termination handling.
Apply this diff to refactor the code:
- private static unsafe string GetWindowTitle(HWND hwnd)
+ private static string GetWindowTitle(HWND hwnd)
{
- var capacity = PInvoke.GetWindowTextLength(hwnd) + 1;
- char[] buffer = new char[capacity];
- fixed (char* pBuffer = buffer)
- {
- if (PInvoke.GetWindowText(hwnd, (PWSTR)pBuffer, capacity) == 0)
- {
- return string.Empty;
- }
-
- int validLength = Array.IndexOf(buffer, '\0');
- if (validLength < 0) validLength = capacity;
- return new string(buffer, 0, validLength);
- }
+ int capacity = PInvoke.GetWindowTextLength(hwnd) + 1;
+ var sb = new StringBuilder(capacity);
+ if (PInvoke.GetWindowText(hwnd, sb, capacity) == 0)
+ {
+ return string.Empty;
+ }
+ return sb.ToString();
}
Plugins/Flow.Launcher.Plugin.Program/Programs/ShellLinkHelper.cs
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@Jack251970 I add some change to your pr, please take a look. Mainly it does two things:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessHelper.cs (1)
89-114: 🛠️ Refactor suggestionEliminate unsafe code using StringBuilder.
The method currently uses unsafe code with pointer manipulation, which can be replaced with a safer StringBuilder approach as suggested in the previous review.
- public unsafe string TryGetProcessFilename(Process p) + public string TryGetProcessFilename(Process p) { try { - var handle = PInvoke.OpenProcess(PROCESS_ACCESS_RIGHTS.PROCESS_QUERY_LIMITED_INFORMATION, false, (uint)p.Id); - if (handle.Value == IntPtr.Zero) + using var safeHandle = PInvoke.OpenProcess(PROCESS_ACCESS_RIGHTS.PROCESS_QUERY_LIMITED_INFORMATION, false, (uint)p.Id); + if (safeHandle.IsInvalid) { return string.Empty; } - using var safeHandle = new SafeProcessHandle(handle.Value, true); - uint capacity = 2000; - Span<char> buffer = new char[capacity]; - fixed (char* pBuffer = buffer) - { - if (!PInvoke.QueryFullProcessImageName(safeHandle, PROCESS_NAME_FORMAT.PROCESS_NAME_WIN32, (PWSTR)pBuffer, ref capacity)) - { - return string.Empty; - } - - return buffer[..(int)capacity].ToString(); - } + uint capacity = 2000; + var sb = new StringBuilder((int)capacity); + if (!PInvoke.QueryFullProcessImageName(safeHandle, PROCESS_NAME_FORMAT.PROCESS_NAME_WIN32, sb, ref capacity)) + { + return string.Empty; + } + return sb.ToString(); } catch { return string.Empty; } }
🧹 Nitpick comments (9)
Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessHelper.cs (2)
89-89: Add XML documentation for the method.Consider adding XML documentation to describe the method's purpose, parameters, return value, and potential exceptions.
+ /// <summary> + /// Attempts to get the full path of the executable file for the specified process. + /// </summary> + /// <param name="p">The process to query.</param> + /// <returns>The full path of the process executable, or an empty string if the path cannot be retrieved.</returns> public unsafe string TryGetProcessFilename(Process p)
93-96: Improve handle management using using declaration.The current handle management could be improved by using a using declaration directly with OpenProcess.
- var handle = PInvoke.OpenProcess(PROCESS_ACCESS_RIGHTS.PROCESS_QUERY_LIMITED_INFORMATION, false, (uint)p.Id); - if (handle.Value == IntPtr.Zero) - { - return string.Empty; - } - - using var safeHandle = new SafeProcessHandle(handle.Value, true); + using var safeHandle = PInvoke.OpenProcess(PROCESS_ACCESS_RIGHTS.PROCESS_QUERY_LIMITED_INFORMATION, false, (uint)p.Id); + if (safeHandle.IsInvalid) + { + return string.Empty; + }Flow.Launcher/Helper/WallpaperPathRetrieval.cs (2)
19-25: Add null check and validation for wallpaper pathThe wallpaper path retrieval could return null or empty string which should be handled explicitly before returning.
Apply this diff:
public static unsafe string GetWallpaperPath() { var wallpaperPtr = stackalloc char[MAX_PATH]; PInvoke.SystemParametersInfo(SYSTEM_PARAMETERS_INFO_ACTION.SPI_GETDESKWALLPAPER, (uint)MAX_PATH, wallpaperPtr, 0); var wallpaper = MemoryMarshal.CreateReadOnlySpanFromNullTerminated(wallpaperPtr); - - return wallpaper.ToString(); + var path = wallpaper.ToString(); + return string.IsNullOrWhiteSpace(path) ? null : path; }
36-43: Add logging for color parsing failuresThe catch block silently swallows exceptions from color parsing. Consider logging these failures to help with debugging.
Flow.Launcher/Helper/DWMDropShadow.cs (1)
48-48: Replace magic number with sizeof operatorThe hard-coded value '4' for the size parameter should be replaced with
sizeof(int)for better maintainability.Apply this diff:
-var ret1 = PInvoke.DwmSetWindowAttribute(new (helper.Handle), DWMWINDOWATTRIBUTE.DWMWA_NCRENDERING_POLICY, &val, 4); +var ret1 = PInvoke.DwmSetWindowAttribute(new (helper.Handle), DWMWINDOWATTRIBUTE.DWMWA_NCRENDERING_POLICY, &val, sizeof(int));Flow.Launcher.Infrastructure/Hotkey/GlobalHotkey.cs (1)
32-34: Consider using null-conditional operator for ModuleName accessThe ModuleName property access could throw if MainModule is null.
Apply this diff:
-return PInvoke.SetWindowsHookEx(hookId, proc, PInvoke.GetModuleHandle(curModule.ModuleName), 0); +return PInvoke.SetWindowsHookEx(hookId, proc, PInvoke.GetModuleHandle(curModule?.ModuleName), 0);Flow.Launcher/Helper/WindowsInteropHelper.cs (1)
108-125: Add documentation for the window style modificationWhile the error handling is robust, consider adding a comment explaining which specific window style bits are being modified and their impact. This would help future maintainers understand the implications of the style changes.
public static void DisableControlBox(Window win) { var hwnd = new HWND(new WindowInteropHelper(win).Handle); + // Remove the system menu (WS_SYSMENU) style bit to disable the control box + // This affects: close button, minimize button, maximize button, and system menu var style = PInvoke.GetWindowLong(hwnd, WINDOW_LONG_PTR_INDEX.GWL_STYLE);Plugins/Flow.Launcher.Plugin.Program/Programs/ShellLocalization.cs (2)
22-54: Consider the tradeoffs of unsafe code vs. performanceWhile the previous review suggested avoiding unsafe code, the current implementation makes good use of modern C# features like
Span<T>andMemoryMarshalwhich provide both safety and performance benefits. The pointer operations are well-contained within the fixed block and proper resource cleanup is implemented.However, to improve maintainability, consider adding XML documentation to explain:
- Why unsafe code is necessary here
- What safety measures are in place
- The performance benefits of this approach
24-25: Consider making buffer size configurableThe hard-coded buffer size of 1024 characters might be insufficient for some localized strings. Consider:
- Making this configurable via a constant at the class level
- Adding a comment explaining why this size was chosen
- Adding validation for very long paths
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Flow.Launcher.Infrastructure/Hotkey/GlobalHotkey.cs(3 hunks)Flow.Launcher.Plugin/SharedCommands/ShellCommand.cs(3 hunks)Flow.Launcher/Helper/DWMDropShadow.cs(2 hunks)Flow.Launcher/Helper/WallpaperPathRetrieval.cs(2 hunks)Flow.Launcher/Helper/WindowsInteropHelper.cs(4 hunks)Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessHelper.cs(2 hunks)Plugins/Flow.Launcher.Plugin.Program/Programs/ShellLinkHelper.cs(3 hunks)Plugins/Flow.Launcher.Plugin.Program/Programs/ShellLocalization.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Flow.Launcher.Plugin/SharedCommands/ShellCommand.cs
🔇 Additional comments (15)
Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessHelper.cs (1)
1-10: LGTM! Imports align with CSWin32 migration.
The new imports correctly support the transition from DllImport to CSWin32, providing access to the necessary Windows API functionality through managed wrappers.
Flow.Launcher/Helper/DWMDropShadow.cs (1)
52-54: LGTM: Proper MARGINS initialization and error handling
The MARGINS structure is correctly initialized and the HRESULT is properly checked.
Flow.Launcher.Infrastructure/Hotkey/GlobalHotkey.cs (2)
55-56: LGTM: Fixed right Win key detection
The implementation now correctly checks both left and right Windows keys, fixing the issue mentioned in the PR objectives.
91-91: LGTM: Proper resource cleanup
The hook handle is properly disposed using the SafeHandle's Dispose method.
Flow.Launcher/Helper/WindowsInteropHelper.cs (5)
9-11: LGTM! Improved type safety with HWND
The transition from IntPtr to HWND for window handles and the addition of CSWin32 namespaces enhances type safety and aligns with modern Windows API best practices.
Also applies to: 18-19
23-37: LGTM! Proper null handling with HWND
The properties correctly handle null checks using HWND.Null and utilize the PInvoke class for Windows API calls.
Line range hint 44-70: LGTM! Efficient buffer handling with stackalloc
The method now uses modern C# features for efficient buffer handling:
stackallocfor small, temporary buffersSpan<char>for safe memory management- Fixed pointer operations for native interop
149-149: LGTM! Consistent formatting
The minor formatting change maintains consistency without affecting functionality.
Line range hint 1-151: Verify complete migration to CSWin32 across codebase
While the changes in this file are well-implemented, there are other files in the codebase that still use DllImport. Consider creating follow-up tasks to migrate the remaining files for consistency:
Flow.Launcher.Core/Resource/Theme.csFlow.Launcher.Plugin.Explorer/Helper/ShellContextMenu.csFlow.Launcher.Plugin.Explorer/Helper/ShellContextMenuDisplayHelper.csFlow.Launcher.Plugin.Explorer/Search/Everything/*.cs
Plugins/Flow.Launcher.Plugin.Program/Programs/ShellLocalization.cs (2)
4-6: LGTM: Using directives align with CSWin32 migration
The new using directives correctly import the necessary Windows.Win32 namespaces for the CSWin32-based implementation.
45-46: Document buffer clearing behavior
The comment indicates uncertainty about PInvoke.LoadString behavior. Consider:
- Verifying the actual behavior through testing or documentation
- Updating the comment to reflect the findings
- If the behavior is confirmed, remove the "not sure about" phrasing
✅ Verification successful
Buffer clearing is necessary for null-terminated string handling
The code is using Windows API functions through PInvoke to handle localized shell item names. The buffer clearing is necessary because:
- The code uses
MemoryMarshal.CreateReadOnlySpanFromNullTerminated()which relies on null-termination - The same buffer is reused after
ExpandEnvironmentStringscall LoadStringis a Windows API function that writes a null-terminated string to the buffer
The comment should be updated to explain the requirement rather than expressing uncertainty:
// Clear the buffer to ensure proper null-termination for LoadString and subsequent CreateReadOnlySpanFromNullTerminated call
buffer.Clear();🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests or documentation about LoadString behavior
rg -i "loadstring.*test"
rg -i "loadstring.*behavior"
Length of output: 57
Script:
#!/bin/bash
# Let's look for the PInvoke.LoadString usage and implementation
rg -A 5 "LoadString"
# Also search for any related comments or documentation
rg -i "pinvoke.*loadstring"
Length of output: 1893
Script:
#!/bin/bash
# Let's look at the broader context of this file to understand the buffer usage
cat "Plugins/Flow.Launcher.Plugin.Program/Programs/ShellLocalization.cs"
# Also check if there are any related Windows API or PInvoke documentation references
rg -i "loadstring.*windows"
rg -i "windows.*api.*string"
Length of output: 3703
Plugins/Flow.Launcher.Plugin.Program/Programs/ShellLinkHelper.cs (4)
5-7: LGTM! Appropriate use of CSWin32 imports
The migration to Windows.Win32 namespaces aligns with the PR objective of replacing DllImport with CSWin32, providing better type safety and IntelliSense support.
49-53: Refactor repeated buffer handling pattern
The buffer handling pattern is repeated multiple times in the code. This should be extracted into a helper method.
25-31:
Add null check and implement IDisposable pattern
The COM object handling should follow proper disposal patterns and include parameter validation.
+ public class ShellLinkHelper : IDisposable
+ {
+ private bool _disposed;
+ private ShellLink? _link;
+
- public unsafe string retrieveTargetPath(string path)
+ public unsafe string retrieveTargetPath(string path)
{
+ if (path == null) throw new ArgumentNullException(nameof(path));
+ if (_disposed) throw new ObjectDisposedException(nameof(ShellLinkHelper));
+
- var link = new ShellLink();
+ _link = new ShellLink();
const int STGM_READ = 0;
- ((IPersistFile)link).Load(path, STGM_READ);
+ ((IPersistFile)_link).Load(path, STGM_READ);Likely invalid or redundant comment.
34-42: Consider buffer overflow risks with stackalloc
The use of stackalloc with MAX_PATH (260) size could cause stack overflow in certain scenarios. Consider using a pooled buffer or heap allocation for safer memory management.
Also, document why SLGP_FLAGS.SLGP_SHORTPATH is used instead of SLGP_RAWPATH.
It looks good to me. Thanks! |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Plugins/Flow.Launcher.Plugin.Program/Programs/ShellLinkHelper.cs (2)
34-44: Consider validating path length before stackallocWhile using stackalloc and MemoryMarshal is more efficient, consider validating that the path length doesn't exceed MAX_PATH before allocation to prevent stack overflow.
+ if (path.Length >= MAX_PATH) + throw new ArgumentException($"Path length exceeds maximum allowed length of {MAX_PATH}", nameof(path)); Span<char> buffer = stackalloc char[MAX_PATH];
Line range hint
25-82: Overall improvement in memory management and type safetyThe transition to CSWin32 and the use of modern C# features (stackalloc, Span, MemoryMarshal) improve both type safety and performance. However, consider implementing the suggested improvements from previous reviews to make the code more robust and maintainable:
- Add proper null checks and COM object disposal
- Extract repeated buffer handling pattern
- Add consistent error handling
- Add path length validation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Plugins/Flow.Launcher.Plugin.Program/Programs/ShellLinkHelper.cs(3 hunks)
🔇 Additional comments (5)
Plugins/Flow.Launcher.Plugin.Program/Programs/ShellLinkHelper.cs (5)
5-7: LGTM: CSWin32 imports properly replace DllImport
The transition to Windows.Win32 namespaces aligns with the PR objective of replacing DllImport with CSWin32, providing better type safety and maintainability.
25-25: Add null check and proper COM object disposal
The unsafe keyword is correctly added, but the previous review suggestions about adding null checks and proper COM object disposal are still relevant.
30-31: LGTM: Proper HWND initialization
The HWND initialization using IntPtr.Zero is correct and consistent with CSWin32 usage.
57-61: Extract repeated buffer handling pattern
The previous review suggestion about extracting the repeated buffer handling pattern into a helper method is still valid and would improve maintainability.
71-75: Add consistent error handling for GetArguments
The previous review suggestion about adding consistent error handling for GetArguments is still valid to maintain robustness.
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. Forbidden patterns 🙅 (1)In order to address this, you could change the content to not match the forbidden patterns (comments before forbidden patterns may help explain why they're forbidden), add patterns for acceptable instances, or adjust the forbidden patterns themselves. These forbidden patterns matched content: s.b. workaround(s)If the flagged items are 🤯 false positivesIf items relate to a ...
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Flow.Launcher.Plugin/SharedCommands/ShellCommand.cs (1)
53-65: Performance optimization with safe memory management.The implementation cleverly uses
stackallocfor small buffers while falling back to heap allocation for larger ones, which is more performant than usingStringBuilder. The error handling is also well-documented.However, consider adding XML documentation to explain the return value behavior:
+ /// <summary> + /// Gets the title of the specified window. + /// </summary> + /// <param name="hwnd">A handle to the window or control containing the text.</param> + /// <returns> + /// The window title string if successful, or an empty string if the window has no title bar or text, + /// the title bar is empty, or the window handle is invalid. + /// </returns> private static unsafe string GetWindowTitle(HWND hwnd)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher.Plugin/SharedCommands/ShellCommand.cs(3 hunks)
🔇 Additional comments (3)
Flow.Launcher.Plugin/SharedCommands/ShellCommand.cs (3)
6-7: LGTM! Appropriate imports for CSWin32 implementation.
The addition of Windows.Win32 imports aligns with the PR objective of replacing DllImport with CSWin32.
43-43: LGTM! Improved type safety with CSWin32.
The transition to CSWin32 types (HWND, LPARAM) and PInvoke.EnumThreadWindows provides better type safety while maintaining the original functionality.
Also applies to: 46-51
6-7: Verify consistent CSWin32 usage across the codebase.
Let's verify that similar Windows API calls in other files have been consistently updated to use CSWin32.
Also applies to: 43-43, 46-46, 53-53
|
emmm I am a little bit suspecting that this pr breaks the program plugin indexing. I will retest it. |
Thank you for reviewing. |
|
Maybe not because of this. I try the latest dev and it seems to be able to index? I am not sure. Have you experienced some issue with indexing? |
how can I test it? |
|
How about deleting the cache first and then start the latest dev and see whether it can successfully load the programs? |
I have tested and I have not encountered this problem. |
|
Thanks for testing. I guess it's something else. |
| [DllImport("user32.dll")] | ||
| [return: MarshalAs(UnmanagedType.Bool)] | ||
| private static extern bool EnumWindows(EnumWindowsProc lpEnumFunc, IntPtr lParam); |
There was a problem hiding this comment.
@Jack251970 I am getting an error from using the built-in shortcut active explorer path, could you please check if it's caused by this change.
Repro:
- Open any path in Windows Explorer
- Set the Custom Query Hotkey like this
- Use the hotkey
PR
Windows Performance Recorder.SingleInstance.cs.Test already done