(3/3) add option to quit after save or copy operations
ClosedPublic

Authored by ngraham on Feb 4 2018, 4:56 PM.

Details

Summary

Part 3 of 3 for T7841: Revamp buttons on the bottom to solve various usability issues

FEATURE: 389773
FIXED-IN: KDE Applications 18.04

We add a checkbox visible on the main UI to quit Spectacle after save or copy operations. This yields the following benefits:

  • We can remove the "Save & Exit" item from the split button
  • Users gain the ability to have spectacle quit after copying the image to the clipboard

I tried to make the feature apply to Export operations as well, but that proved to be much more complicated and I decided to abandon that for now and do it later in a subsequent patch.

Test Plan

Tested in KDE Neon:

  • With "Quit after Copy or Save" unchecked:
    • Save: image is saved
    • Save As: file save dialog shown, image is saved
    • Copy to Clipboard: message is shown, image is copied to clipboard
  • With "Quit after Copy or Save" checked:
    • Save: notification shown, Spectacle quits, image is saved
    • Save As: file save dialog shown, notification shown, Spectacle quits, image is saved
    • Copy to Clipboard: image is copied to clipboard (if Klipper is set up to accept images), Spectacle quits

Diff Detail

Repository
R166 Spectacle
Branch
arcpatch-D10301
Lint
No Linters Available
Unit
No Unit Test Coverage
There are a very large number of changes, so older changes are hidden. Show Older Changes
ngraham requested review of this revision.Feb 4 2018, 4:56 PM
ngraham edited the summary of this revision. (Show Details)
ngraham edited the summary of this revision. (Show Details)Feb 4 2018, 5:37 PM
ngraham edited the summary of this revision. (Show Details)
rkflx requested changes to this revision.Feb 7 2018, 1:19 AM
rkflx added a subscriber: rkflx.

Reviewed this, there are a couple of issues.

doc/index.docbook
166

"will quit": prepend "option" or remove "The"

Save or Copy → save or copy

Quit → Close? (would help those not knowing what quitting is)

179

"No context available", but please make sure to also replace all references to "Save & Exit" with the new option.

179

Save or Copy → saving or copying?

Quit → Close? (would help those not knowing what quitting is)

src/Gui/KSMainWindow.cpp
199

Now Ctrl+Q does not work anymore.

262–264

That's wrong: This way Print does not quit when accepting, but when cancelling (IMO it should never quit, so perhaps edit the test plan and remove this code).

269–271

Personally, I wouldn't quit here either. Did not test this part, but at least I'd expect some form of feedback about success or error.

294–297

Is pasting a screenshot after Copy To ClipboardQuit actually working? For me it isn't, regardless of the state of the checkbox. Can you confirm?

325–326

I see you copied this over, however do you know what this is for? Perhaps related to Bug 389694?

333

Somewhere there's a bug: Cancelling the Save As dialog quits, but it should only abort the dialog.

src/Gui/KSMainWindow.h
84

Why are you transforming this into a member variable instead of keeping it a local variable? SpectacleConfig::instance() is a static returning only a reference anyway.

src/Gui/KSWidget.cpp
111

Quit → Close? (would help those not knowing what quitting is)

spectacle → Spectacle

114

Makes sense probably, but could you change the spacing so Options is closer to the group it is related to instead of being closer to the group above?

src/Gui/KSWidget.h
79

Change name accordingly?

This revision now requires changes to proceed.Feb 7 2018, 1:19 AM

Thanks for the thorough review! I'll work on these.

src/Gui/KSMainWindow.cpp
294–297

Oh right, this doesn't work yet on X11 because the image can't be saved to the standard X11 clipboard. See D9117 for more information. The patch author there might be able to get it working with a bit of technical assistance (a bit beyond my level, I'm afraid).

rkflx added inline comments.Feb 7 2018, 10:40 AM
src/Gui/KSMainWindow.cpp
294–297

I could get it to work for manually copying and quitting by unchecking "Ignore images" in Klipper's settings (it's checked by default, but why?). Still, for auto-quit I'm still unable to paste afterwards and the image does not appear in Klipper.

