Add autosave feature
ClosedPublic

Authored by aprcela on Aug 16 2019, 9:52 PM.

Details

Summary

FEATURE: 390415
Add an option to automatically save the screenshot immediately after it has been taken

Test Plan

  1. Enable the option Autosave the image to 'Save location' with 'Filename'
  2. Take a screenshot
  3. Image gets saved ASAP

If you launch Spectacle with a hotkey (for example: PrtSc), it takes the screenshot and saves the image immediately.

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.
There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a project: Spectacle. · View Herald TranscriptAug 16 2019, 9:52 PM
Restricted Application added a subscriber: Spectacle. · View Herald Transcript
aprcela requested review of this revision.Aug 16 2019, 9:52 PM
aprcela edited the test plan for this revision. (Show Details)Aug 16 2019, 9:54 PM

Has merge conflicts if D23162 gets approved.

That's okay, you can fix them after that lands.

aprcela edited the test plan for this revision. (Show Details)Aug 16 2019, 10:22 PM
aprcela updated this revision to Diff 63894.Aug 17 2019, 9:07 AM
  • Indicate that it quits only after user's copy/save interaction
aprcela updated this revision to Diff 64056.Aug 19 2019, 4:40 PM
  • Merge with D23162 and update config layouts

I hope i did the merge with D23162 correctly. This is what it would look like:

Needs a rebase

aprcela updated this revision to Diff 64254.Aug 21 2019, 9:37 PM
  • Merge with D23162 and update config layouts
  • Rebase with D23162 and modify to use enum
aprcela updated this revision to Diff 64257.Aug 21 2019, 9:45 PM
  • Resolved merge conflicts
aprcela added a subscriber: davidre.EditedAug 21 2019, 9:47 PM

Needs a rebase

Done. Needs review by @davidre

Layout looks like this:

Do we want autosave and save to clipboard as radio buttons, so only one can be saved. Or do we want it separate, so a User can have both options active?

Works great.

However, now that I see the three radio buttons, I realize that it might be nice to be able to auto-save as well as copy to clipboard, which seems to be what https://bugs.kde.org/show_bug.cgi?id=390415 is asking for. I wouldn't use it, but I guess the reporter would.

So instead of three radio buttons ("do nothing", "copy to clipboard", "auto save"), maybe we could actually simplify the UI to two checkboxes ("copy to clipboard", "auto save"), with both being unchecked implying "do nothing".

Thoughts?

aprcela added a comment.EditedAug 21 2019, 10:01 PM

Works great.

However, now that I see the three radio buttons, I realize that it might be nice to be able to auto-save as well as copy to clipboard, which seems to be what https://bugs.kde.org/show_bug.cgi?id=390415 is asking for. I wouldn't use it, but I guess the reporter would.

So instead of three radio buttons ("do nothing", "copy to clipboard", "auto save"), maybe we could actually simplify the UI to two checkboxes ("copy to clipboard", "auto save"), with both being unchecked implying "do nothing".

Thoughts?

Ok, I'll go with that one. Two checkboxes instead of three radiobuttons sounds good :)

Other option would be:

Give me a bit, atm fighting with git worktrees here :D

@davidre Can we still use the enums if we'd go with checkboxes only (instead of Radiobuttons)?

Actually I was envisioning something even simpler:

After taking a screenshot: [ ] Copy image to clipboard
                           [ ] Autosave the image to the default save location

Then we don't need any radio buttons at all, and the "do nothing" option is implied by not having either one checked.

Works great.

However, now that I see the three radio buttons, I realize that it might be nice to be able to auto-save as well as copy to clipboard, which seems to be what https://bugs.kde.org/show_bug.cgi?id=390415 is asking for. I wouldn't use it, but I guess the reporter would.

So instead of three radio buttons ("do nothing", "copy to clipboard", "auto save"), maybe we could actually simplify the UI to two checkboxes ("copy to clipboard", "auto save"), with both being unchecked implying "do nothing".

Thoughts?

Ok, I'll go with that one. Two checkboxes instead of three radiobuttons sounds good :)

Other option would be:

Give me a bit, atm fighting with git worktrees here :D

@davidre Can we still use the enums if we'd go with checkboxes only (instead of Radiobuttons)?

If we go with two checkboxes the enum approach wouldn't work anymore. It's best suited for when you have multiple exclusive things. If we go with two checkboxes two bools are the way to go. Sorry I made you go through the enum hassle yesterday but I thought the radiobuttons were the consensus.

src/Gui/SettingsDialog/GeneralOptionsPage.cpp
45

Unrelated whitespace change

62–63

Comment needs to be updated

78

Why change this variable name? Unrelated to this patch

aprcela updated this revision to Diff 64277.Aug 22 2019, 9:41 AM
aprcela marked 3 inline comments as done.
  • Rewrite to not use enum

