Differential D10709

Add optional window title to filename templates
ClosedPublic
Actions

Authored by rominf on Feb 21 2018, 12:15 PM.

Details

Reviewers
rkflx
Group Reviewers
Spectacle
Maniphest Tasks
T8036: Use window title to suggest file names
Commits
R166:d47ab66eb9a2: Add optional window title to filename templates
Summary

Add '%T' placeholder for filename pattern to insert window title.

Example filenames for pattern %Y-%M-%D %H-%m-%S. %T:
2018-02-21 19-11-55. romas : byobu.png
2018-02-21 19-12-20.png

Example filenames for pattern %T:
romas : byobu.png
Screenshot.png

Closes T8036
FEATURE: 378463
FIXED-IN: 18.04.0

Test Plan

Steps:

  1. Go to Configure... -> Save.
  2. Enter to Default Save Filename -> Filename tempate %Y-%M-%D %H-%m-%S. %T.
  3. Press OK.
  4. Save screenshot.
  5. Observe that screenshot filename matches pattern.

Examples of filenames with empty window title:

InputResult
Screenshot_%T_ProjectXScreenshot_ProjectX
%T_ProjectXProjectX
Screenshot_%TScreenshot
%TScreenshot

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
rkflx added a comment.Feb 24 2018, 11:49 AM
In D10709#212545, @rominf wrote:

It doesn't work with %T -> .png too.

Why not?

rominf added a comment.Feb 24 2018, 12:08 PM
In D10709#212557, @rkflx wrote:
In D10709#212545, @rominf wrote:

It doesn't work with %T -> .png too.

Why not?

Because empty (except extension) filename looks strange. Also, I wonder how this will be displayed in file managers that are configured to hide extensions. File with an empty name?

rkflx added a comment.EditedFeb 24 2018, 3:29 PM
In D10709#212558, @rominf wrote:
In D10709#212557, @rkflx wrote:
In D10709#212545, @rominf wrote:

It doesn't work with %T -> .png too.

Why not?

Because empty (except extension) filename looks strange. Also, I wonder how this will be displayed in file managers that are configured to hide extensions. File with an empty name?

Of course it would work without annoying everyone with Screenshot in most of their filenames. Taking your example:

%T → Screenshot.png
%Y-%M-%D_%H-%m-%S_%T → 2018-02-21_19-10-42.png

Edit: Don't forget that users could already start their filename template with Screenshot_, so duplicating this would look quite odd.

rominf updated this revision to Diff 27974.Feb 25 2018, 1:06 AM

Remove separators if there is no window title

rominf edited the summary of this revision. (Show Details)Feb 25 2018, 1:08 AM
In D10709#212677, @rkflx wrote:
In D10709#212558, @rominf wrote:
In D10709#212557, @rkflx wrote:
In D10709#212545, @rominf wrote:

It doesn't work with %T -> .png too.

Why not?

Because empty (except extension) filename looks strange. Also, I wonder how this will be displayed in file managers that are configured to hide extensions. File with an empty name?

Of course it would work without annoying everyone with Screenshot in most of their filenames. Taking your example:

%T → Screenshot.png
%Y-%M-%D_%H-%m-%S_%T → 2018-02-21_19-10-42.png

Edit: Don't forget that users could already start their filename template with Screenshot_, so duplicating this would look quite odd.

Done. Please check it. Do you like it now? ;)

rominf marked 2 inline comments as done.Feb 25 2018, 1:13 AM
rominf updated this revision to Diff 27981.Feb 25 2018, 5:45 AM

Replace QStringLiteral with isEmpty()

rkflx added a comment.Feb 25 2018, 7:42 AM
In D10709#213137, @rominf wrote:

Done. Please check it. Do you like it now? ;)

Could not test yet, but your description sounds lovely ;) My next steps:

  • test edge cases, try to break it
  • final code review

Note this may take me a while, because there are a couple of other review requests in front of yours in the queue…

ngraham added a comment.Feb 26 2018, 1:28 AM

Here's one issue: the explanatory text now exceeds the bounds of the group box in Spectacle's settings:

Also, if we don't add the new %T tag by default, 99.9% of spectacle's users will never see or benefit from it.

rominf added a comment.Feb 26 2018, 9:16 AM
In D10709#214055, @ngraham wrote:

