Count down the seconds until a screenshot is taken in the window title
ClosedPublic

Authored by felixernst on Jun 7 2019, 5:48 PM.

Details

Summary

This way the user knows by the progressbar and by the changing
number when the screenshot is going to be taken.

Test Plan

Diff Detail

Repository
R166 Spectacle
Lint
Lint Skipped
Unit
Unit Tests Skipped
felixernst requested review of this revision.Jun 7 2019, 5:48 PM
felixernst created this revision.
felixernst edited the summary of this revision. (Show Details)Jun 7 2019, 8:02 PM

Wouldn't ceil do the same?

felixernst planned changes to this revision.Jun 15 2019, 10:03 AM

Use ceil instead.

But for now I'll wait until D21638: Display delay in the taskmanager has landed and I'll rebase to master then.

Friendly ping my change is now pushed

felixernst updated this revision to Diff 60668.Jun 25 2019, 7:45 PM

Rebase to master

What do you say it is down to one line now! Your latest changes really helped there.
I would have created this diff sooner but it's way too hot in my room and I had to rebuild all dependencies because I switched to developing in a virtual machine. So I was delaying this but seeing that it won't be cooler anytime soon I took this burden on me. :P

I used <QtMath> but I could just as well have used <math.h>. I didn't research what of both might be better. Maybe I'll look into this later.

ngraham requested changes to this revision.Jun 25 2019, 8:08 PM
ngraham added a subscriber: ngraham.
ngraham added inline comments.
src/Gui/KSMainWindow.cpp
295

This needs to be localized with i18n(); "s" doean't mean "seconds" in all languages.

This revision now requires changes to proceed.Jun 25 2019, 8:08 PM
felixernst edited the summary of this revision. (Show Details)Jun 25 2019, 9:55 PM
davidre added inline comments.
src/Gui/KSMainWindow.cpp
296

Is there a difference between qCeil and ceil/std::ceil? I genuinely don't know or what we prefer. Maybe @broulik does?

felixernst updated this revision to Diff 60671.EditedJun 25 2019, 10:32 PM
felixernst marked an inline comment as done.

Make seconds localized
I actually tried to research if "s" is an international
symbol beforehand and it seemed to me like it was
(https://en.wikipedia.org/wiki/International_System_of_Units)
but I trust your call on this.

felixernst added inline comments.Jun 25 2019, 10:42 PM
src/Gui/KSMainWindow.cpp
296

qCeil uses std::ceil internally. It is defined like this in qmath.h:

inline int qCeil(qreal v)
{
    using std::ceil;
    return int(ceil(v));
}
ngraham added inline comments.Jun 28 2019, 1:25 PM
src/Gui/KSMainWindow.cpp
295

Now that I look at this again, I think it needs to use i18np() ("p" for plural) because not every language has the same rules plurals.

So this would be setWindowTitle(i18nc("@title:window seconds", "%1 s", "%1 s", blabla);

And while we're at it, maybe we could use the full word too: setWindowTitle(i18nc("@title:window seconds", "%1 second", "%1 seconds", blabla);

davidre added inline comments.Jun 28 2019, 2:03 PM
src/Gui/KSMainWindow.cpp
295

Shouln't then this be rather i18ncp?

ngraham added inline comments.Jun 28 2019, 3:31 PM
src/Gui/KSMainWindow.cpp
295

Right, my bad.

felixernst marked 3 inline comments as done.

Use i18ncp and have "second/s" instead of an "s"

ngraham accepted this revision.Jun 29 2019, 9:16 PM

@davidre, does this look good to you too?

This revision is now accepted and ready to land.Jun 29 2019, 9:16 PM
davidre accepted this revision.Jun 29 2019, 10:37 PM

Looks good to me. I have just one more idea.
When you were to configure 4.5 seconds it would show 5 for half a second then 4.
Maybe we could something like min(int(timeout), ceil(...)).
@felixernst What do you think?

I would agree with that but the UI only allows for integer values as a timeout right now.

I would agree with that but the UI only allows for integer values as a timeout right now.

You can type in non-integer values

I would agree with that but the UI only allows for integer values as a timeout right now.

You can type in non-integer values

Yep, and In fact I've set the default timeout to 0.5 seconds. :)

felixernst updated this revision to Diff 60885.Jun 30 2019, 8:54 PM

Do not use ceil until the first integer was reached
This way when a non-integer value like 4,5 seconds is entered
it won't show 5 seconds in the title for the first .5 seconds of
the timer.
I also made the math more easily understandable by extracting
calculations into separate lines.

felixernst added a comment.EditedJun 30 2019, 8:56 PM

I thought non-integer values could not be entered because entering a dot there is not possible for me. Comma works though.

ngraham added inline comments.Jun 30 2019, 9:16 PM
src/Gui/KSMainWindow.cpp
295

Prefer descriptive variable names, e.g. timeoutInS -> timeoutInSeconds

felixernst updated this revision to Diff 60889.Jun 30 2019, 9:49 PM
felixernst marked an inline comment as done.

Change "timeoutInS" to "timeoutInSeconds" and add const

ngraham accepted this revision.Jun 30 2019, 11:22 PM

LGTM, I'm ready to land this. Do you agree, @davidre?

I don't have commit access. Would one of you land it for me please?

I never did this before, but let's try it.

This revision was automatically updated to reflect the committed changes.
broulik added inline comments.Jul 3 2019, 10:37 AM
src/Gui/KSMainWindow.cpp
297

You forgot to set it back when the animation stops. When screenshot finishes I now have a "0 seconds" window, also affects D22234

davidre added inline comments.Jul 3 2019, 11:32 AM
src/Gui/KSMainWindow.cpp
297

Hmm I don't see that behavior and also don't understand it. If you screenshot finishes the title should be set to "unsaved" in setScreenshotAndShow

broulik added inline comments.Jul 3 2019, 11:33 AM
src/Gui/KSMainWindow.cpp
297

What I did:

  • In Spectacle main window choose "Rectangular region"
  • Set timeout to 2 seconds
  • Hit "take new screenshot"
  • When region selector comes up, hit Escape to cancel
  • Spectacle unminimizes and now has "0 seconds" as its title
davidre added inline comments.Jul 3 2019, 11:41 AM
src/Gui/KSMainWindow.cpp
297

Ah yes when you cancel. I will fix it in D22234