FEATURE: 390415
Add an option to automatically save the screenshot immediately after it has been taken
Details
- Reviewers
davidre ngraham - Group Reviewers
VDG Spectacle - Commits
- R166:7f809fc94e9b: Add autosave feature
- Enable the option Autosave the image to 'Save location' with 'Filename'
- Take a screenshot
- 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
- Branch
- B390415 (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 15195 Build 15213: arc lint + arc unit
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?
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.
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 ↗ | (On Diff #64257) | Unrelated whitespace change |
64 ↗ | (On Diff #64257) | Comment needs to be updated |
81 ↗ | (On Diff #64257) | Why change this variable name? Unrelated to this patch |
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 | ||
---|---|---|
270 | false doesn't help, since doSave emits imageSaved, which in turn sends an inline message via KSMainWindow::imageSaved. |
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, ....); });
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 ↗ | (On Diff #64306) | The copying thing is missing here. Just call doCopyToClipboard |
src/ExportManager.h | ||
77 ↗ | (On Diff #64306) | imageSavedAndCopied? |
src/SpectacleCore.cpp | ||
268 | 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. |
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 ↗ | (On Diff #64306) | If I set doCopyToClipboard anywhere between L508 and L514, Spectacle hangs upon creating the file and saving to clipboard. |
src/Gui/KSMainWindow.cpp | ||
128 ↗ | (On Diff #64306) | already connected signal |
src/SpectacleCore.cpp | ||
268 | i can remove the whole line and send a QUrl() in the line below as: |
Did some changes. Please check now.
Also: see the hint for doCopyToClipboard:
https://phabricator.kde.org/D23210#inline-131853
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.
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
Thanks! And having a great mentor, @davidre , makes the code look great too! :)
src/ExportManager.cpp | ||
---|---|---|
507 ↗ | (On Diff #64312) | 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 | ||
492 ↗ | (On Diff #64312) | We can make this action a member variable to reduce code duplication and use it here and in imageSaved. |
src/Gui/KSMainWindow.cpp | ||
---|---|---|
484 ↗ | (On Diff #64405) | We can also move and unify the connect calls into init. |
src/Gui/KSMainWindow.cpp | ||
---|---|---|
484 ↗ | (On Diff #64405) | Took me a while to understand this, as I never worked with lambdas before.. What about the variable location? |