Here's one issue: the explanatory text now exceeds the bounds of the group box in Spectacle's settings:

Strange. Did you apply resize(500, 500);. Because it looks like you didn't.

Also, if we don't add the new %T tag by default, 99.9% of spectacle's users will never see or benefit from it.

Agreed. I propose to use almost ISO 8601 (YYYYMMDD hhmmss) + %Y%M%D %H%m%S. %T. Spaces will be in the filename if we use %T. I don't see why we should be afraid of them.

rkflx added a comment.Feb 26 2018, 9:18 AM

I think we already established to discuss the topic of the default filename in a separate task? Let's not overload this patch.

@ngraham Did you read the complete discussion before commenting?

rkflx added a comment.Feb 26 2018, 10:07 AM
In D10804#214243, @rominf wrote:

F5730612: %T.png

Was %T.png intentional, or did you trigger a bug?

rominf added a comment.Feb 26 2018, 10:12 AM
In D10709#214246, @rkflx wrote:
In D10804#214243, @rominf wrote:

F5730612: %T.png

Was %T.png intentional, or did you trigger a bug?

I have %T in my system settings. It works fine with the debug version of Spectacle. I just ran stable Spectacle from my OS that still doesn't know about this placeholder.

ngraham added a comment.Feb 26 2018, 2:03 PM
In D10709#214223, @rkflx wrote:

I think we already established to discuss the topic of the default filename in a separate task? Let's not overload this patch.

@ngraham Did you read the complete discussion before commenting?

Sorry, my bad. I did read the complete discussion, but it was a day or two ago and I must admit I forgot this part of it! So many patches in flight these days... A good problem to have, really.

ngraham added a comment.Feb 26 2018, 2:04 PM
In D10709#214222, @rominf wrote:

Strange. Did you apply resize(500, 500);. Because it looks like you didn't.

I used arc patch, and I noticed that arc complained that this patch based on a commit not in the working copy. Could you re-base from master? Maybe that will fix it.

rominf updated this revision to Diff 28124.Feb 26 2018, 3:36 PM

Rebase on master

rominf added a comment.Feb 26 2018, 3:36 PM
In D10709#214388, @ngraham wrote:
In D10709#214222, @rominf wrote:

Strange. Did you apply resize(500, 500);. Because it looks like you didn't.

I used arc patch, and I noticed that arc complained that this patch based on a commit not in the working copy. Could you re-base from master? Maybe that will fix it.

Done

rominf added a comment.Mar 2 2018, 1:48 PM

@rkflx Ping. What do you think?

rkflx added a comment.Mar 2 2018, 1:49 PM

I'm on it. Code review takes much more time than commenting on UI design ;)

rkflx requested changes to this revision.EditedMar 4 2018, 7:16 AM
rkflx retitled this revision from Save windows screenshots with filename containing title to Add optional window title to filename templates.
rkflx edited the summary of this revision. (Show Details)
rkflx added a task: T8036: Use window title to suggest file names.

Works in most cases, but I think there are still some things which could be improved:

In D10709#212395, @rkflx wrote:

Most users will never go to this dialog, some users will use characters like .,:/_ etc. between other templates/custom text and the %T placeholder, and a few select will do something weird. Therefore most cases can be dealt with by simply removing those special characters before and after %T in case the window title is empty.

Are you sure your RegEx is working correctly for the above? You are matching only in one direction. Also, matching only a single character from a selected set would probably be better than matching everything until %.

InputResultExpected Result
Screenshot_%T_ProjectXScreenshot_Screenshot_ProjectX
%T_ProjectXProjectX
Screenshot_%TScreenshot_Screenshot

Should also work for .,:- and .

In D10709#211384, @rominf wrote:
In D10709#211093, @rkflx wrote:

Could you check this? We should test whether an emoji set in a webpage's title can be in the filename, also I could imagine that there are length restrictions. I'm not saying we should handle this ourselves, but we need to make sure that e.g. Qt is doing it for us.

According to https://stackoverflow.com/a/3085216/2108548 Qt doesn't handle filename restrictions. I searched for the good solution for this. It seems that the only real solution is to use boost. It's not an option, I suppose. I asked a question on IRC, nobody replied.

