Calculate date and time values for file name at time of screen shot rather than time of save
ClosedPublic

Authored by utecht on Aug 27 2018, 5:52 PM.

Details

Summary

Added property to ExportManager to hold timestamp of screenshot that is then used when creating the file name instead of the file's save timestamp.

BUG: 394504
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 requested review of this revision.Aug 27 2018, 5:52 PM
utecht created this revision.
ngraham edited the summary of this revision. (Show Details)Sep 3 2018, 10:34 PM
ngraham added a reviewer: Spectacle.
ngraham set the repository for this revision to R166 Spectacle.

Thanks for this patch! Sorry for the extended review time.

Unfortunately, it doesn't apply against the current state of git master when I attempt to apply it with arc patch D15106:

Created and checked out branch arcpatch-D15106.
Checking patch src/SpectacleCore.cpp...
Hunk #1 succeeded at 183 (offset 1 line).
Checking patch src/ExportManager.h...
Checking patch src/ExportManager.cpp...
error: while searching for:
    KSharedConfigPtr config = KSharedConfig::openConfig(QStringLiteral("spectaclerc"));
    KConfigGroup generalConfig = KConfigGroup(config, "General");

    const QDateTime timestamp = QDateTime::currentDateTime();
    QString baseName = generalConfig.readEntry("save-filename-format", "Screenshot_%Y%M%D_%H%m%S");

    QString title;

error: patch failed: src/ExportManager.cpp:173
Applied patch src/SpectacleCore.cpp cleanly.
Applied patch src/ExportManager.h cleanly.
Applying patch src/ExportManager.cpp with 1 reject...
Hunk #1 applied cleanly.
Rejected hunk #2.

 Patch Failed! 
Usage Exception: Unable to apply patch!

Does this patch assume and require D15059 first?

Can you rebase it and update the patch? You can do that with git pull --rebase.

Also, since it seems like you're interested in helping out with Spectacle and submitting patches, allow me to recommend setting up the arc command-line tool, which makes submitting and updating patches a breeze (har har har). It's really easy: https://community.kde.org/Infrastructure/Phabricator#Using_Arcanist_to_post_patches

I can offer assistance if you need a hand.

utecht updated this revision to Diff 41059.Sep 5 2018, 3:38 PM

I have reverted the commit that was causing the issue on my local feature branch which I hope allows the diff to work. If it doesn't, I'll go one step further and start a new branch from the upstream repository and reapply the changes manually.

Thank you for your tips and patience with this. I'm new to all of this and didn't quite get the git workflow correct the first time. I was able to install arc and will plan to use that with future diffs.

Hmm, still doesn't work:

$  (master) arc
 patch D15106
Created and checked out branch arcpatch-D15106.
Checking patch src/SpectacleCore.cpp...
Hunk #1 succeeded at 183 (offset 1 line).
Checking patch src/ExportManager.h...
Checking patch src/ExportManager.cpp...
error: while searching for:
    KSharedConfigPtr config = KSharedConfig::openConfig(QStringLiteral("spectaclerc"));
    KConfigGroup generalConfig = KConfigGroup(config, "General");

    const QDateTime timestamp = QDateTime::currentDateTime();
    QString baseName = generalConfig.readEntry("save-filename-format", "Screenshot_%Y%M%D_%H%m%S");

    QString title;

error: patch failed: src/ExportManager.cpp:173
Applied patch src/SpectacleCore.cpp cleanly.
Applied patch src/ExportManager.h cleanly.
Applying patch src/ExportManager.cpp with 1 reject...
Hunk #1 applied cleanly.
Rejected hunk #2.

 Patch Failed! 
Usage Exception: Unable to apply patch!

You should be able to just re-base your patch on master with git pull --rebase origin master.

Using arc should help, too.

utecht updated this revision to Diff 41129.Sep 7 2018, 5:04 AM

Let's try this one.

ngraham accepted this revision.Sep 11 2018, 12:40 PM

Better now, thanks! And sorry for the extended review time.

This works well and the code looks sane. I'll land it in 18.08.2 since it's a bugfix and doesn't involve any string changes.

This revision is now accepted and ready to land.Sep 11 2018, 12:40 PM
ngraham edited the summary of this revision. (Show Details)Sep 11 2018, 12:40 PM

Never mind, this patch depends on changes in master, so into 18.12 it goes.

ngraham edited the summary of this revision. (Show Details)Sep 11 2018, 12:45 PM
This revision was automatically updated to reflect the committed changes.
Restricted Application added a project: Spectacle. · View Herald TranscriptSep 11 2018, 12:49 PM