Differential D16191

Don't save to a temp file if exporting to another app
ClosedPublic
Actions

Authored by ngraham on Oct 14 2018, 4:02 AM.

Details

Reviewers
broulik
utecht
alexeymin
shubham
Group Reviewers
Spectacle
KDE Applications
Commits
R166:e5fd391d2416: Don't save to a temp file if exporting to another app
Summary

When exporting a screenshot to a local app for further processing, the image is saved to /tmp/, which causes issues in the other app, not least of which is that the file is not writable, so Save operations fail.

This patch makes Spectacle save the file to the default location when exporting to another app.

BUG: 399395
FIXED-IN: 18.12.0

Test Plan
  1. Export → [some app or Other Application...]
  2. Observe that the screenshot is saved at the default location, and then opened in the app of your choice
  3. Edit the image in that app and do File → Save
  4. Observe that it works!

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.
ngraham created this revision.Oct 14 2018, 4:02 AM
Restricted Application added a project: Spectacle. · View Herald TranscriptOct 14 2018, 4:02 AM
ngraham requested review of this revision.Oct 14 2018, 4:02 AM
ngraham edited the test plan for this revision. (Show Details)Oct 14 2018, 4:03 AM
ngraham edited the test plan for this revision. (Show Details)Oct 19 2018, 4:39 AM
alexeymin added a comment.Oct 19 2018, 8:12 AM

Spectacle code is far from perfect, I'm suspect there are many other places to add const or fix Q_FOREACH ...

src/Gui/ExportMenu.cpp
83

While you're here, you could change that deprecated Q_FOREACH to C++11 style range for
for (const KService::Ptr &service: services) {
Of course this is totally unrelated to this patch. But someone sooner or later wil have to do this anyway...
https://www.kdab.com/goodbye-q_foreach/

Or this can be done in separate commit without review request.

84

should be const

88–90

can be const

107–109

can be const

pino added a subscriber: pino.Oct 19 2018, 8:26 AM
pino added inline comments.
src/Gui/ExportMenu.cpp
83

Or this can be done in separate commit without review request.

Yes, please do *not* mix unrelated changes in the same commit.

alexeymin added a comment.Oct 19 2018, 8:31 AM

@pino 👌 , so what would be the preferred way to fix those small but eventually-needed changes?

pino added a comment.Oct 19 2018, 9:02 AM
In D16191#345681, @alexeymin wrote:

@pino 👌 , so what would be the preferred way to fix those small but eventually-needed changes?

They are not needed for *this* review request, so create a new, separate one.

ngraham updated this revision to Diff 43918.Oct 19 2018, 12:52 PM
ngraham marked 2 inline comments as done.

Const-ify const-able variables

This revision is now accepted and ready to land.Oct 19 2018, 1:20 PM
Closed by commit R166:e5fd391d2416: Don't save to a temp file if exporting to another app (authored by ngraham). · Explain WhyOct 19 2018, 1:26 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Msrc/Gui/ExportMenu.cpp (8 lines)

Diff 43924

View Options

src/Gui/ExportMenu.cpp

Loading...