My research led to different conclusions:

  • Most filesystems relevant for us allow up to 255 bytes per filename.
  • There is no restriction in the character set, but in practice / won't work.
  • Spectacle already shows two error dialogs in sequence if the filename is too long or contains illegal characters.
  • For hotkey-triggered captures we probably don't want error dialogs, but an automatic fallback.

As web browsers in particular can contain quite long titles, we should handle that case. Looking at Chromium, when saving a file a similar restriction is enforced, i.e. the last parts are cut off.

Chromium and Firefox also replace / with _.

Could you implement that and update your test plan regarding edge cases too?

doc/index.docbook
314

Window's → Window

src/ExportManager.cpp
22–29

Unrelated change.

196–222

Why do you need = QStringLiteral();?

203

const

203–204

Add comment about what the RegEx is for.

215–216

{}

src/Gui/SettingsDialog/SaveOptionsPage.cpp
107

Window's → Window

src/PlatformBackends/ImageGrabber.h
97

Why do you need this?

src/PlatformBackends/X11ImageGrabber.cpp
556

getRealWindowUnderCursor() might be called again later on, please cache it.

src/PlatformBackends/X11ImageGrabber.h
90

Did you look at whether it would be possible to get the window title on Wayland too? Is there a platform-independent way to get at the window title?

This revision now requires changes to proceed.Mar 4 2018, 7:17 AM
rominf updated this revision to Diff 28602.Mar 4 2018, 3:29 PM
rominf marked 4 inline comments as done.
  • React to comments
  • Better way to remove '%T' with separators around it
rominf updated this revision to Diff 28609.Mar 4 2018, 4:55 PM
  • Truncate long filenames, replace "/" with "_"
rominf edited the test plan for this revision. (Show Details)Mar 4 2018, 5:00 PM
rominf marked 4 inline comments as done.
rominf added inline comments.
src/ExportManager.cpp
22–29

@rkflx, how should I fix unsorted #includes? If I do it with my normal changes combined, you are unsatisfied: "unrelated changes". Can I fix #includes in all files of Dolphin in a separate diff?

rominf updated this revision to Diff 28612.Mar 4 2018, 5:25 PM

Rebase on master

rominf updated this revision to Diff 28613.Mar 4 2018, 5:28 PM

Fix docbook

rominf updated this revision to Diff 28615.Mar 4 2018, 5:44 PM

Cleanup

rkflx requested changes to this revision.Mar 6 2018, 12:20 AM

Thanks for the updates. Small hint: Every time you use arc diff you are "requesting review". If you are not done yet, it would be better to either batch all changes together or to add --plan-changes.

In D10709#218290, @rominf wrote:
  • Better way to remove '%T' with separators around it

Works great now.

In D10709#218358, @rominf wrote:
  • Truncate long filenames

Did you even test this? I can break it in the most obvious way: Open window with title longer than the allowed length, try to save screenshot, observe error message.

src/ExportManager.cpp
22–29

Reordering includes creates only very little value, but costs reviewer time, clutters the git history and will be imperfect again in no time. Nevertheless, removing unused includes across the code base has its value, but it would need to go into a separate Diff anyway.

196–222

I still don't understand why a simple QString title; would not work.

204

Nice!

src/PlatformBackends/X11ImageGrabber.cpp
556

Moving the function call up will only work if the cache variable is not changed in-between. You should codify that assumption by making it const.

This revision now requires changes to proceed.Mar 6 2018, 12:20 AM
rominf marked 6 inline comments as done.Mar 6 2018, 11:02 AM
rominf updated this revision to Diff 28807.Mar 6 2018, 11:03 AM

Truncate long filenames done right

rominf added inline comments.Mar 6 2018, 11:07 AM
src/PlatformBackends/X11ImageGrabber.h
90

I don't have a Wayland to test on, sorry. Hence, I'll leave this for another contributor.

rominf updated this revision to Diff 28812.Mar 6 2018, 11:13 AM

Fix compilation

ngraham added a comment.Mar 6 2018, 12:03 PM
In D10709#214536, @rominf wrote:
In D10709#214388, @ngraham wrote:
In D10709#214222, @rominf wrote:

Strange. Did you apply resize(500, 500);. Because it looks like you didn't.

