Skip to content

feat: Delete key removes items in source and profile lists#2469

Open
Fikri-20 wants to merge 2 commits intoborgbase:masterfrom
Fikri-20:feat/delete-key-for-lists
Open

feat: Delete key removes items in source and profile lists#2469
Fikri-20 wants to merge 2 commits intoborgbase:masterfrom
Fikri-20:feat/delete-key-for-lists

Conversation

@Fikri-20
Copy link
Copy Markdown

Closes #2447

Added a Delete key shortcut to both the source files table and the profile selector list, wired to the existing remove/delete actions.

@Fikri-20
Copy link
Copy Markdown
Author

hey @m3nu , could you please review the PR and tell me if I need to tweak anything else?

@Fikri-20 Fikri-20 force-pushed the feat/delete-key-for-lists branch from d61e29c to cca77e6 Compare March 25, 2026 22:08
@Fikri-20
Copy link
Copy Markdown
Author

Hi @m3nu, I'm applying for GSoC 2026 and have these PR ready for review. Could you please take a look?

@m3nu
Copy link
Copy Markdown
Contributor

m3nu commented Mar 25, 2026

Review of PR #2469

Clean, focused PR. Reuses existing profile_delete_action and source_remove methods, so all validation (confirmation dialog, last-profile guard) is preserved. Tests cover both shortcuts. No new imports needed in production code.

Issues

  1. macOS keyboard mismatch: Qt.Key.Key_Delete maps to the forward-delete key, which doesn't exist on most Mac keyboards (only full-size ones). On macOS, users expect Backspace (Qt's Key_Backspace) for deletion. The shortcut will be effectively unusable on most Macs. Consider adding Key_Backspace as an additional shortcut, or using QKeySequence.StandardKey.Delete which Qt maps platform-appropriately.

  2. Inconsistent style between files:

    • main_window.py creates the shortcut inline: QShortcut(...).activated.connect(...)
    • source_tab.py uses a two-line pattern with a local variable, matching the existing shortcut_copy pattern in that file.

    The one-liner works (Qt parent-child ownership keeps it alive), but the two-line pattern is clearer and consistent with the rest of source_tab.

  3. Exclusion tab not covered: The exclusion list also has a remove action. Not blocking, but worth considering for consistency.

@Fikri-20 Fikri-20 force-pushed the feat/delete-key-for-lists branch from cca77e6 to 1044975 Compare March 25, 2026 23:26
Fikri-20 and others added 2 commits March 26, 2026 01:34
On macOS, Key_Delete maps to forward-delete which doesn't exist on most
laptops. StandardKey.Delete is platform-appropriate (Backspace on macOS,
Delete on others).
@Fikri-20 Fikri-20 force-pushed the feat/delete-key-for-lists branch from 1044975 to fbd0225 Compare March 25, 2026 23:34
@Fikri-20
Copy link
Copy Markdown
Author

Hi @m3nu, thanks for the review! I've updated the PR to use QKeySequence.StandardKey.Delete for cross-platform compatibility as suggested. This should now work correctly on macOS laptops as well.

Let me know if any other tweaks are needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support using the Delete key to remove items in Profile and Source lists

2 participants