Update it to not use enum. Don't worry, it wasn't useless. I learned more :)

With this approach, we have one small problem. Now it tries to send both notifications. First the one that the image has been autosaved to location x, then the one that it has been copied to clipboard. This leads to not showing the first one at all, and the second notification gets drawn with a lag.
See comment at L275 in SpectacleCore.cpp

One solution, the only one currently that I can come up with, is to make ExportManager::doAutoSave which would emit imageAutoSaved and that one then calls KSMainWindow::imageAutoSaved which does only L478 and L479 (all three I mentioned, have to be created).
https://cgit.kde.org/spectacle.git/tree/src/Gui/KSMainWindow.cpp#n478

Do you have any idea of how to solve this double notification problem more simple?

src/SpectacleCore.cpp
272

false doesn't help, since doSave emits imageSaved, which in turn sends an inline message via KSMainWindow::imageSaved.

davidre added a comment.EditedAug 22 2019, 12:55 PM

Update it to not use enum. Don't worry, it wasn't useless. I learned more :)

With this approach, we have one small problem. Now it tries to send both notifications. First the one that the image has been autosaved to location x, then the one that it has been copied to clipboard. This leads to not showing the first one at all, and the second notification gets drawn with a lag.
See comment at L275 in SpectacleCore.cpp

One solution, the only one currently that I can come up with, is to make ExportManager::doAutoSave which would emit imageAutoSaved and that one then calls KSMainWindow::imageAutoSaved which does only L478 and L479 (all three I mentioned, have to be created).
https://cgit.kde.org/spectacle.git/tree/src/Gui/KSMainWindow.cpp#n478

Do you have any idea of how to solve this double notification problem more simple?

First I would drop the notification and only show an inline message. For the problem that multiple inline messages are displayed after another: First let's simplifiy the logic here:
Instead of two two level ifs

if( copyToClipboard && autosave) {
    ...
} else if (copyToclipboard) {
    ...
} else if (autosave) {
}

For the feedback we could probably disconnect the mainwindow from the signal and then after sucessful save connect it back. Something like

disconnect(lExportManager, &ExportManager::imageSaved, mMainWindow, ....);
connect(lExportManager, &ExportManager::imageSaved, [](const QUrl& url) {
      mMainWindow->showInlineMessage(...);
      connect(lExportManager, &ExportManager::imageSaved, mMainWindow, ....);
});
davidre added a comment.EditedAug 22 2019, 12:59 PM

Or we create an ExportManager::doSaveAndCopy method which emits a new signal which then we connect to a new KSMainWindow::showSavedAndCopiedFeedback.

aprcela updated this revision to Diff 64306.Aug 22 2019, 1:39 PM
  • Add new AutoSave related methods.

Or we create an ExportManager::doSaveAndCopy method which emits a new signal which then we connect to a new KSMainWindow::showSavedAndCopiedFeedback.

I've done it with this approach.
If this is ok, we can go on with the next problem:
If autosave is enabled, then it overrides the option under Save: Copy file location to clipboard after saving. Maybe disable this one, if autosave is checked?

Great! Now we also have to set the windowTitle and modification status like in KSMainWindow::imageSaved. Maybe instead of the showFeedback method we could do a imageCopiedAndSaved method inside KSMainWindow and connect that to the signal of the ExportManager. Like the imageSaved.
I think that would be cleaner. Do you agree? And after that one of us could do the same for just copying so that everything is consistent.

src/ExportManager.cpp
507

The copying thing is missing here. Just call doCopyToClipboard

src/ExportManager.h
77

imageSavedAndCopied?

src/SpectacleCore.cpp
271

mFileNameUrl is not used in GuiMode jus save to SpectacleConfig::defaultSaveLocation()

src/SpectacleCore.h
95 ↗(On Diff #64306)

we can make this a local variable we only use it in one method.

aprcela marked 5 inline comments as done.Aug 22 2019, 2:56 PM

Great! Now we also have to set the windowTitle and modification status like in KSMainWindow::imageSaved. Maybe instead of the showFeedback method we could do a imageCopiedAndSaved method inside KSMainWindow and connect that to the signal of the ExportManager. Like the imageSaved.
I think that would be cleaner. Do you agree? And after that one of us could do the same for just copying so that everything is consistent.

imageCopiedAndSaved is done, as showSavedAndCopiedFeedback, shall I rename it to imageCopiedAndSaved or imageSavedAndCopied ,to keep naming consistent with the other namings (doSaveAndCopy etc.)? And the signal is connected :)

New problem: doCopyToClipboard inside doSaveAndCopy, see note

src/ExportManager.cpp
507

If I set doCopyToClipboard anywhere between L508 and L514, Spectacle hangs upon creating the file and saving to clipboard.
If it's at L519, Spectacle doesn't hang up for a while but the showInlineMessage animations hangs for a brief moment..

