Skip to content
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

Use keyPressEvent rather than shortcut for polyline delete segment #1892

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

scribblemaniac
Copy link
Member

#1861 Introduced the ability to delete segments of an active polyline with a shortcut (the backspace key by default). This prevented the keypress event from being received by ScribbleArea::keyPressEvent, which in turn broke the delete selection action. This PR reverts the creation of the new shortcut, and uses PolylineTool::keyPressEvent instead to trigger this behavior. This allows for both the polyline tool and selection to use the backspace shortcuts. I also think this is a better way to do things code-wise.

However, it does mean that the polyline delete segment action is another one of shortcuts which is currently hardcoded. I do think it was a nice idea to have this as a customizable shortcut, but I think window-level shortcuts were the wrong way to go about this, or at the very least, all shortcuts would have to be handled this way to prevent some from getting overridden the delete selection action was.

The shortcut, which by default is the Backspace key, will prevent
the keypress from being received by ScribbleArea::keyPressEvent,
which in turn breaks the delete selection action.
@scribblemaniac scribblemaniac added Bug Polyline Tool Select Tool 🔹 Minor PR (only one reviewer required) labels Nov 17, 2024
@scribblemaniac scribblemaniac added this to the 0.8.0 milestone Nov 17, 2024
@MrStevns
Copy link
Member

MrStevns commented Nov 17, 2024

It is supposedly possible to have both hardcoded events and shortcuts, using event filters and listening to the QShortcutOverride event. Was that something you looked into?

Saying that we either do hardcoded or customized is not easy nor entirely possible either. For example what about modifiers which can't be bound to shortcuts on their own?

@scribblemaniac
Copy link
Member Author

QShortcutOverride is not something I've looked into yet, and does look like it could be helpful. It is possible to customize modifiers-only shortcuts with code similar this in keyPressEvent:

QSettings settings(PENCIL2D, PENCIL2D);
int keyCode = settings.value(KEY_EYEDROPPER, Qt::Key_Alt).toInt();
if (event.key() == keyCode) { doStuff() }

Of course this won't work with multiple keys, and Qt has no way to retrieve all currently held buttons (aside from modifier keys). Similarly we could store and retrieve QKeySequences and compare them in QShortcutEvent events, but that won't work for modifier-only shortcuts. There are other things to consider too such as interactions between shortcuts, loosing window focus, etc. It's a problem that requires more thought to properly solve. For the immediate issue with the selection deletion, I think we should keep it simple.

Copy link
Member

@MrStevns MrStevns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, the whole shortcut, focus, and in general keyboard interaction requires a better foundation to get working properly.

I don't expect that to be sorted in this PR, I'll accept the PR, then we'll have to figure out how to deal with these combinations in the future.

Maybe we can use QShortcutOverride or maybe we'll have to figure out another way.

PR tested. Deletion of selections work again and removing polyline segments using escape Key also still works.

Feel free to merge 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Polyline Tool Select Tool 🔹 Minor PR (only one reviewer required)
Projects
Status: Approved
Development

Successfully merging this pull request may close these issues.

2 participants