Use single number as filename for screenshot with empty filename template
AbandonedPublic

Authored by alexeymin on Jan 25 2018, 5:38 PM.

Details

Reviewers
ngraham
egorov
rkflx
Group Reviewers
Spectacle
Summary

Currently, when user deletes filename template in Spectacle settings, we use this empty template and results can look weird. For example, if base directory for screenshots is ~/Pictures, Spectacle will create screenshots with names like ~/Pictures.png ~/Pictures-1.png, ~Pictures-2.png etc.

With this patch, Spectacle will create single-number names with empty template: ~/Pictures/1.png, ~/Pictures/2.png etc.

KSnaphot could behave similarly when given single-number name.

Reported by Vladimir Ivanov.

FEATURE: 373759
FIXED-IN: KDE Applications 18.04

Test Plan
  • Remove filename template in Spectacle.
  • Make usual screenshot and save it, note filename is "1.png" (following ones will have names 2.png, 3.png, ...).
  • Make screenshot from temporary. For example, make screenshot and drag its preview to some text field. Then click 'Paste Location'. Note that temporary file name is "1.png".

Diff Detail

Repository
R166 Spectacle
Lint
Lint Skipped
Unit
Unit Tests Skipped
egorov requested review of this revision.Jan 25 2018, 5:38 PM
egorov created this revision.
ngraham requested changes to this revision.Jan 28 2018, 4:46 AM

Thanks for the patch! Does what it says it does, but I found a corner case: if you delete the template and choose "Save As...", the file save dialog box uses a default filename of ".png". We probably need to add the number there, too.

This revision now requires changes to proceed.Jan 28 2018, 4:46 AM

How about introducing another template like '%#' instead of hard coding the '1'?
This would also allow for %#### to pad with zeros.

Thanks for the patch! Does what it says it does, but I found a corner case: if you delete the template and choose "Save As...", the file save dialog box uses a default filename of ".png". We probably need to add the number there, too.

Good catch! Fixing it.

How about introducing another template like '%#' instead of hard coding the '1'?
This would also allow for %#### to pad with zeros.

Great idea. I was thinking about something similar, but started with just a number. I'll try to implement it.

egorov updated this revision to Diff 26142.Jan 28 2018, 4:27 PM

Fixed "Save As" file naming with empty filename template

How about introducing another template like '%#' ...

Question is: how do we present user with such template? We have one, and this patch fixes behavior when it is empty. How to describe this new template? What will we do when both are empty?

how do we present user with such template?

My guess

kdegraphics/spectacle/src/Gui/SettingsDialog/SaveOptionsPage.cpp:95

Found it by searching for '%Y'

What will we do when both are empty?

Another guess: Default behaviour.

~/Pictures.png
~/Pictures-1.png
...
ngraham accepted this revision.Jan 28 2018, 10:02 PM

Great, works fine now. I'm formally accepting it, but won't land it yet, pending possible future work on the other template. Alternatively, I can land it now, and you can do the other thing in another patch. Your choice.

This revision is now accepted and ready to land.Jan 28 2018, 10:02 PM
egorov added a comment.Feb 5 2018, 3:40 AM

how do we present user with such template?

My guess

kdegraphics/spectacle/src/Gui/SettingsDialog/SaveOptionsPage.cpp:95

Ah, got it. I was thinking about second field to introduce this template, don't know why (question about "both" empty templates originates here).

What will we do when both are empty?

Another guess: Default behaviour.

I propose to forbid empty templates. Current behavior is confusing: we say nothing about how it will work and just use logic which happened to be here.

egorov added a comment.Feb 5 2018, 3:42 AM

Great, works fine now. I'm formally accepting it, but won't land it yet, pending possible future work on the other template. Alternatively, I can land it now, and you can do the other thing in another patch. Your choice.

Let me make a new template (so behavior would be more obvious to user) and then merge it. Also, I forgot to add this logic to docs. Should it go to separate diff or this one?

Docs should be updated in the same diff. Thanks for remembering that@

ngraham edited reviewers, added: Spectacle; removed: bgupta.Feb 5 2018, 3:49 AM

I also tested and it seems to work as described. Is there additional work planned?

ngraham edited the summary of this revision. (Show Details)Feb 10 2018, 3:49 PM

@egorov, are you ready for this to land, or are you planning additional changes?

ngraham edited the summary of this revision. (Show Details)Feb 10 2018, 3:51 PM
rkflx added a subscriber: rkflx.Feb 21 2018, 1:26 PM

