This patch allow the use of "/" in the filename field of the config dialog to save screenshots in auto-generated subdirectories inside the default save directory.
FEATURE: 394183
Details
Details
Tested for local and remote (Google Drive) save locations.
Diff Detail
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.
Comment Actions
Cool, thanks for the patch! I'll hopefully be able to test it out soon.
For now, please move the bug number from the title to the summary (see https://community.kde.org/Infrastructure/Phabricator#Add_special_keywords for details).
Comment Actions
Thanks, title and summary are looking much better now.
Tested the patch now. Works really well in general but I could break it by putting a / at the very start or end of the template, resulting in
QImageWriter cannot write image
Regarding the configuration dialog, I wonder whether it would make more sense to use the Location lineedit for choosing the subdirectories, as for Save As the directory part will be ignored anyway, resulting in the Filename field being incorrect. For example:
- Put the general template help text on top.
- Location lineedit and subdirectory help text below that.
- Filename lineedit at the bottom.
I also have a couple of suggestions for the wording and code organization below, but otherwise the code looks quite good already ;)
Tested for local and remote (Google Drive) save locations
How did you test saving to remote locations exactly, i.e. what URL did you put into the lineedit? I tried with fish://, but that fails even without your patch.
Please let us know your email address too (either here or by mai), so we'll be able to land the patch on your behalf eventually. BTW, did you choose the bug because it was marked as a junior job?
| src/ExportManager.cpp | ||
|---|---|---|
| 351 | Add const. | |
| 351–359 | I would move all of that inside of localSave. (Same applies for remoteSave, of course.) | |
| 354 | Add const. | |
| 357 | How about this: Cannot save screenshot, because creating the directory failed: \n %1 Reading https://api.kde.org/frameworks/ki18n/html/prg_guide.html, you might want to use this: emit errorMessage(xi18nc("@info", "Cannot save screenshot, because creating the directory failed:<nl/><filename>%1</filename>", dirPath.path())); | |
| 369 | Any reason you use saveLocation() here, while above you use dir? | |
| 372 | ditectory → remote directory (See also other comment.) | |
| src/Gui/SettingsDialog/SaveOptionsPage.cpp | ||
| 108–109 |
| |
Comment Actions
I also think that it would make more sense, but I tried it and there are some problems since the Location entry is not a lineedit but a KUrlRequester (% is replaced with %2525). Maybe I could add another lineedit or edit the Filename label to make it more descriptive.
I also have a couple of suggestions for the wording and code organization below, but otherwise the code looks quite good already ;)
Ok, I will fix the whole patch after we solve the configuration dialog issue.
Tested for local and remote (Google Drive) save locations
How did you test saving to remote locations exactly, i.e. what URL did you put into the lineedit? I tried with fish://, but that fails even without your patch.
I tested it with my Google Drive account, using gdrive:/marcoscarpetta02 as Location and %Y/%M/Screenshot_%T_%Y%M%D_%H%m%S as Filename.
Please let us know your email address too (either here or by mail), so we'll be able to land the patch on your behalf eventually.
marcoscarpetta02@gmail.com
BTW, did you choose the bug because it was marked as a junior job?
Yes
Comment Actions
Indeed, that's unfortunate. I thought about using different placeholders or a custom input widget, but in the end there would always be problems with directory URLs, as we cannot distinguish between placeholders and deliberate components of the URL (short of adding another level of escaping).
As a third line edit is probably more confusing than it helps, I guess the best option is to simply keep the dialog as-is. Conceptually this is then a kind of "extended" filename, but labelling it "filename" is probably still okay.
| src/ExportManager.cpp | ||
|---|---|---|
| 221–223 |
This solves the issue, but maybe there is a better solution I am not seeing. | |
| 369 | dirPath is the auto-generated directory, while saveLocation() is the base save location, already existing (https://api.kde.org/frameworks/kio/html/namespaceKIO.html#af51bf5661a028268d0f2ed83a5e5d774). | |
Comment Actions
Nice, we are nearly there.
Watchers of Spectacle: Any objections regarding the feature or the UI?
| src/ExportManager.cpp | ||
|---|---|---|
| 221–223 | You could use regular expressions, but I guess for the common case of no superfluous slashes that would be slower. Nevertheless, please adapt the coding style to use:
| |
| 369 | Ah, I see. This is probably a performance optimization, i.e. KIO could omit checking whether some parts of the remote path already exist. | |
| src/Gui/SettingsDialog/SaveOptionsPage.cpp | ||
| 108–109 |
Don't you think this explanation would be good to have? You could also put it after the example if you like that better. | |
Comment Actions
Very nice feature. In terms of the presentation, how about this for the new string?
"To save to a sub-folder, use slashes to describe the desired path, e.g.:"
| src/Gui/SettingsDialog/SaveOptionsPage.cpp | ||
|---|---|---|
| 108–109 | Yes, I forgot this last time.
This sounds good too. | |
| src/Gui/SettingsDialog/SaveOptionsPage.cpp | ||
|---|---|---|
| 108–109 | +1 | |
| src/ExportManager.cpp | ||
|---|---|---|
| 319 | No need for a comma here. | |
Comment Actions
Thanks again for your patch, I'll land it in a moment on your behalf.
Let us know if you need ideas for what to work on next if you like it here so far (IIRC there are even some junior jobs relating to the filename templates in Spectacle, or maybe you are interested in Gwenview…).
| src/ExportManager.cpp | ||
|---|---|---|
| 319 | Thanks, I'll fix it when committing. | |