Put Processes with Visible Window On the Top & Do not kill FL Process #3150
Conversation
This comment has been minimized.
This comment has been minimized.
|
🥷 Code experts: Jack251970 Jack251970 has most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: To learn more about /:\ gitStream - Visit our Docs |
📝 WalkthroughWalkthroughThe pull request introduces modifications to the Process Killer plugin in Flow Launcher, enhancing process management capabilities. Key changes include the declaration of a Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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: 0
🧹 Nitpick comments (3)
Plugins/Flow.Launcher.Plugin.ProcessKiller/Main.cs (2)
68-70: Consolidate dictionary usage
You're retrieving a dictionary of processes with non-empty window titles while also retrieving a broaderprocessList. Consider whether this dictionary or an alternative data structure can more seamlessly integrate with the main list. This might streamline lookups and reduce repeated iteration.
78-90: Avoid potential performance overhead in large loops
The approach of building a newResultfor each process is fine for modest lists, but in systems with many processes, enumerating them all might cause performance concerns. Periodically review if you need a paging mechanism or a more optimized approach for large process sets.Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessHelper.cs (1)
80-104: Handle potential exceptions fromProcess.GetProcessById
If the process exits between the time of enumeration and retrieval, it may throw an exception. Consider wrapping it in a try-catch block to avoid runtime issues.Potential approach:
+try +{ + var process = Process.GetProcessById((int)processId); + if (!processDict.ContainsKey((int)processId)) + { + processDict.Add((int)processId, windowTitle.ToString()); + } +} +catch (ArgumentException) +{ + // Process no longer exists, skip +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Plugins/Flow.Launcher.Plugin.ProcessKiller/Main.cs(3 hunks)Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessHelper.cs(3 hunks)
🔇 Additional comments (11)
Plugins/Flow.Launcher.Plugin.ProcessKiller/Main.cs (4)
63-64: Use a dedicated model for clarity
Defining a record for process data is a neat way to group and transport relevant info (ProcessName, MainWindowTitle). This improves maintainability and readability. Great approach!
71-71: Consider user feedback for empty query
When processList.Any() is false, you return null. If someone types an invalid or empty query, returning null means no results. Make sure this behavior aligns with desired user experience (e.g., we might want to show a “No matching processes” item).
101-104: Sorting logic fits the use case
You’re correctly using OrderBy to push processes with non-empty window titles to the top. The subsequent .ThenBy(x => x.Title) helps group them by logical title ordering. This is straightforward and readable.
109-120: Re-check concurrency or potential race conditions
Inserting a “Kill All” option is a solid UX improvement. However, calls to TryKill for multiple processes could face concurrency or ephemeral process states. If a process closes just as the user selects “Kill All,” handle or log that possibility gracefully.
Would you like me to create a verification script to check for concurrency handling across all kill calls?
Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessHelper.cs (7)
1-1: File header looks good
No issues found regarding the top-level using directives.
14-16: Verify multi-platform considerations for EnumWindows
EnumWindows is a Windows API function. Ensure that the plugin gracefully handles or skips this functionality on non-Windows platforms.
17-17: Delegate for EnumWindowsProc
This delegate looks correctly declared for the callback.
22-23: Checks for window visibility
Using IsWindowVisible is a valid approach for filtering out hidden windows.
25-26: GetWindowThreadProcessId usage
Acquiring the process ID via GetWindowThreadProcessId is standard practice and looks good here.
77-79: Docstring clarity
The XML comment accurately describes the method’s purpose and return value.
19-20:
Use a larger StringBuilder capacity
Calling new StringBuilder() defaults to 16 as capacity. This may truncate window titles longer than 16 characters.
Consider specifying a larger capacity, for example:
-StringBuilder windowTitle = new StringBuilder();
+StringBuilder windowTitle = new StringBuilder(256);Likely invalid or redundant comment.
|
make sense! Thanks. How do you think if we rank them by the z-index on the screen (I don't know whether it is possible though)? |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessHelper.cs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Report (PR)
🔇 Additional comments (1)
Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessHelper.cs (1)
79-82: LGTM on API documentationThe XML documentation for the method is clear and descriptive, accurately explaining the purpose and return value of the method.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Plugins/Flow.Launcher.Plugin.ProcessKiller/NativeMethods.txt (1)
2-7: Native Methods for Window Handling AddedThe addition of new native method names—
OpenProcess,EnumWindows,GetWindowTextLength,GetWindowText,IsWindowVisible, andGetWindowThreadProcessId—is aligned with the functionality to prioritize processes that have a non-empty main window title. This enhancement supports the new process filtering feature by ensuring that processes linked to visible windows can be identified and managed more effectively.A minor suggestion: consider adding inline comments or a short documentation block in this file to briefly describe the purpose of each native method. This will help maintain clarity and aid future maintenance or onboarding of new developers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Plugins/Flow.Launcher.Plugin.ProcessKiller/Main.cs(3 hunks)Plugins/Flow.Launcher.Plugin.ProcessKiller/NativeMethods.txt(1 hunks)Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessHelper.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessHelper.cs
- Plugins/Flow.Launcher.Plugin.ProcessKiller/Main.cs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessHelper.cs (1)
65-94:⚠️ Potential issueFix resource management and add exception handling
The method has two critical issues:
- No resource management for the
Processobject at line 83- No exception handling around
Process.GetProcessById()which may throw if the process terminatesYou should implement the following fixes:
public static unsafe Dictionary<int, string> GetProcessesWithNonEmptyWindowTitle() { var processDict = new Dictionary<int, string>(); PInvoke.EnumWindows((hWnd, _) => { var windowTitle = GetWindowTitle(hWnd); if (!string.IsNullOrWhiteSpace(windowTitle) && PInvoke.IsWindowVisible(hWnd)) { uint processId = 0; var result = PInvoke.GetWindowThreadProcessId(hWnd, &processId); if (result == 0u || processId == 0u) { return false; } - var process = Process.GetProcessById((int)processId); - if (!processDict.ContainsKey((int)processId)) - { - processDict.Add((int)processId, windowTitle); - } + try + { + using var process = Process.GetProcessById((int)processId); + if (!processDict.ContainsKey((int)processId)) + { + processDict.Add((int)processId, windowTitle); + } + } + catch (ArgumentException) + { + // Process might have exited between enumeration and retrieval + } + catch (InvalidOperationException) + { + // Process might be in an invalid state + } } return true; }, IntPtr.Zero); return processDict; }Note that this issue was already identified in a previous review but hasn't been addressed in this version.
🧹 Nitpick comments (1)
Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessHelper.cs (1)
65-94: Consider handling processes with multiple windowsCurrently, the dictionary only stores one window title per process ID. If a process has multiple windows, only the last one encountered will be stored. Consider whether your sorting logic would benefit from having access to all window titles for a process.
If you need to track multiple windows per process, you could modify the return type:
- public static unsafe Dictionary<int, string> GetProcessesWithNonEmptyWindowTitle() + public static unsafe Dictionary<int, List<string>> GetProcessesWithNonEmptyWindowTitle() { - var processDict = new Dictionary<int, string>(); + var processDict = new Dictionary<int, List<string>>(); PInvoke.EnumWindows((hWnd, _) => { var windowTitle = GetWindowTitle(hWnd); if (!string.IsNullOrWhiteSpace(windowTitle) && PInvoke.IsWindowVisible(hWnd)) { // ... existing code to get processId ... try { using var process = Process.GetProcessById((int)processId); if (!processDict.ContainsKey((int)processId)) { - processDict.Add((int)processId, windowTitle); + processDict.Add((int)processId, new List<string> { windowTitle }); } + else + { + processDict[(int)processId].Add(windowTitle); + } } // ... exception handling ... } return true; }, IntPtr.Zero); return processDict; }This change would allow you to track all window titles for each process, which might provide more context for your sorting algorithm.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher.Infrastructure/FileExplorerHelper.cs(0 hunks)Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessHelper.cs(2 hunks)
💤 Files with no reviewable changes (1)
- Flow.Launcher.Infrastructure/FileExplorerHelper.cs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: gitStream.cm
🔇 Additional comments (3)
Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessHelper.cs (3)
16-16: Good syntax update using modern C# featuresUsing the target-typed new expression (
new()) is cleaner and more maintainable.
96-109: Well-implemented window title retrieval methodThis is an excellent implementation of
GetWindowTitle:
- Efficiently determines required buffer size
- Uses
stackallocfor small buffers to avoid heap allocations- Falls back to heap allocation for larger buffers
- Uses modern C# features like
Span<T>and fixed buffers- Handles empty return values correctly
1-11: 🛠️ Refactor suggestionAdd missing using directives for Runtime.InteropServices and Text
The code is using interop features and string manipulation, but missing the required using statements.
using Flow.Launcher.Infrastructure; using Flow.Launcher.Infrastructure.Logger; using Microsoft.Win32.SafeHandles; using System; using System.Collections.Generic; using System.Diagnostics; using System.Linq; +using System.Runtime.InteropServices; +using System.Text; using Windows.Win32; using Windows.Win32.Foundation; using Windows.Win32.System.Threading;Note that this issue was already identified in a previous review but hasn't been addressed in this version.
This comment has been minimized.
This comment has been minimized.
|
@PaulPSta Hi, I have cleaned up your codes and done some improvements here. Firstly, I replaced DllImport with PInvoke for code quality. Secondly, every time you kill one process, I think the main window should remain open and re-query results instead of closing. This allows users to terminate multiple processes without interruption. Could you please check if this works for you? |
Jack251970
changed the title
Jack251970
changed the title
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessHelper.cs (1)
83-109:⚠️ Potential issueHandle potential exceptions and improve resource management
The implementation of
GetProcessesWithNonEmptyWindowTitleneeds to handle potential exceptions and properly dispose of Process objects.public static unsafe Dictionary<int, string> GetProcessesWithNonEmptyWindowTitle() { var processDict = new Dictionary<int, string>(); PInvoke.EnumWindows((hWnd, _) => { var windowTitle = GetWindowTitle(hWnd); if (!string.IsNullOrWhiteSpace(windowTitle) && PInvoke.IsWindowVisible(hWnd)) { uint processId = 0; var result = PInvoke.GetWindowThreadProcessId(hWnd, &processId); - if (result == 0u || processId == 0u) + if (result == 0u || processId == 0u) { - return false; + return true; // Continue enumeration even if we couldn't get this window's process ID } - var process = Process.GetProcessById((int)processId); - if (!processDict.ContainsKey((int)processId)) - { - processDict.Add((int)processId, windowTitle); - } + try + { + using var process = Process.GetProcessById((int)processId); + if (!processDict.ContainsKey((int)processId)) + { + processDict.Add((int)processId, windowTitle); + } + } + catch (ArgumentException) + { + // Process might have exited between enumeration and retrieval + } + catch (InvalidOperationException) + { + // Process might be in an invalid state + } } return true; }, IntPtr.Zero); return processDict; }This change:
- Makes the method continue enumeration even if GetWindowThreadProcessId fails
- Uses a try-catch block to handle potential exceptions from Process.GetProcessById
- Uses a using statement for proper disposal of the Process object
🧹 Nitpick comments (1)
Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessHelper.cs (1)
42-78: Consider deduplicating process entries in search results.The current implementation might add the same process twice to the results list - once when matching by window title and again when matching by process name and ID.
public List<ProcessResult> GetMatchingProcesses(string searchTerm, Dictionary<int, string> processWindowTitle) { var processlist = new List<ProcessResult>(); + var addedProcessIds = new HashSet<int>(); foreach (var p in Process.GetProcesses()) { if (IsSystemProcess(p)) continue; if (string.IsNullOrWhiteSpace(searchTerm)) { // Show all non-system processes processlist.Add(new ProcessResult(p, 0)); + addedProcessIds.Add(p.Id); } else { // Search window title first if (processWindowTitle.TryGetValue(p.Id, out var windowTitle)) { var score = StringMatcher.FuzzySearch(searchTerm, windowTitle).Score; if (score > 0) { processlist.Add(new ProcessResult(p, score)); + addedProcessIds.Add(p.Id); + continue; } } // Search process name and process id var score1 = StringMatcher.FuzzySearch(searchTerm, p.ProcessName + " - " + p.Id).Score; - if (score1 > 0) + if (score1 > 0 && !addedProcessIds.Contains(p.Id)) { processlist.Add(new ProcessResult(p, score1)); + addedProcessIds.Add(p.Id); continue; } } } return processlist; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Plugins/Flow.Launcher.Plugin.ProcessKiller/Main.cs(2 hunks)Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessHelper.cs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Plugins/Flow.Launcher.Plugin.ProcessKiller/Main.cs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: gitStream.cm
🔇 Additional comments (3)
Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessHelper.cs (3)
34-37: Good preventive measure for Flow Launcher process.Adding the constant for the Flow Launcher process name and modifying
IsSystemProcessto prevent killing the Flow Launcher itself is a good safety measure. This aligns with the PR objective to prevent the program from self-terminating.
111-124: Good implementation of the GetWindowTitle methodThe implementation of
GetWindowTitleefficiently uses stack allocation for small buffers and heap allocation for larger ones. The use of Span and unsafe code is appropriate for this low-level Windows API interaction.
1-11: Missing using statements for Span and unsafe operationsThe code uses Span and unsafe code, but the necessary using statements are missing.
using Flow.Launcher.Infrastructure; using Flow.Launcher.Infrastructure.Logger; using Microsoft.Win32.SafeHandles; using System; using System.Collections.Generic; using System.Diagnostics; using System.Linq; +using System.Runtime.InteropServices; +using System.Text; using Windows.Win32; using Windows.Win32.Foundation; using Windows.Win32.System.Threading;
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessHelper.cs (1)
62-91:⚠️ Potential issueHandle potential exceptions when retrieving processes by ID.
GetProcessByIdcan throwArgumentExceptionorInvalidOperationExceptionif the process has exited between enumeration and retrieval. Currently, the callback returnsfalseif an error arises early, but an unhandled exception would breakEnumWindows. Consider a try/catch block aroundProcess.GetProcessByIdto avoid partial enumeration failures.- var process = Process.GetProcessById((int)processId); - if (!processDict.ContainsKey((int)processId)) - { - processDict.Add((int)processId, windowTitle); - } + try + { + var process = Process.GetProcessById((int)processId); + if (!processDict.ContainsKey((int)processId)) + { + processDict.Add((int)processId, windowTitle); + } + } + catch (Exception) + { + // If process has exited, ignore and continue enumerating + }
🧹 Nitpick comments (4)
Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessResult.cs (1)
21-25: Good read-only properties for external access.Providing
Title,TitleMatch, andTooltipas read-only properties makes the model more robust and predictable. Consider adding short doc comments summarizing each property’s purpose for maintainability.Plugins/Flow.Launcher.Plugin.ProcessKiller/Main.cs (1)
144-145: Repeated re-query could affect responsiveness for multiple process kills.Calling
_context.API.ReQuery()once per kill updates results dynamically, but for many kills in quick succession, this might degrade responsiveness. Consider deferring re-query until all kills are complete or providing a batch-kill mode.Also applies to: 169-171
Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessHelper.cs (2)
14-35: Treating Flow Launcher as a system process.Excluding Flow Launcher from termination is consistent with the PR objective. If there's ever a need for advanced users to kill Flow Launcher, you could make this configurable rather than hardcoded in
_systemProcessList.
116-129: Exception logging is comprehensive.The
TryKillmethod logs any failure to kill the process with context. This ensures traceability for users without halting the workflow. If partial kills are common, consider whether to continue or abort subsequent kills in the calling code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Plugins/Flow.Launcher.Plugin.ProcessKiller/Flow.Launcher.Plugin.ProcessKiller.csproj(0 hunks)Plugins/Flow.Launcher.Plugin.ProcessKiller/Main.cs(4 hunks)Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessHelper.cs(5 hunks)Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessResult.cs(1 hunks)
💤 Files with no reviewable changes (1)
- Plugins/Flow.Launcher.Plugin.ProcessKiller/Flow.Launcher.Plugin.ProcessKiller.csproj
🔇 Additional comments (7)
Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessResult.cs (2)
2-2: Use of new SharedModels namespace.The
using Flow.Launcher.Plugin.SharedModels;statement correctly provides access toMatchResult. There are no immediate concerns here, given thatMatchResultis appropriately referenced in the constructor and property.
8-14: Constructor signature aligns well with new data needs.The updated constructor appropriately captures the new parameters (
title,match, andtooltip) and stores them in read-only properties, ensuring immutability. This is a clean approach to aggregating process data (score, match information, etc.) in one place.Plugins/Flow.Launcher.Plugin.ProcessKiller/Main.cs (2)
9-9: Read-only initialization of ProcessHelper.Declaring
processHelperasreadonlywith the concise object instantiation improves immutability and readability. This is a good modern C# practice.
72-125:❓ Verification inconclusive
Consider potential performance impact with large process lists.
Applying fuzzy scoring to every process and enumerating all windows can be expensive if hundreds of processes are running or if repeated frequently. If performance becomes a concern, consider caching or partial searching.
🏁 Script executed:
#!/bin/bash # Description: Basic check for how many processes might typically exist on this system when enumerating # This can inform whether fuzzy search might be a bottleneck. # Attempt to see how many lines of active processes exist ps aux | wc -lLength of output: 111
Action Required: Evaluate potential performance overhead with high process counts.
The current implementation applies fuzzy search on all processes and intensity could increase with a high number of processes. Although typical systems might not trigger a problem, if hundreds of processes exist or the operation is repeated frequently, consider profiling and caching fuzzy search results or limiting the search scope to prevent performance degradation.
Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessHelper.cs (3)
38-43: Useful utility for combining process name and ID.
GetProcessNameIdTitleis a convenient helper. Watch for edge cases if a process unexpectedly terminates before printing its name/ID. Typically, it’s fine as is, but you could add defensive checks if you suspect concurrency issues.
48-60: Unfiltered list of user processes.
GetMatchingProcessesreturns all non-system processes without window constraints, aligning with the plugin’s design. If future requirements arise (e.g., ignoring background tasks lacking windows), filter them here to keep logic cohesive.
93-105: Efficient approach to retrieve window titles.Using a capacity-based buffer with
Span<char>is a good approach. Just ensure the overhead remains minimal for very large windows’ text. This is typically fine as the maximum title length is not too large in practice.
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. If the flagged items are 🤯 false positivesIf items relate to a ...
|
Put Processes with Visible Window On the Top
When using the killer process feature, I often find it difficult to recall the exact name of the process I want to terminate. However, in most cases, I am trying to close a process associated with a visible window on my screen, rather than a background service or job.
To improve usability and speed, I propose sorting the search results to prioritize processes that have a non-empty MainWindowTitle, so that processes with visible windows appear first.
The process name & id is still visible and I move it to the tooltip of the title.
Additionally, since we are using FL, we should not allow to kill FL process, so I remove FL process from the list.
Test