Add setting for custom filename on save image #187
Conversation
src/mpc-hc/MainFrm.cpp
Outdated
| } else if (GetPlaybackMode() == PM_DVD) { | ||
| prefix.Format(_T("dvd_snapshot_%s"), GetVidPos()); | ||
| snapshotName = MakeSnapshotFileName(GetFileName(), GetVidPos()); |
There was a problem hiding this comment.
I don't think that GetFileName() is valid for DVDs.
src/mpc-hc/MainFrm.cpp
Outdated
| @@ -4880,15 +4954,15 @@ void CMainFrame::OnFileSaveImage() | |||
|
|
|||
| CPath psrc(s.strSnapshotPath); | |||
|
|
|||
| CStringW prefix = _T("snapshot"); | |||
| CString snapshotName = _T("snapshot"); | |||
There was a problem hiding this comment.
This assignment can be move to end of if/else for unhandled GetPlaybackMode() values.
src/mpc-hc/MainFrm.cpp
Outdated
| @@ -4948,17 +5022,17 @@ void CMainFrame::OnFileSaveImageAuto() | |||
| return; | |||
| } | |||
|
|
|||
| CStringW prefix = _T("snapshot"); | |||
| CString snapshotName = _T("snapshot"); | |||
There was a problem hiding this comment.
This code block is similar as the other one. Could be refactored or moved into MakeSnapshotFileName().
src/mpc-hc/mpc-hc.rc
Outdated
| "Use legacy toolbar instead of new vectorized one." | ||
| IDS_SUBMENU_COPYURL "Copy URL" | ||
| IDS_PPAGEADVANCED_SNAPSHOT_NAME_FORMAT | ||
| "Filename format used for snapshots on ""Save Image"". %f is the video filename, %t is the current video position, %Y, %m, %d is the current year, month, day, etc." |
There was a problem hiding this comment.
"Filename template that is used for"
For example: "%f_snapshot_%t_[%Y.%m.%d_%H.%M.%S]", where %f equals video file name, %t current video timestamp, %Y current year, etc.
…his allows excluding timestamp and position. ToDo: translations
|
Thanks for patch, but I don't think this is best approach. 95% of people will not understand what to type there and how this work. And it is not good to give user full control over format string. I would prefer few preset that every user can understand and select best one. Similar to what @clsid2 done in his patch. But I'm open for discussion here. |
|
Is the main concern the possibility of a security vulnerability? I believe it is better to give users the option of having a custom format string, since power users will most likely be the ones using this feature. FWIW, VLC is doing the same thing: https://wiki.videolan.org/Documentation:Play_HowTo/Format_String/ |
|
This feature should also be useful for less experienced PC users. So perhaps my simpler approach would indeed be better. Which custom string would you personally use if your patch would be used? |
|
I don't think less experienced users would use this feature. The users who want to change the filename prefer full control over it, like the person who created the original Trac ticket for this feature: https://trac.mpc-hc.org/ticket/4241 If less experienced users that are interested in this feature do exist, I think they will be ok as long as we provide sufficient documentation. It seems to be working for VLC. Personally I prefer the video time at the beginning, so I would use |
This adds an option in advanced settings to allow users to customize the default filename used for snapshots when using File > Save Image (or pressing F5). Currently snapshot filenames are hardcoded like
video.mkv_snapshot_01.12_[2017.08.01_16.59.08]. This option lets users choose their own filename format string, defaulted to%f_snapshot_%t_[%Y.%m.%d_%H.%M.%S]to copy current behavior.Trac: https://trac.mpc-hc.org/ticket/4241