-
-
Notifications
You must be signed in to change notification settings - Fork 283
Customizable Post Grab Actions #606
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
Introduce a configurable system for post-capture actions in Fullscreen Grab, including a new settings dialog, dynamic menu generation, and persistent user preferences. Refactor related models and UI for extensibility and maintainability. Add unit tests for action management logic.
Introduces a user setting and UI toggle to control whether the post-grab menu stays open after selecting an action. Also adds InputGestureText to ButtonInfo.
Removed InputGestureText from ButtonInfo and related tests. Input gestures for post-grab actions are now set dynamically in FullscreenGrab.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements customizable post-grab actions for the Fullscreen Grab feature, allowing users to configure which text transformation actions are available after capturing text. The hardcoded menu items in the fullscreen grab context menu have been replaced with a dynamic system managed by PostGrabActionManager.
Changes:
- Introduced
PostGrabActionManagerutility class to manage post-grab actions configuration - Added
PostGrabActionEditorUI for users to customize available actions and their order - Refactored
FullscreenGrabto dynamically load and execute actions based on user preferences - Added three new settings properties for persisting action configurations and check states
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| Text-Grab/Utilities/PostGrabActionManager.cs | New utility class managing post-grab actions (loading, saving, executing) |
| Text-Grab/Views/FullscreenGrab.xaml.cs | Refactored to dynamically load actions via LoadDynamicPostGrabActions() and execute them |
| Text-Grab/Views/FullscreenGrab.xaml | Commented out hardcoded menu items in favor of dynamic generation |
| Text-Grab/Controls/PostGrabActionEditor.xaml.cs | New editor dialog for managing enabled actions list |
| Text-Grab/Controls/PostGrabActionEditor.xaml | UI for the post-grab action editor with add/remove/reorder functionality |
| Text-Grab/Pages/FullscreenGrabSettings.xaml.cs | Added button to open editor and display current actions count |
| Text-Grab/Pages/FullscreenGrabSettings.xaml | Added UI section for customizing post-capture actions (removed ScrollViewer) |
| Text-Grab/Models/ButtonInfo.cs | Added enum DefaultCheckState and new properties for post-grab action support |
| Text-Grab/Properties/Settings.settings | Added three new settings for post-grab configuration |
| Text-Grab/Properties/Settings.Designer.cs | Generated code for new settings properties |
| Text-Grab/App.config | Added configuration entries for new settings |
| Text-Grab/Pages/GeneralSettings.xaml.cs | Added Clear() before populating WebSearchersComboBox to prevent duplicates |
| Tests/PostGrabActionManagerTests.cs | Comprehensive unit tests for PostGrabActionManager functionality |
Files not reviewed (1)
- Text-Grab/Properties/Settings.Designer.cs: Language not supported
| public override int GetHashCode() | ||
| { | ||
| return System.HashCode.Combine(ButtonText, SymbolText, Background, Command, ClickEvent); | ||
| return System.HashCode.Combine(ButtonText, SymbolText, Background, Command, ClickEvent, IsRelevantForFullscreenGrab, IsRelevantForEditWindow); | ||
| } |
Copilot
AI
Jan 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The GetHashCode implementation doesn't include DefaultCheckState, but it was added to the HashCode.Combine call. However, other important properties like OrderNumber, SymbolIcon, and IsSymbol are still missing from the hash code calculation. This creates inconsistent equality semantics - two ButtonInfo objects with the same text but different OrderNumber or SymbolIcon values will have the same hash code. Either include all semantically significant properties in the hash code, or reconsider which properties should determine equality.
| string[] stringSplit = text.Split(Environment.NewLine); | ||
| string finalString = ""; | ||
| foreach (string line in stringSplit) | ||
| if (!string.IsNullOrWhiteSpace(line)) | ||
| finalString += line.Trim() + Environment.NewLine; | ||
| result = finalString; |
Copilot
AI
Jan 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String concatenation in a loop is inefficient. Each concatenation creates a new string object, resulting in O(n²) complexity for n lines. Consider using StringBuilder for better performance, especially when processing text with many lines.
| menuItem.Click += PostActionMenuItem_Click; | ||
|
|
||
| // Add keyboard handling | ||
| contextMenu.PreviewKeyDown += FullscreenGrab_KeyDown; |
Copilot
AI
Jan 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Event handler for PreviewKeyDown is being added to the ContextMenu inside the loop for each MenuItem. This means the same event handler (FullscreenGrab_KeyDown) will be attached multiple times - once for each action. This should be moved outside the loop to attach only once, or the handler should be attached to individual menu items instead of the context menu.
| private void LoadDynamicPostGrabActions() | ||
| { | ||
| if (NextStepDropDownButton.Flyout is not ContextMenu contextMenu) | ||
| return; | ||
|
|
||
| // Clear existing items | ||
| contextMenu.Items.Clear(); | ||
|
|
||
| // Get enabled post-grab actions from settings | ||
| List<ButtonInfo> enabledActions = PostGrabActionManager.GetEnabledPostGrabActions(); | ||
|
|
||
| // Get the PostGrabStayOpen setting | ||
| bool stayOpen = DefaultSettings.PostGrabStayOpen; | ||
|
|
||
| int index = 1; | ||
| foreach (ButtonInfo action in enabledActions) | ||
| { | ||
| MenuItem menuItem = new() | ||
| { | ||
| Header = action.ButtonText, | ||
| IsCheckable = true, | ||
| Tag = action, | ||
| IsChecked = PostGrabActionManager.GetCheckState(action), | ||
| StaysOpenOnClick = stayOpen, | ||
| InputGestureText = $"Ctrl+{index}" | ||
| }; | ||
|
|
||
| // Wire up click handler | ||
| menuItem.Click += PostActionMenuItem_Click; | ||
|
|
||
| // Add keyboard handling | ||
| contextMenu.PreviewKeyDown += FullscreenGrab_KeyDown; | ||
|
|
||
| contextMenu.Items.Add(menuItem); | ||
| index++; | ||
| } | ||
|
|
||
| contextMenu.Items.Add(new Separator()); | ||
|
|
||
| // Add "Edit this list..." menu item | ||
| MenuItem editPostGrabMenuItem = new() | ||
| { | ||
| Header = "Edit this list..." | ||
| }; | ||
| editPostGrabMenuItem.Click += EditPostGrabActions_Click; | ||
| contextMenu.Items.Add(editPostGrabMenuItem); | ||
|
|
||
| // Add "Close this menu" menu item | ||
| MenuItem hidePostGrabMenuItem = new() | ||
| { | ||
| Header = "Close this menu" | ||
| }; | ||
| hidePostGrabMenuItem.Click += HidePostGrabActions_Click; | ||
| contextMenu.Items.Add(hidePostGrabMenuItem); | ||
|
|
||
| // Update the dropdown button appearance | ||
| CheckIfAnyPostActionsSelected(); | ||
| } |
Copilot
AI
Jan 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Event handlers attached to dynamically created MenuItems (menuItem.Click and contextMenu.PreviewKeyDown) are not being cleaned up in Window_Unloaded. This creates a potential memory leak, especially if the window is opened and closed multiple times. Consider either: 1) Storing references to dynamically created items and removing handlers in Window_Unloaded, or 2) Using weak event patterns, or 3) Clearing and recreating the menu each time it opens rather than in Window_Loaded.
| foreach (ButtonInfo button in ButtonInfo.AllButtons) | ||
| { | ||
| if (button.IsRelevantForFullscreenGrab && !allPostGrabActions.Any(b => b.ButtonText == button.ButtonText)) | ||
| { | ||
| allPostGrabActions.Add(button); | ||
| } | ||
| } |
Copilot
AI
Jan 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| string finalString = ""; | ||
| foreach (string line in stringSplit) | ||
| if (!string.IsNullOrWhiteSpace(line)) | ||
| finalString += line.Trim() + Environment.NewLine; | ||
| result = finalString; |
Copilot
AI
Jan 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| string finalString = ""; | |
| foreach (string line in stringSplit) | |
| if (!string.IsNullOrWhiteSpace(line)) | |
| finalString += line.Trim() + Environment.NewLine; | |
| result = finalString; | |
| var trimmedLines = stringSplit | |
| .Where(line => !string.IsNullOrWhiteSpace(line)) | |
| .Select(line => line.Trim()) | |
| .ToArray(); | |
| result = trimmedLines.Length == 0 | |
| ? string.Empty | |
| : string.Join(Environment.NewLine, trimmedLines) + Environment.NewLine; |
| if (checkStates is not null && checkStates.TryGetValue(action.ButtonText, out bool storedState)) | ||
| { | ||
| // If the action is set to LastUsed, use the stored state | ||
| if (action.DefaultCheckState == DefaultCheckState.LastUsed) | ||
| return storedState; |
Copilot
AI
Jan 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 'if' statements can be combined.
| if (checkStates is not null && checkStates.TryGetValue(action.ButtonText, out bool storedState)) | |
| { | |
| // If the action is set to LastUsed, use the stored state | |
| if (action.DefaultCheckState == DefaultCheckState.LastUsed) | |
| return storedState; | |
| if (checkStates is not null | |
| && checkStates.TryGetValue(action.ButtonText, out bool storedState) | |
| && action.DefaultCheckState == DefaultCheckState.LastUsed) | |
| { | |
| // If the action is set to LastUsed, use the stored state | |
| return storedState; |
| if (sender is MenuItem menuItem && menuItem.Tag is ButtonInfo action) | ||
| { | ||
| if (action.DefaultCheckState == DefaultCheckState.LastUsed) | ||
| { | ||
| PostGrabActionManager.SaveCheckState(action, menuItem.IsChecked); | ||
| } |
Copilot
AI
Jan 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 'if' statements can be combined.
| if (sender is MenuItem menuItem && menuItem.Tag is ButtonInfo action) | |
| { | |
| if (action.DefaultCheckState == DefaultCheckState.LastUsed) | |
| { | |
| PostGrabActionManager.SaveCheckState(action, menuItem.IsChecked); | |
| } | |
| if (sender is MenuItem menuItem | |
| && menuItem.Tag is ButtonInfo action | |
| && action.DefaultCheckState == DefaultCheckState.LastUsed) | |
| { | |
| PostGrabActionManager.SaveCheckState(action, menuItem.IsChecked); |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@TheJoeFin I've opened a new pull request, #607, to work on those changes. Once the pull request is ready, I'll request review from you. |
…on, fix event handler leaks Co-authored-by: TheJoeFin <[email protected]>
Co-authored-by: TheJoeFin <[email protected]>
Co-authored-by: TheJoeFin <[email protected]>
Co-authored-by: TheJoeFin <[email protected]>
Refactor post-grab actions: fix memory leaks, improve performance, enhance maintainability
Reduced editor window height and updated ListView scrollbars. Commented out the "Translate to system language" post-grab action.
Adjusted test expectations after removing the action from defaults.
Fixes #604