Display delay in the taskmanager
ClosedPublic

Authored by davidre on Jun 7 2019, 6:33 AM.

Details

Summary

Before when taking a screenshot with delay Spectacle hid itself without any
Feedback to the user. There was no visible indication if the screenshotting
process continued, of the remaining time or if the program was still running.
This displays the display as progress in the taskmanager.

Test Plan

Diff Detail

Repository
R166 Spectacle
Branch
progress (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 12508
Build 12526: arc lint + arc unit
davidre created this revision.Jun 7 2019, 6:33 AM
Restricted Application added a project: Spectacle. · View Herald TranscriptJun 7 2019, 6:33 AM
Restricted Application added a subscriber: Spectacle. · View Herald Transcript
davidre requested review of this revision.Jun 7 2019, 6:33 AM
davidre edited the test plan for this revision. (Show Details)Jun 7 2019, 6:36 AM
davidre added reviewers: Spectacle, broulik, felixernst.
davidre added 1 blocking reviewer(s): broulik.Jun 7 2019, 6:39 AM

Because this is heavily based on @broulik work from KDevelop he needs to agree to relicensing his code from GPL 2 to LGPL 2.

davidre edited the summary of this revision. (Show Details)Jun 7 2019, 6:49 AM
davidre updated this revision to Diff 59314.Jun 7 2019, 6:50 AM

Better default

While I don't think my bits left in the code reach a neccessary threshold of originality, I agree to a relicensing from GPLv2 to LGPLv2+ (not sure why Spectacle as an application is *L*GPL, though)

src/Gui/KSMainWindow.cpp
276

While this QTimer is parented to the KSMainWindow unless it gets deleted you pile up timers, if user takes a couple of screenshots in a row.

src/Gui/TimerProgress.cpp
26

Unused

33

interval with one L

38

Please spread this out over multiple lines for readability

TimerProgress::TimerProgress(...)
    : QObject(parent)
    , mTimer(timer)
    , ...
{
42

Please cleanup the coding style in this area

62

Use QGuiApplication::desktopFileName()

src/Gui/TimerProgress.h
38

I think having a "more timer like" appearance, e.g. updating it every second instead of a continuous animation would provide better feedback for this usecase.

davidre updated this revision to Diff 59323.Jun 7 2019, 9:19 AM
davidre marked 5 inline comments as done.
  • Comments

I thought 1 second was a bit to long so trying 500ms now

davidre marked 2 inline comments as done.Jun 7 2019, 9:20 AM
davidre edited the test plan for this revision. (Show Details)Jun 7 2019, 9:27 AM
felixernst added a comment.EditedJun 7 2019, 10:59 AM

I have never done a review before and I feel like I am going a bit over the top with the testing here. But it's too late now. :)

Edit: I set up arcanist and tested your diff and everything is fine with the delay.
After "Take a New Screenshot" is pressed it takes exactly 4 seconds in your video until Spectacle vanishes. It takes a total of 5 seconds to open Spectacle again with the new screenshot. In my local tests with Spectacle 17.12.3 and 19.07.70 it takes exactly 4 (+-0.1) seconds for Spectacle to reopen. I am not sure this has anything to do with this commit. I just thought I'd mention that. One second extra delay is a bit much and it makes the progressbar less reliable as one can't tell exactly when the screenshot is going to be taken. Maybe it is just lag because of the recording though.
I can't test your exact code right now as I don't have arcanist set up and can't figure out on the spot how to apply the diff.


Comparison video: Spectacle 17.12.3, recording with low quality and high fps. I open the videos in Kdenlive for the exact times of the frames.

About the update rate: I think there are actually two different cases:
If the time is relatively low (<7s or something) then having an update every second isn't that helpful. For example for a 3s delay there would be two ticks total: 1/3 and 2/3. I think for these cases a smoothly filling bar would be best and look great.
For longer delay times a tick every second makes sense because one can very easily tell the rythm of the ticking if it happens 7 times or more. When one gets a feel for the ticking then one can tell exactly when the screenshot is going to be taken. I think a smoothly filling bar could work here as well though.

Another idea probably unrelated to this commit: Maybe the window title itself could additionally count down the seconds in the task manager. Like "Spectacle -- 3s"

This comment was removed by felixernst.

I have never done a review before and I feel like I am going a bit over the top with the testing here. But it's too late now. :)

Edit: I set up arcanist and tested your diff and everything is fine with the delay.
...
About the update rate: I think there are actually two different cases:
If the time is relatively low (<7s or something) then having an update every second isn't that helpful. For example for a 3s delay there would be two ticks total: 1/3 and 2/3. I think for these cases a smoothly filling bar would be best and look great.
For longer delay times a tick every second makes sense because one can very easily tell the rythm of the ticking if it happens 7 times or more. When one gets a feel for the ticking then one can tell exactly when the screenshot is going to be taken. I think a smoothly filling bar could work here as well though.

I don't think having two different behaviors is a good idea. I fear that one could think that one or the other are broken.
With your feedback and from others I would say let's go with continuous.

Another idea probably unrelated to this commit: Maybe the window title itself could additionally count down the seconds in the task manager. Like "Spectacle -- 3s"

I like that idea! Maybe you can do a patch for that? :)

I don't think having two different behaviors is a good idea. I fear that one could think that one or the other are broken.
With your feedback and from others I would say let's go with continuous.

+1

I like that idea! Maybe you can do a patch for that? :)

That's what I've been trying the last hour :D But I am quite new to Qt and kinda failing miserably so far. I'll keep trying.