src/Gui/KSMainWindow.cpp
128

already connected signal

src/SpectacleCore.cpp
271

i can remove the whole line and send a QUrl() in the line below as:
lExportManager->doSave(QUrl());

aprcela updated this revision to Diff 64312.Aug 22 2019, 3:17 PM
aprcela marked 2 inline comments as done.
  • Renameing and reordering
aprcela marked 2 inline comments as done.Aug 22 2019, 3:22 PM

Did some changes. Please check now.
Also: see the hint for doCopyToClipboard:

https://phabricator.kde.org/D23210#inline-131853

Great! Now we also have to set the windowTitle and modification status like in KSMainWindow::imageSaved. Maybe instead of the showFeedback method we could do a imageCopiedAndSaved method inside KSMainWindow and connect that to the signal of the ExportManager. Like the imageSaved.
I think that would be cleaner. Do you agree? And after that one of us could do the same for just copying so that everything is consistent.

What exactly do you mean with 'for just copying' ? Add a signal/slot for Copying to clipboard, besides the doCopyToClipboard ?

Just wanted to chime in and say that this is looking super-great!

Did some changes. Please check now.
Also: see the hint for doCopyToClipboard:

https://phabricator.kde.org/D23210#inline-131853

Great! Now we also have to set the windowTitle and modification status like in KSMainWindow::imageSaved. Maybe instead of the showFeedback method we could do a imageCopiedAndSaved method inside KSMainWindow and connect that to the signal of the ExportManager. Like the imageSaved.
I think that would be cleaner. Do you agree? And after that one of us could do the same for just copying so that everything is consistent.

What exactly do you mean with 'for just copying' ? Add a signal/slot for Copying to clipboard, besides the doCopyToClipboard ?

Will look at the code later but with that I meant using the same code structure for copying to the clipboard in a follow up patch. Emitting a signal in export manager and connecting to that in the window. Just to make the control flow consistent.

Will look at the code later but with that I meant using the same code structure for copying to the clipboard in a follow up patch. Emitting a signal in export manager and connecting to that in the window. Just to make the control flow consistent.

Ah, now I get it :)
Keep in mind that there might be a Task settings panel soon. So we might have to rewrite some bunch. -> https://phabricator.kde.org/T11422

Just wanted to chime in and say that this is looking super-great!

Thanks! And having a great mentor, @davidre , makes the code look great too! :)

Yes, @davidre is amazing.

davidre added inline comments.Aug 23 2019, 9:15 AM
src/ExportManager.cpp
507

I don't think it's too bad. If you watch it doesn't hang but starts when the image is saved (you can see the window title changing). Can you try to get a video if I am seeing something different than you?

src/Gui/KSMainWindow.cpp
503

We can make this action a member variable to reduce code duplication and use it here and in imageSaved.

aprcela updated this revision to Diff 64405.Aug 23 2019, 9:54 AM
aprcela marked an inline comment as done.
  • Reduce code duplication
aprcela marked an inline comment as done.Aug 23 2019, 9:55 AM
aprcela added inline comments.
src/ExportManager.cpp
507

Can't reproduce it now. Maybe my reboot last night helped..

src/Gui/KSMainWindow.cpp
503

Changed.
Gets set in init()
Name: mOpenContaining

davidre added inline comments.Aug 23 2019, 10:04 AM
src/Gui/KSMainWindow.cpp
484–485

We can also move and unify the connect calls into init.

aprcela marked 4 inline comments as done.Aug 24 2019, 2:50 PM
aprcela added inline comments.
src/Gui/KSMainWindow.cpp
484–485

Took me a while to understand this, as I never worked with lambdas before..

What about the variable location?
It's local to the scope of this method.

aprcela updated this revision to Diff 64518.Aug 24 2019, 9:48 PM
  • Reduce code duplication
aprcela marked 2 inline comments as done.Aug 24 2019, 9:49 PM

Done , tested & working :)

davidre added inline comments.Aug 26 2019, 8:32 AM
src/Gui/KSMainWindow.cpp
485

Sorry, I totally missed that but good idea to use lastSaveFile().

src/SpectacleCore.cpp
271

Since doSave and doSaveAndCopy(const QUrl &url = QUrl()) alreadyy have QUrl() as default argument you can just call them without providing a argument.

aprcela updated this revision to Diff 64646.Aug 26 2019, 8:47 AM
aprcela marked 2 inline comments as done.
  • Remove unnecessary variables

Removed the unnecessary variables.
Tested & working.

davidre accepted this revision.Aug 26 2019, 9:10 AM

Thanks!

This revision is now accepted and ready to land.Aug 26 2019, 9:10 AM
ngraham accepted this revision.Aug 26 2019, 2:14 PM
This revision was automatically updated to reflect the committed changes.