As a heads-up, T8036 is also fiddling with the templates.

cfeck added a subscriber: cfeck.Feb 28 2018, 11:17 PM

What is the status of this patch?

cfeck edited the summary of this revision. (Show Details)Feb 28 2018, 11:18 PM
rkflx requested changes to this revision.Mar 9 2018, 8:26 PM

@egorov Thanks for the patch.

It does not apply anymore on master (so it would need rebasing), but reading the summary I wonder whether this really implements what the reporter wants in Bug 373759 and how KSnapshot used to work:

  • Take a snapshot, save as Snap_001.png
  • Take a new snapshot: Spetacle autonumerate as Snap_002.png.
  • Close snapshot and returns to default naming convention for new running.

IOW, in KSnapshot you could Save As, e.g. ABC_420.png, and each subsequent Save (in Spectacle speak) would prepopulate the filename field with a number based on the last used filename, e.g. ABC_421.png in this case. Note that this did not start at 1, but simply detected whether the previously used filename contained a number at the end and incremented that. This had nothing to do with a filename template. (KSnapshot's source code should still be around somewhere to look at how that was done.)


If you simply want to solve the problem of an empty filename template, I'm not sure whether that's needed anymore, because since d47ab66eb9a2 Spectacle defaults to Screenshot in that case, with numbers appended by the usual mechanism.

In case you don't intend to further work on Bug 373759, it might be best to abandon this Diff. For introducing other features, please open a new Diff then.

How about introducing another template like '%#' instead of hard coding the '1'?
This would also allow for %#### to pad with zeros.

Perhaps I'm missing something, but how would this feature work exactly, i.e. where does the initial number come from?

  • Is it 1 after each restart of Spectacle? How to communicate that to the user? What about clashes with existing filenames?
  • Is the last counter value saved in the configuration? How can users reset those values?
  • Is it read from the filename? Can this be reliably expressed with a regular expression, i.e. how to match <old time> on %newT and not on %##?

Sorry for all those doubts, I hope y'all still want to contribute to Spectacle in one way or another despite that ;)

This revision now requires changes to proceed.Mar 9 2018, 8:26 PM

@egorov, any progress on addressing @rkflx's concerns?

rkflx resigned from this revision.Aug 25 2018, 6:43 AM

@egorov, are you still around?

We (me) could take over this patch, if original author is no longer available ๐Ÿ˜•

Please feel free!

alexeymin commandeered this revision.Oct 12 2018, 10:16 AM
alexeymin edited reviewers, added: egorov; removed: alexeymin.
alexeymin updated this revision to Diff 43453.EditedOct 12 2018, 10:46 AM

Original source. I also slightly changed formatting and added const keyword where found possible.

Applies on current master.
Works as before.

ngraham added a subscriber: utecht.Oct 13 2018, 3:27 PM

Yep, it works! Thanks @alexeymin!

However, I notice we how have competing implementations of this feature. @utecht has also submitted D15059, which has the advantage of using the existing token mechanism, and it's got some inline documentation:

However, it has some unnecessary UI changes and seems to have stalled.

I think I prefer the approach to making the sequential number feature a new token, rather than a hidden feature you get only if you remove the existing template, which is not as discoverable or user-friendly. However we don't need the "Next sequential number" chooser like that patch has.

Thoughts on how we proceed? @alexeymin and @utecht?

alexeymin added a comment.EditedOct 13 2018, 6:38 PM

What happens if user deletes filename template in Spectacle settings? Does new token help?

Probably we should merge both, one after another...

What happens if user deletes filename template in Spectacle settings? Does new token help?

Hmm, why would the user do that? If there is no template, the behavior is unclear and undefined. I think it would be better to disallow this state or show an inline error message with a KMessageWidget in this case.

alexeymin added a comment.EditedOct 13 2018, 7:31 PM

D10099 Use single number as filename for screenshot with empty filename template

Description of this diff says that.
This diff fixes just this behaviour, with empty template.
Automatic numbering is already present in Spectacle code

Sorry, I'm not sure I understand. :(

Hmm, why would the user do that? If there is no template, the behavior is unclear and undefined.

This patch originally fixed that, now behavior is clear and defined.

It may be defined, but it's not exactly clear. If we're going to allow an empty text field, we should set a placeholder string so that people can see what the actual pattern will be when the text field is empty. Also, this needs to be documented in the docbook.

alexeymin abandoned this revision.Oct 13 2018, 9:32 PM