It might be a problem in your code, if it works on X11 in other cases.

ngraham updated this revision to Diff 26732.Feb 8 2018, 12:05 AM

Merge master and make a first pass at addressing review comments

ngraham updated this revision to Diff 26734.Feb 8 2018, 12:10 AM
ngraham marked 3 inline comments as done.

More changes

ngraham updated this revision to Diff 26735.Feb 8 2018, 12:11 AM

One more fix

ngraham marked 6 inline comments as done.Feb 8 2018, 12:13 AM
ngraham added inline comments.Feb 8 2018, 12:34 AM
src/Gui/KSMainWindow.cpp
325–326

This is to prevent Spectacle from quitting before the notification is displayed, I believe. I'm open to better ways of accomplishing the same thing, because I've noticed that bug, too.

ngraham updated this revision to Diff 26740.Feb 8 2018, 12:34 AM

One more fix

rkflx added a comment.Feb 8 2018, 12:48 AM

Great, now arc patch works ;)

I'll look at the rest later.

src/Gui/KSMainWindow.cpp
325–326

Okay, thanks for checking. I have no time to investigate this further currently.

Let's do it this way: Keep the calls everywhere they are needed, and we have the bug to track the issue (perhaps add a comment over there).

ngraham edited the summary of this revision. (Show Details)Feb 8 2018, 1:49 AM
rkflx added a comment.Feb 8 2018, 9:13 PM

I'll look at the rest later.

Looks good so far, I'm confident you'll solve the remaining issues eventually.

src/Gui/KSMainWindow.cpp
347

//?

rkflx added a comment.Feb 8 2018, 9:18 PM

Oops, spoke too soon.

doc/index.docbook
225

Instead of removing, adapt to Save / Save As.

ngraham updated this revision to Diff 26858.EditedFeb 10 2018, 4:39 AM

Address review comments; almost everything works now (just need to restore Ctrl+Q quit shortcut).

Video of copy-and-quit working properly when Klipper accepts images:

ngraham marked 8 inline comments as done.Feb 10 2018, 4:40 AM
ngraham marked an inline comment as done.Feb 10 2018, 4:43 AM
ngraham added a comment.EditedFeb 10 2018, 5:31 AM

Still working on re-activating the Quit shortcut. For some reason this is not obvious to me. A hint would be appreciated if this is really easy and someone else sees it.

ngraham edited the summary of this revision. (Show Details)Feb 10 2018, 5:33 AM
ngraham updated this revision to Diff 26867.EditedFeb 10 2018, 1:07 PM

Restore Ctrl+Q quit shortcut (had to actually add the QAction to something in order for it to be active).

ngraham marked an inline comment as done.Feb 10 2018, 1:08 PM
ngraham edited the summary of this revision. (Show Details)Feb 10 2018, 3:52 PM

Yay, better than before ;)

BUG: 389691

Nope, pressing Ctrl+S or Ctrl++S multiple times still triggers the warning. Let's solve this in a different patch.

Also, don't forget to adapt your test plan regarding Print.


There still seems to be a problem: Copy To Clipboard does not quit Spectacle for me. Note that before, Bug 389694 was only triggered by "Configure Notifications", while you now have the bug far more often.

Also it behaves really strange, making me think that quitting only after the notification was shown might not be needed at all:

ActionPromptNotification
Copyblocked forever
Savereturns after hiding notificationshown
Save Asreturns immediatelyshown
Configure notificationblocked forever

Note that applying D10424 does not change anything for me in the table above.

The "blocked forever" for Copy has to go, the rest can be solved in D10424.

src/Gui/KSMainWindow.cpp
196–197

This works for me too and might be more modern Qt:

QAction *actionQuit = KStandardAction::quit(qApp, &QApplication::quit, this);
actionQuit->setShortcut(QKeySequence::Quit);

Yay, better than before ;)

BUG: 389691

Nope, pressing Ctrl+S or Ctrl++S multiple times still triggers the warning. Let's solve this in a different patch.

Ah, you're right, you need to hit 'em multiple times. Weird.

