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.
Details
- Reviewers
broulik felixernst ngraham - Group Reviewers
Spectacle - Commits
- R166:da42ea07b373: Display delay in the taskmanager
Diff Detail
- Repository
- R166 Spectacle
- Branch
- arcpatch-D21638
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 12721 Build 12739: arc lint + arc unit
Because this is heavily based on @broulik work from KDevelop he needs to agree to relicensing his code from GPL 2 to LGPL 2.
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 | ||
---|---|---|
275 | 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. |
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.
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"
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? :)
+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.
src/Gui/TimerProgress.cpp | ||
---|---|---|
32 | Maybe I could make all the timer stuff const QTimer* const or (better?) const QTimer& ? |
- 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 | ||
---|---|---|
282 | 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: |
If you look inside SpectacleCore::takeNewScreenshot there is already the delay added. And for me it hides itself in time as it should
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?
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.
src/Gui/KSMainWindow.cpp | ||
---|---|---|
281 | I think deleting an object from within a slot is dangerous, probably should be using timer->deleteLater() |
src/Gui/TimerProgress.cpp | ||
---|---|---|
50 | Should I use deleteLater() here, too? |
src/Gui/TimerProgress.cpp | ||
---|---|---|
50 | Since you're not deleting the object that emitted the signal, should be safe |
I think will use a Animation for the progress bar which seems less akward then using the second timer.