Differential D16304

Add default file name token for (padded) sequential file numbering
ClosedPublic
Actions

Authored by utecht on Oct 18 2018, 6:22 PM.

Details

Reviewers
alexeymin
ngraham
Commits
R166:754e1d3a92c1: Add default file name token for (padded) sequential file numbering
Summary

Default file name tokens %d will add an auto-incremented sequential file number when creating file names.
Numbering starts with 1. If the save directory has other files present with the same name (excluding
extension) but another number in the place of %d, the next number after that of the existing file will be
used to create the new file name.
The sequential number can be padded with %Nd where N is a number of total digits. If the padded token is
used in the
default file name, only file names with a number of the padded length or longer will be considered when
determining the next sequential number to use in the file name. For example, with the save name like
'Screenshot-%3d' and a save directory with only 'Screenshot-3,' the next generated file name will be
'Screenshot-001' since the existing file's number portion is not at least as long as the padded number
length.

BUG: 358641
BUG: 373759
FIXED-IN: 18.12.0

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.
utecht created this revision.Oct 18 2018, 6:22 PM
Restricted Application added a project: Spectacle. · View Herald TranscriptOct 18 2018, 6:22 PM
Restricted Application added a subscriber: Spectacle. · View Herald Transcript
utecht requested review of this revision.Oct 18 2018, 6:22 PM
utecht added a comment.Oct 18 2018, 6:29 PM

BUG: 358641
BUG: 373759

alexeymin requested changes to this revision.EditedOct 18 2018, 6:29 PM
alexeymin added a subscriber: alexeymin.

Resolve confilcts between <<<<<<< and >>>>>>> in several places?
Pretty sure this won't compile 😆

Also, will it integrate some ideas from D10099 ? I mean what to do with empty filename template, maybe automatically assume %d for format?

src/ExportManager.cpp
165

Well, that's fun

This revision now requires changes to proceed.Oct 18 2018, 6:29 PM
shubham added a subscriber: shubham.Oct 18 2018, 6:32 PM
shubham added inline comments.
src/ExportManager.cpp
165

Stashed away changes

shubham removed a subscriber: shubham.Oct 18 2018, 6:32 PM
utecht updated this revision to Diff 43891.Oct 18 2018, 6:33 PM

removed some merge conflict markers

alexeymin added inline comments.Oct 18 2018, 6:40 PM
src/Gui/SettingsDialog/SaveOptionsPage.cpp
126

here?

shubham added a comment.Oct 18 2018, 6:42 PM

@utecht Please don't rush with patches. Take time to resolve all the issues and then compile it to be sure it works.

utecht updated this revision to Diff 43892.Oct 18 2018, 6:46 PM

Removed more merge conflict markers. Sorry. Updated local method call to use upstream name.

ngraham added a subscriber: ngraham.Oct 18 2018, 6:50 PM

Thanks, will test and review later today!

The keywords go in the Summary section, not in a comment. Please add

BUG: 358641
BUG: 373759
FIXED-IN: 18.12.0

to the Summary section. Thanks!

utecht edited the summary of this revision. (Show Details)Oct 18 2018, 7:03 PM
utecht added a comment.Oct 18 2018, 7:32 PM

I think the empty file name template setting defaulting to sequential file number is an interesting idea. It is a good fail safe. If that seems like a reasonable add on to this diff, I can add %d as the default string coming from SpectacleConfig if save-filename-format is empty.

Alternatively I think that ngraham had suggested that the settings form could be amended to prevent the user from leaving that field blank. Thoughts?

ngraham added a comment.Oct 18 2018, 7:40 PM
In D16304#345574, @utecht wrote:

I think the empty file name template setting defaulting to sequential file number is an interesting idea. It is a good fail safe. If that seems like a reasonable add on to this diff, I can add %d as the default string coming from SpectacleConfig if save-filename-format is empty.

I'm fine with that as long as it visually takes the form of placeholder text in the text field when empty (i.e. %d would be written in light gray like all other text fields' placeholder text). That would communicate to the user that it's the default/fallback choice. This is probably a bit more humane than my other idea of showing an error message if the text field is blank.

utecht updated this revision to Diff 43896.Oct 18 2018, 8:24 PM

Add failsafe for blank filename in save options form. Placeholder and default file name is now %d. Also
improve regex that checks for similar file names for edge case with only sequential number file name.

ngraham accepted this revision.Oct 18 2018, 11:14 PM
ngraham added a subscriber: shubham.

I like it! This works great. Code looks good to me too, though I'll let other reviewers have their say as well. This happens to cause one minor visual regression which is not your fault (issue with my recent change to the settings dialog layout) and I will fix that separately.

@alexeymin, and @shubham, what do you think?

alexeymin added a comment.Oct 18 2018, 11:46 PM

I'll test tomorrow, going to sleep >_<

alexeymin requested changes to this revision.Oct 19 2018, 7:49 AM
alexeymin added inline comments.
src/ExportManager.cpp
202

*dir is never deleted? Suggest using either QDir dir(baseDir) or QScopedPointer<QDir> dir(new QDir(baseDir)). Better first, why not create on stack?

203

Style: missing space after comma

209

can be const

210

can be const

214

can be const

219

Why not C++11-style for-loop?
for (const QString &filteredFile: filteredFiles) { ...

228

can be const

src/SpectacleConfig.cpp
254–256

can be const

This revision now requires changes to proceed.Oct 19 2018, 7:49 AM
utecht updated this revision to Diff 43926.Oct 19 2018, 2:18 PM

Fixed memory leak. Added const to several QStrings. Update for loop to C++11 style.

utecht added a comment.Oct 19 2018, 2:21 PM

Thank you for your patience and feedback on this. I appreciate it since I'm still a bit new to C++ and this workflow. Would you like me to check the done boxes above with the in line diff comments? Or is that something the reviewer(s) do? Thanks.

ngraham added a comment.Oct 19 2018, 2:33 PM

Yes, when you change the code to address a comment, you check its "Done" checkbox and then click Submit on the bottom of the page.

utecht marked 10 inline comments as done.Oct 19 2018, 2:36 PM
utecht marked an inline comment as done.
alexeymin added a comment.Oct 19 2018, 3:19 PM

Tested, works fine otherwise.

src/ExportManager.cpp
203

This string list can also be const

utecht updated this revision to Diff 43937.Oct 19 2018, 3:49 PM

Made QStringList const too.

alexeymin accepted this revision.Oct 19 2018, 3:59 PM

Fine, @utecht do you have a developer account to land this?

This revision is now accepted and ready to land.Oct 19 2018, 3:59 PM
utecht added a comment.Oct 19 2018, 6:24 PM

I don't think so. I have a KDE identity and login for the KDE bugzilla. Is one of these what you're asking about?

ngraham added a comment.Oct 19 2018, 10:38 PM

Nah, to land patches on your own, you need a Contributor account (see https://techbase.kde.org/Contribute/Get_a_Contributor_Account).

I'll land this one for you. Thanks so much for the awesome contributions to Spectacle!

Closed by commit R166:754e1d3a92c1: Add default file name token for (padded) sequential file numbering (authored by utecht, committed by ngraham). · Explain WhyOct 19 2018, 10:39 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 43953

View Options

src/ExportManager.cpp

Loading...
View Options

src/Gui/SettingsDialog/SaveOptionsPage.cpp

Loading...
View Options

src/SpectacleConfig.cpp

Loading...