Add Ctrl+T as addditional shortcut for Take Screenshot button
Needs RevisionPublic

Authored by sharvey on Mar 1 2018, 6:56 PM.

Details

Reviewers
ngraham
rkflx
Group Reviewers
Spectacle
Summary

Per request of @rkflx in D10879, add Ctrl+T as additional shortcut to trigger the Take New Screenshot button

Test Plan
  • launch Spectacle in GUI mode
  • attempt to take screenshot by pressing Enter as before (ensure previous behavior unchanged)
  • attempt to take screenshot by pressing Ctrl-T and getting a successful capture.

Diff Detail

Repository
R166 Spectacle
Branch
ctrl-t-shortcut (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
sharvey requested review of this revision.Mar 1 2018, 6:56 PM
sharvey created this revision.
sharvey edited the test plan for this revision. (Show Details)Mar 1 2018, 9:31 PM
rkflx added a comment.Mar 1 2018, 9:36 PM

Nice test plan ;)

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
sharvey updated this revision to Diff 28392.Mar 2 2018, 11:23 AM
  • 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.

ngraham accepted this revision.Mar 2 2018, 9:37 PM

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?

This revision is now accepted and ready to land.Mar 2 2018, 9:37 PM
rkflx requested changes to this revision.Mar 9 2018, 8:18 PM

@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?

This revision now requires changes to proceed.Mar 9 2018, 8:18 PM

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.

rkflx added a comment.EditedMar 9 2018, 8:54 PM

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.
sharvey added a comment.EditedMar 12 2018, 3:35 PM

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.

Indeed, the QTimer::singleShotcall appears to be redundant. The use of animateClick should be sufficient.

Yup.

connect(ctrlT, &QShortcut::activated, this, &KSWidget::newScreenshotClicked);

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 → newScreenshotClicked

Now you are proposing this:

QShortcut::activated → newScreenshotClicked

Personally, 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 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.

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 ;)

rkflx resigned from this revision.Aug 25 2018, 6:43 AM

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).

^^