Details
Diff Detail
- Repository
- R166 Spectacle
- Branch
- ctrl-t-shortcut (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage
Works great! I might suggest a bit of code re-organization:
- Put the two QShortcut definitions and their connect lines adjacent to one another, rather than having clickFunc() between them
- Now that we have two shortcuts, rename the shortcut variable to something more descriptive
- Revise Ctrl+T addition with better code organization
Per @ngraham: Group shortcut definitions together; rename variable 'shortcut' for more clarity and consistency
I rearranged the lines of code to separate the clickFunc keypress function from the two QShortcut definitions. Original variable shortcut is now enterKey for clarity and to match new shortcut variable ctrlT. I re-tested and both keys work as intended.
Exactly how I would have done it! Works fine in my testing.
And how nice, you used arc so I don't have to bug you for an email address (assuming I didn't already know it!) before landing the patch. Any objections, @rkflx?
@sharvey I'm afraid we have to change the scope of your patch a bit (I'm very sorry!).
When I suggested the feature, I was under the assumption that Take a New Screenshot could only be triggered by pressing the default dialog button, i.e. Enter. Now that I read the parts of the code you digged out (thanks for that!), it becomes clear to me that there is already another shortcut for the left hand to trigger the button: Ctrl+N. What was mistakenly relabeled as enterKey is in fact the shortcut for that key combination, grep the docs for QKeySequence::New.
The root cause users (and apparently developers too) stumble upon is that there's no UI in Spectacle to review or change the shortcuts, see https://bugs.kde.org/show_bug.cgi?id=388592. I understand implementing that might be a bit too much to ask for, so I have an alternative suggestion:
There is a cleanup opportunity right in the clickFunc lambda. Look at the API docs what animateClick does, then I think you'll wonder just like I do at the moment what the purpose of QTimer::singleShot actually is.
After that's dealt with, it might even make sense to get rid of clickFunc and write it directly inside the connect. I can help you with that if you don't know what I mean.
Does this sound like a plan?
I see now the purpose of ‘QKeySequence::New’ I intentionally left it unchanged under the law of “if it’s not broken, don’t fix it.” I should have stumbled across the usage while looking for the syntax to add another key. It makes perfect sense now.
I’ll look at the code again next week. I’m curious to see how the functions could be optimized. It’s just a little hard to do on a smartphone.
Yeah, that's why we still need "normal" computers for a while… As a tip, use an IDE like Qt Creator:
- If over a function, you can just press F1 to jump to the API docs.
- Ctrl-clicking on a function takes you to the definition.
- Ctrl+⇧+U list all usage of a variable or function (semantically correct, i.e. it's not just a simple text-based search).
- Ctrl+Space auto-completes from all available variables and functions in that context.
Indeed, the QTimer::singleShotcall appears to be redundant. The use of animateClick should be sufficient. It's not just an animation effect; it's a click event as well. Per the Qt documentation:
All signals associated with a click are emitted as appropriate.
Testing with the QTimer function commented out shows Spectacle still successfully takes a screenshot when pressing Enter, CTRL + N, or in this branch, CTRL + T.
I will work on reducing clickFunc() to be part of the connect function. I understand the concept and the goal; it's the C++ Qt implementation that I need to study.
At the risk of being redundant myself, are we scrapping CTRL + T or keeping it? I'm learning to ask direct questions and not make assumptions.
Adjusting the connect call(s) turned out to be quite straightforward. It was just a matter of triggering the KSWidget::newScreenshotClicked() slot exactly the same way as the connection is defined for clicking mTakeScreenshotButton directly (line 127).
connect(ctrlT, &QShortcut::activated, this, &KSWidget::newScreenshotClicked);
I understand how the two signals (QShortcut::activated and KSWidget:newScreenShotClicked) are processed in sequence, with this referring to the root-level widget (the dialog itself).
It's always nice to simplify code while keeping it readable and understandable.
Let me know if we want to keep CTRL+T and I'll update this submission - or create a new one, whichever is preferred.
Sorry for the wait, busy with the 18.04 Beta and general design work these days.
Yup.
Before, signals and function calls flowed like this:
QShortcut::activated → clickFunc → animateClick → QPushButton::clicked → newScreenshotClicked
Now you are proposing this:
QShortcut::activated → newScreenshotClicked
Personally, I liked the animation. Don't you think we should keep it? ;) I only wanted to get rid of clickFunc, by using a lambda instead of a function in the connect. Not sure you know what I mean by that, but I found an example where this is explained – grep for "Lambdas in Qt connections". You don't have to understand or even read the complete article, but I hope you can adapt the example there to Spectacle.
At the risk of being redundant myself, are we scrapping CTRL + T or keeping it?
I'd say the existing shortcut in the code is enough for now, with the long-term goal of adding a Configure Shortcuts dialog to Spectacle, where users could change and add shortcuts as they like.
I'll update this submission - or create a new one, whichever is preferred.
It's probably best to create a new revision and abandon this one, because the topic changed quite a bit (sorry again).
Before, signals and function calls flowed like this:
QShortcut::activated → clickFunc → animateClick → QPushButton::clicked → newScreenshotClickedNow you are proposing this:
QShortcut::activated → newScreenshotClickedPersonally, I liked the animation. Don't you think we should keep it? ;)
I modeled my connect function change on this example from the Qt documentation:
MyWidget::MyWidget() { myButton = new QPushButton(this); connect(myButton, SIGNAL(clicked()), this, SIGNAL(buttonClicked())); }
It seemed straightforward and effective. But now I understand what you were looking for.
I will admit that lambdas are one of the more mystical programming techniques. They haven't come into play in the other languages I've coded in, so while I've seen them, the concept hasn't yet soaked into my brain. I'll study that article along with my C++ books in the goal of acquiring the mystical understanding of the lambda.
It's probably best to create a new revision and abandon this one, because the topic changed quite a bit (sorry again).
I feel like I'm leaving a trail of wreckage in my wake, but I appreciate your willingness to assist in my education. I'll rework this and resubmit it as a new submission.
I took a look at the wishlist request for a configuration page for the keys. I see how the pages are added to the configuration dialog. It's similar to how it's done in a QML plasmoid, just a little more manual bolt-turning and hand-crafting needed in a C++ application. I'll keep this in the back of my mind as a longer-term goal. It's a good combination of subjects I need to learn. Once I get my existing Spectacle backlog cleared, I may look at it further. I now have a laptop with a Print key, so I can experiment with the combinations. But I have smaller challenges to accomplish first.
Again, good luck with the 18.04 beta. I'm running the KDE neon developer edition, so I've already seen some of the in-progress changes. Looks good so far!
I've seen the standard KShortcutsDialog mostly used as a standalone dialog, so I guess it would make sense to turn the Configure button into a menu button, where one entry opens the settings as before, and a new entry would open the shortcuts config.
However, there might be more challenges, i.e. how to plug the existing shortcuts into that dialog.
But I have smaller challenges to accomplish first.
Good call ;)