I used arc patch, and I noticed that arc complained that this patch based on a commit not in the working copy. Could you re-base from master? Maybe that will fix it.

Done

This problem remains for me.

rkflx added a comment.Mar 6 2018, 12:07 PM
In D10709#219864, @ngraham wrote:
In D10709#214388, @ngraham wrote:

I used arc patch, and I noticed that arc complained that this patch based on a commit not in the working copy. Could you re-base from master? Maybe that will fix it.

This problem remains for me.

Hmh, works for me. Try a fresh checkout.

ngraham added a comment.Mar 6 2018, 12:10 PM

Ah, found the issue: I've increased the "general" font size by 1 px. Seems that the hinting is sort of broken here with elevated font sizes.

src/PlatformBackends/X11ImageGrabber.h
90

Just install the Wayland package (plasma-workspace-wayland on Ubuntu-based systems) and give it a shot.

rkflx requested changes to this revision.Mar 6 2018, 12:17 PM
In D10709#219899, @ngraham wrote:

Ah, found the issue: I've increased the "general" font size by 1 px. Seems that the hinting is sort of broken here with elevated font sizes.

Ah, I thought your comment was about arc patch, given the quoting…

Just checked this by increasing all font sizes by 1pt, you are right (apart from blaming it on the hinting): All items should be visible regardless of font size, i.e. the minimum size should adapt to the content.

@rominf Please fix, preferably without hardcoding sizes.

This revision now requires changes to proceed.Mar 6 2018, 12:17 PM
rkflx added a comment.Mar 6 2018, 3:58 PM
In D10709#218358, @rominf wrote:
  • Truncate long filenames

Thanks, works now.

src/ExportManager.cpp
228–230

QLatin1Literal

src/PlatformBackends/X11ImageGrabber.cpp
433–434

Please don't change unrelated code, because it makes the Git history unreadable.

Only for the parts you are changing anyway you can perform small cleanups.

rominf marked 2 inline comments as done.Mar 6 2018, 4:05 PM
rkflx added a comment.Mar 6 2018, 4:10 PM

@rominf marked 2 inline comments as done.

As far as I can see they are not done?

rominf marked an inline comment as not done.Mar 6 2018, 4:15 PM
rominf added inline comments.
src/PlatformBackends/X11ImageGrabber.cpp
433–434

Should I revert this? What should I do with small fixes like this? Open lots of separate diff for a few lines of code or just don't cleanup the code that works?

rominf updated this revision to Diff 28848.Mar 6 2018, 4:28 PM
rominf marked an inline comment as not done.

Fit filename format help into small window

rkflx added inline comments.Mar 6 2018, 4:29 PM
src/PlatformBackends/X11ImageGrabber.cpp
433–434

Should I revert this?

Please do.

What should I do with small fixes like this?

IMO it's not really worthwhile spending much effort on this. Either do larger cleanups, or change things if you are touching parts of that code anyway.

rominf updated this revision to Diff 28849.Mar 6 2018, 4:40 PM

Revert unrelated changes

rominf marked 3 inline comments as done.Mar 6 2018, 4:40 PM
ngraham added a comment.Mar 6 2018, 4:41 PM

There we go, now the text all fits!

rkflx accepted this revision.Mar 6 2018, 4:42 PM

Thanks Roman. Finally we are good to go!

In D10709#220138, @ngraham wrote:

There we go, now the text all fits!

Yup.

This revision is now accepted and ready to land.Mar 6 2018, 4:42 PM
ngraham added a comment.Mar 6 2018, 4:43 PM

+1, ship it!

Closed by commit R166:d47ab66eb9a2: Add optional window title to filename templates (authored by rominf). · Explain WhyMar 6 2018, 4:44 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 28850

View Options

doc/index.docbook

Loading...
View Options

src/ExportManager.h

Loading...
View Options

src/ExportManager.cpp

Loading...
View Options

src/Gui/SettingsDialog/SaveOptionsPage.cpp

Loading...
View Options

src/PlatformBackends/ImageGrabber.h

Loading...
View Options

src/PlatformBackends/X11ImageGrabber.h

Loading...
View Options

src/PlatformBackends/X11ImageGrabber.cpp

Loading...
View Options

src/SpectacleCore.cpp

Loading...