There still seems to be a problem: Copy To Clipboard does not quit Spectacle for me. Note that before, Bug 389694 was only triggered by "Configure Notifications", while you now have the bug far more often.

Hmm, that's odd. It totally works for me, and I don't hit Bug 389694, either. Checking "Quit after save or copy" and clicking the Copy to Clipboard button saves the image to the clipboard and exits Spectacle.

I wonder what might account for these discrepancies. I'm doing this in a KDE Neon VM FYI.

ngraham edited the summary of this revision. (Show Details)Feb 11 2018, 3:34 AM
ngraham edited the test plan for this revision. (Show Details)
ngraham updated this revision to Diff 26902.Feb 11 2018, 3:40 AM
ngraham marked an inline comment as done.

Modernize Qt code for quitting via keyboard shortcut

Actually never mind, you're right, I can reproduce the hang forever with copy-and-quit. It's because the allDone() signal is never emitted after the main window is hidden. Normally the notification emits that after it exits, but for the copy case, there is no notification, so we never get the signal, so Spectacle never exits.

rkflx added a comment.EditedFeb 11 2018, 11:56 AM
ActionPromptNotification
Copyblocked forever
Savereturns after hiding notificationshown
Save Asreturns immediatelyshown
Configure notificationblocked forever

Now it gets interesting: For Save the Open button of the notification works, while it fails now for Save As. This probably means Save is the way to go (and the corresponding code is correct), Save As must be fixed and Configure notification has to be solved in Plasma or KNotification.

Still, we'd need to find a reliable way to send something to the clipboard before quitting. Perhaps this is caused by the event loop?

rkflx added a comment.Feb 11 2018, 1:01 PM

Still, we'd need to find a reliable way to send something to the clipboard before quitting. Perhaps this is caused by the event loop?

Putting in some sleeping() solves it for me, but that is ugly. However, I noticed something else from a UI standpoint:

  • Spectacle uses a 10sec timeout for the KMessageWidget, which seems quite long compared to Kate's or Plasma's notifications.
  • The KMessageWidget is slightly annoying, because it moves the buttons around.

How about replacing KMessageWidget with a KNotification, and also show a notification on quit? I tested it: Works great and solves both problems. Let me know if I should refactor/clean up the code and put it up for review.

src/Gui/KSMainWindow.cpp
362

Save As must be fixed

qApp->setQuitOnLastWindowClosed(false);
hide();
ngraham updated this revision to Diff 26960.Feb 11 2018, 9:01 PM

Use a hideous hack for now to make quit-after-copy work properly

rkflx added a comment.Feb 11 2018, 9:17 PM

Yay! Works for me too, let's just do our timeout hack.

Even if we adapt Klipper: I think you should update the docbook too to mention the caveat, because not everybody will be running Spectacle with Klipper (regardless of within Plasma or not).

Not sure about the tooltip (I guess adapting it dynamically to a running clipboard manager is too complicated.)

src/Gui/KSMainWindow.cpp
317

Modernize like you've done before…

362

Ping (otherwise clicking on Open in the notfication is broken).

ngraham updated this revision to Diff 26961.Feb 11 2018, 9:18 PM

Also fix Save As case

ngraham marked an inline comment as done.Feb 11 2018, 9:18 PM
ngraham marked an inline comment as done.Feb 11 2018, 9:20 PM
ngraham updated this revision to Diff 26962.Feb 11 2018, 9:22 PM

Make the hideous hack at least a modern hideous hack

ngraham updated this revision to Diff 26963.Feb 11 2018, 9:25 PM

Mention the caveat regarding quit-after-copy in the docbook

ngraham marked an inline comment as done.Feb 11 2018, 9:26 PM
rkflx accepted this revision.Feb 11 2018, 9:31 PM

It's done!

Make the hideous hack at least a modern hideous hack

:D

This revision is now accepted and ready to land.Feb 11 2018, 9:31 PM
ngraham updated this revision to Diff 26964.Feb 11 2018, 9:34 PM
  • Merge branch 'master' into arcpatch-D10301
This revision was automatically updated to reflect the committed changes.