Change the screenshot button to cancel during the timeout
ClosedPublic

Authored by davidre on Jul 3 2019, 10:18 AM.

Details

Summary

During the timeout period Spectacle now displays an animation in the taskmanager.
If the windows is unminized and the button clicked again (possibly multiple
times) the window will hide and show and hide again (depending on how often you
clicked) while all the button presses are processed.
To avoid this situation change the button to cancel the currently running
process. It then can also be triggered by the appropiate shortcut.

Test Plan
  • Unminimize and click the button
  • Let the screenshot finish, the button is in it's original state
  • cancel a region selection

Diff Detail

Repository
R166 Spectacle
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
davidre created this revision.Jul 3 2019, 10:18 AM
Restricted Application added a project: Spectacle. · View Herald TranscriptJul 3 2019, 10:18 AM
davidre requested review of this revision.Jul 3 2019, 10:18 AM

Cool

src/Gui/KSWidget.cpp
125

Keep the label as it was: "Take a New Screenshot"

131

You forget to set the window title back. It stays at e.g. "2 seconds" if I cancel

139

I would force it to Qt::ToolButtonTextBesideIcon however I tried with diferent styles and it always looked like a QPushButton, so maybe that behavior is only relevant for when inside a QToolbar

170

Unrelated change

222

Clever trick I must say :)

src/Gui/KSWidget.h
30

Coding style. But this can probably just be forward declared here class QAction;

86

While the coding style is ... interesting, I would keep it and the new variables consistent with the rest for now

davidre added inline comments.Jul 3 2019, 11:58 AM
src/Gui/KSWidget.h
86

I thought given enough time maybe every variable will be touched and the style changed

davidre updated this revision to Diff 61067.Jul 3 2019, 1:28 PM
davidre marked 5 inline comments as done.
  • minor comments
davidre added inline comments.Jul 3 2019, 1:28 PM
src/Gui/KSWidget.cpp
170

It looked so bad I thought I had accidentally changed it

davidre updated this revision to Diff 61072.Jul 3 2019, 1:42 PM
davidre marked an inline comment as done.
  • restore window title
broulik added inline comments.Jul 3 2019, 1:59 PM
src/Gui/KSMainWindow.cpp
330

Can't you just call restoreWindowTitle() in any case?

338–342

What is this supposed to do? Do you mean pixmap.isNull() or pixmap.size().isEmpty()? Looks unrelated to the patch, though.

510

Can you perhaps move the setWindowModified() handling into this method, too?

src/Gui/KSWidget.cpp
267

Thinking about it, would using an enum make this nicer?
setButtonState(State::Cancel) or (State::TakeNewScreenshot)` instead of having this one dedicated method for one case but the other one scattered around the code

src/Gui/KSWidget.h
24–25

Unused

davidre added inline comments.Jul 3 2019, 2:07 PM
src/Gui/KSMainWindow.cpp
330

No because if you saved a screenshot and then take a new one we want to the title to be "Unsaved" and not the filename of the previous saved file.

338–342

In this patch I also changed that we go through this function if the region selection is canceled (to reset title and and button).
I tried to move the resize call into the above if but it produced different results and the size was off. This is just another condition that is false when the above happened. I agree it would be better to change it to the same condition as above.

davidre added inline comments.Jul 3 2019, 2:11 PM
src/Gui/KSMainWindow.cpp
510

Do you think of making this a general method to handle windowModified and title setting? Because otherwise I see no need because windowModified doesn't change when we restore the title.

davidre updated this revision to Diff 61075.Jul 3 2019, 2:36 PM
davidre marked 2 inline comments as done.
  • remove unused includes
  • Use an enum
davidre updated this revision to Diff 61076.Jul 3 2019, 2:50 PM
  • Use the same condition

I quite like it. One UI suggestion: when the button is displaying "Cancel" rather than "Take new screenshot", maybe we should give it keyboard focus so you can quickly activate it with the spacebar or return key in addition to via a click.

Pressing Escape will also cancel the timeout

Can we get this into 19.08?

Can't add new i18n strings in stable, unless you ask translators for their OK.

ngraham accepted this revision.Jul 16 2019, 5:52 PM

Yep, we still have two days.

I'll quit the bikeshedding; let's get this in.

This revision is now accepted and ready to land.Jul 16 2019, 5:52 PM
This revision was automatically updated to reflect the committed changes.