Improve process killer performance & Add option to let process killer show window title #3468
Conversation
|
Tested and good to go. |
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. |
📝 WalkthroughWalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsViewModel
participant Settings
participant Main
participant ProcessHelper
User->>SettingsViewModel: Toggle ShowWindowTitle checkbox
SettingsViewModel->>Settings: Update ShowWindowTitle value
User->>Main: Query processes
Main->>Settings: Check ShowWindowTitle
alt ShowWindowTitle == true
Main->>ProcessHelper: GetProcessesWithNonEmptyWindowTitle()
ProcessHelper->>ProcessHelper: Collect visible windows
ProcessHelper->>ProcessHelper: Parallel process window handles
ProcessHelper-->>Main: Dictionary<int, string> with window titles
else ShowWindowTitle == false
Main->>Main: Use empty dictionary for window titles
end
Main->>User: Display filtered and scored process results
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🪧 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 (1)
Flow.Launcher.Plugin/Result.cs (1)
131-136: Consider adding annotations to prevent future regressions.Since these fields are critical for backwards compatibility, consider adding annotations or comments to explicitly warn future developers not to convert these back to properties.
/// <summary> /// Delegate to load an icon for this result. /// </summary> + /// <remarks> + /// IMPORTANT: This must remain a field (not a property) for plugin compatibility. + /// Some plugins access this through reflection expecting a field. + /// </remarks> public IconDelegate Icon = null; /// <summary> /// Delegate to load an icon for the badge of this result. /// </summary> + /// <remarks> + /// IMPORTANT: This must remain a field (not a property) for plugin compatibility. + /// Some plugins access this through reflection expecting a field. + /// </remarks> public IconDelegate BadgeIcon = null;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher.Plugin/Result.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (2)
Flow.Launcher.Plugin/Result.cs (2)
131-131: Reverting property to field correctly addresses the MissingFieldException.This change from property
public IconDelegate Icon { get; set; }to fieldpublic IconDelegate Icon = null;resolves the mentioned compatibility issue with the ClipboardPlus plugin. The plugin was looking for a field through reflection but couldn't find it after it had been changed to a property.This change breaks encapsulation but is necessary for backward compatibility. Consider documenting this design decision for future maintainers so this field isn't accidentally changed back to a property.
136-136: Converting BadgeIcon property to field for consistency.Similarly to the Icon property, this change converts BadgeIcon from an auto-implemented property to a field with an explicit null initialization. This maintains consistency with the Icon field and helps prevent future compatibility issues.
This comment has been minimized.
This comment has been minimized.
|
🥷 Code experts: no user but you matched threshold 10 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:
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 |
There was a problem hiding this comment.
Pull Request Overview
This PR reverts delegate properties to fields in Result.cs to address a compatibility issue while also enhancing process killer performance and providing a new option to display window titles.
- Revert delegate properties to fields in Result.cs
- Introduce a new ShowWindowTitle setting in Settings and SettingsViewModel
- Improve process enumeration performance with concurrent processing in ProcessHelper and update ProcessResult creation in Main.cs
Reviewed Changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Plugins/Flow.Launcher.Plugin.ProcessKiller/ViewModels/SettingsViewModel.cs | Added a new ShowWindowTitle property exposing the setting. |
| Plugins/Flow.Launcher.Plugin.ProcessKiller/Settings.cs | Added the ShowWindowTitle property with a default value. |
| Plugins/Flow.Launcher.Plugin.ProcessKiller/ProcessHelper.cs | Refactored enumeration to use concurrent processing for window handles. |
| Plugins/Flow.Launcher.Plugin.ProcessKiller/Main.cs | Updated ProcessResult creation to conditionally display window titles. |
| Flow.Launcher.Plugin/Result.cs | Reverted Icon and BadgeIcon from properties to fields to fix compatibility. |
Files not reviewed (2)
- Plugins/Flow.Launcher.Plugin.ProcessKiller/Languages/en.xaml: Language not supported
- Plugins/Flow.Launcher.Plugin.ProcessKiller/Views/SettingsControl.xaml: Language not supported
Comments suppressed due to low confidence (1)
Flow.Launcher.Plugin/Result.cs:131
- [nitpick] Reverting from a property to a field fixes the compatibility issue, but please ensure that any logic depending on property setter side effects is no longer required.
public IconDelegate Icon = null;
| catch | ||
| { | ||
| // Handle exceptions (e.g., process exited) |
There was a problem hiding this comment.
Consider logging or otherwise capturing the caught exception to facilitate debugging, rather than silently ignoring it.
Copilot uses AI. Check for mistakes.
@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 ...
|
jjw24
removed
bug
jjw24
changed the title
Revert delegate to fields to fix compatibility issue
Improve process killer performance & Add option to let process killer show window title
Test