davidre updated this revision to Diff 59343.Jun 7 2019, 1:33 PM

go back to continuous

davidre edited the test plan for this revision. (Show Details)Jun 7 2019, 1:41 PM
davidre added inline comments.
src/Gui/TimerProgress.cpp
32

Maybe I could make all the timer stuff const QTimer* const or (better?) const QTimer& ?

ngraham added a subscriber: ngraham.Jun 7 2019, 1:46 PM

UI feels great. Really nice feature.

davidre updated this revision to Diff 59381.Jun 8 2019, 7:07 AM
  • Start the capture timer only after the delay

During testing we saw that sometimes the window failed to show
itself again (1 in 30 times estimated by Felix Ernst). It seems
the cause whas that the timeout of the timeout+200ms timer inside
SpectacleCore::takeNewScreenshot which causes the window to show
was handled before the timeout of the timer inside
KSMainWindow::captureScreenshot which runs for timeout and causes
the window to hide. I added debug output to captureScreenshot ("hide")
and setScreenshotAndShow ("show") and took multiple screenshots
until the window didn't show again which resulted in the following
sequence: show hide show hide show ... hide show hide show show hide.

This diff successfully fixes the indefinite hiding of the main window for me but now there is not enough time for the main window to hide in the task bar so the task bar always contains spectacle.

src/Gui/KSMainWindow.cpp
289

A delay is needed to give the task bar time to remove the spectacle entry from view. On my setup 50 ms was enough but this is probably hardware dependent. Maybe we should even keep the old 200 ms delay. Imo:
Change 0 to at least 50

felixernst requested changes to this revision.Jun 8 2019, 10:50 AM
This revision now requires changes to proceed.Jun 8 2019, 10:50 AM

If you look inside SpectacleCore::takeNewScreenshot there is already the delay added. And for me it hides itself in time as it should

felixernst accepted this revision.EditedJun 8 2019, 12:41 PM

And for me it hides itself in time as it should

For me it doesn't at all.

But this bug is unrelated to your change. Sorry for thinking otherwise. Your change only leads to it happening if a delay is selected as well.
I notice now that the bug could maybe just be some weirdness with my setup. It exists for a delay of 0 s on current master as well. I was dumb to compare it to anything else. Beforehand I compared your version in that regard to the one I have installed (17.12.3) where this bug doesn't happen.
The bug doesn't even happen in a Virtual Machine with KDE Neon Dev Edition and Spectacle 19.07.70. So it seems like it might just be some unrelated problem.

If you look inside SpectacleCore::takeNewScreenshot there is already the delay added.

I see. Changing all three occurrences of

200 : 50

to

300 : 150

in SpectacleCore.cpp solves the issue for me. And it probably makes more sense to change it there since my proposed solution creates kind of a loop if I understand it right. But I don't know (yet) what the best solution might be or if a solution is actually necessary.

I think your setup simply exposes that the assumption that 200ms is enough is not always true. Is there something special about it?

felixernst added a comment.EditedJun 8 2019, 1:41 PM

I think your setup simply exposes that the assumption that 200ms is enough is not always true.

Agreed.

Is there something special about it?

Not really. It is a desktop computer with seven year old hardware. It is not even that slow and granted that the bug doesn't happen in a virtual machine on this machine it doesn't seem to have anything to do with speed anyway. It has Kubuntu 18.04 installed though so maybe some interaction between the taskmanager or panels and the up to date spectacle leads to the needed delay.
I don't see a bug report for it so it might really just be a local issue.

broulik added inline comments.Jun 12 2019, 12:06 PM
src/Gui/KSMainWindow.cpp
288

I think deleting an object from within a slot is dangerous, probably should be using timer->deleteLater()

davidre updated this revision to Diff 59644.Jun 12 2019, 12:15 PM
  • Use deleteLater() to delete Timer
davidre marked an inline comment as done.Jun 12 2019, 12:16 PM
davidre added inline comments.
src/Gui/TimerProgress.cpp
50

Should I use deleteLater() here, too?

davidre updated this revision to Diff 59645.Jun 12 2019, 12:22 PM
  • Unused leftover
broulik added inline comments.Jun 12 2019, 12:39 PM
src/Gui/TimerProgress.cpp
50

Since you're not deleting the object that emitted the signal, should be safe

davidre updated this revision to Diff 59647.Jun 12 2019, 12:49 PM
  • Also unminimize when screenshot fails/is canceled
anthonyfieroni added inline comments.
src/Gui/TimerProgress.cpp
34

You don't need to return if you don't use. To not leak make timer parent of TimerProgress.

48–51

Remove, parent is better than delete this.

davidre planned changes to this revision.Jun 16 2019, 2:10 PM

I think will use a Animation for the progress bar which seems less akward then using the second timer.

davidre updated this revision to Diff 60057.Jun 19 2019, 1:17 PM
  • Use QAnimation with suggestions to not return and make the timer the parent
davidre marked 2 inline comments as done.Jun 19 2019, 1:19 PM
davidre updated this revision to Diff 60239.Jun 21 2019, 1:54 PM
  • An own class is not needed anymore
davidre edited the summary of this revision. (Show Details)Jun 21 2019, 2:11 PM
broulik accepted this revision.Jun 21 2019, 3:55 PM
This revision is now accepted and ready to land.Jun 21 2019, 3:55 PM
ngraham accepted this revision.Jun 21 2019, 4:07 PM

So nice!!!

This revision was automatically updated to reflect the committed changes.