Improved workflow (auto-create directories, quick save to sub directories, move/delete original file) #97

l3if3rik · 2024-06-28T17:34:24Z

That's basically all the stuff I developed in several branches on my fork. While your program is the best for my purposes, it lacked some features that I really needed.

Optionally auto-create missing directories
Optionally show a dialog that allows you to save into one of nine sub directories, just with a button click or key press. I missed that, because only after having selected a cut, I know, if it is some action scene, a dialog, whatever. You can guess from the screenshot that it is especially useful for game recordings.
Optionally delete or move the original file after the task was processed. If it is moved, the target can be an absolute directory or a sub directory of its original path.

I do not know how to work with translations, so it is all hard-coded English, currently.

To clarify this further, here are some screenshots:

…as saved

…erals

bartekmotyl · 2024-06-28T20:31:51Z

Thank you very much for submitting! I am currently a bit busy but will try to review as soon as possible.

bartekmotyl · 2024-06-30T12:23:31Z

I have take a first look at the changes and posted some comments (I reviewed only a subset of changes so far, but already wanted to give feedback). I like the ideas very much, but I would really prefer to have separate submission for each of the concepts (at least for auto-creating directories, quick-save folders, post-processing actions), that would really simplify the review and allow to integrate some changes whilst still discussing others.
The first two (auto-creating directories, quick-save folders) seem very useful and straightforward so I would like to integrate them quickly.
The third idea (post-processing actions) also looks very good, but there are some conceptual and architectural difficulties (see details in comments I posted). Moreover, currently (at least for me), SVC just crashes when trying to start cutting.

Anyway, thanks again for taking the effort and posting your changes!

bartekmotyl · 2024-06-30T10:34:30Z

src/SimpleVideoCutter/MainForm.cs

+                videoViewHover.MediaPlayer.Media?.Dispose();
+                videoViewHover.MediaPlayer.Media = null;
+            }
+        }


If such a generic method is added, it should allow to be called in isolation (i.e. without calling OpenNextFileInDirectory or OpenPrevFileInDirectory which btw. can always fail, as there may be just one file in the directory).

But it means, that UnloadVideo should properly update the whole state (including UI) after unloading the state.
So, we miss here, for example:

ClearAllSelections(); UpdateIndexLabel(); EnableButtons();

as well as clearing the fileBeingPlayed

Probably, UnloadVideo() can be removed completely, as OpenNextFileInDirectory() does all things necessary. I added the explicit unloading, because I once had a crash, and I noticed that OpenNextFileInDirectory() only implicitly unloaded the video. I didn't like that. Btw, setting fileBeingPlayed to null before calling OpenNextFileInDirectory() would require adjustments to that method. Let's save us the hassle.

I would love to keep the UnloadVideo, as it would also fix the issue #18. I can imagine that closing a file (to make sure some other program can have exclusive access) may be useful in some workflows.
I think the changes I suggested would be enough (haven't checked though).

btw, setting fileBeingPlayed to null before calling OpenNextFileInDirectory() would require adjustments to that method. Let's save us the hassle.

Yes, currently this would be an issue, but I think adding a lastFileThatWasPlayed variable would solve this issue nicely and should be rather straightforward,

UnloadVideo() is ready to be used now.

bartekmotyl · 2024-06-30T10:43:46Z

src/SimpleVideoCutter/MainForm.cs

@@ -30,6 +32,10 @@ public partial class MainForm : Form
        private string? fileToLoadOnStartup = null;
        private Debouncer debouncerHover = new Debouncer();
        private bool playingSelection = false;
+        private bool shouldAskForDeletionConfirmation = true;


Why not in VideoCutterSettings (as e.g. KeepSelectionAfterCut and others) ?

I like to be reminded once per session. Plus, if that flag was saved in the settings as false, we would need an extra checkbox somewhere to set it to true again. Then again, why not? Wouldn't be much work.

I see your point and of course this is your preference. On the other hand, I would rather like to prefer to keep the consistency and make all the options to work the same way. One could argue ("I like it to be session scope setting") for other variables as well, e.g. KeepSelectionAfterCut or ShowPreview. If the new settings would behave differently, probably there would be bug reports "Deletion confirmation not saved" etc.

In general, having possibility to consider all (or some) of the settings as "session based" sound interesting, but I would rather look for a more consistent solution (and I honestly think that in this case global-settings is a better choice).

Plus, if that flag was saved in the settings as false, we would need an extra checkbox somewhere to set it to true again.
Yes, that's true. But I would tend to ignore this. I rather assume that people do not revisit this kind of decisions (have you ever revisited cookie banner setting on a page that you clicked once? 😅 ). So I would assume that possibility to edit the json file (or just delete it and start from scratch) would be a "good enough" workaround.

bartekmotyl · 2024-06-30T10:43:51Z

src/SimpleVideoCutter/MainForm.cs

@@ -30,6 +32,10 @@ public partial class MainForm : Form
        private string? fileToLoadOnStartup = null;
        private Debouncer debouncerHover = new Debouncer();
        private bool playingSelection = false;
+        private bool shouldAskForDeletionConfirmation = true;
+        private bool shouldNotifyIfCurrentFileIsBeingDeleted = true;


Why not in VideoCutterSettings (as e.g. KeepSelectionAfterCut and others) ?

bartekmotyl · 2024-06-30T10:46:09Z

src/SimpleVideoCutter/MainForm.cs

@@ -726,6 +834,8 @@ private IList<string> GetVideoFilesInDirectory(string currentFilePath)

        private string? GetNextPrevFileInDirectory(string currentFilePath, int direction)
        {
+            Debug.WriteLine(currentFilePath);


I would rather prefer not to keep Debug-s in the code that is in development branch. Feel free to use them in the branch, but remove before opening PR (after testing).

I think I forgot that line there. Just like a surgeon forgets scissors in the stomach sometimes.

Sure, that's not problem. Thanks!

src/SimpleVideoCutter/MainForm.cs

bartekmotyl · 2024-06-30T11:33:13Z

src/SimpleVideoCutter/VideoCutterSettings.cs

@@ -13,6 +14,15 @@ public class VideoCutterSettings
        public string DefaultInitialDirectory { get; set; } = "{UserVideos}";
        public string OutputDirectory { get; set; } = "{UserVideos}";
        public string OutputFilePattern { get; set; } = "{FileDate}-{FileNameWithoutExtension}.{Timestamp}{FileExtension}";
+        public string[] QuickSubDirectories { get; set; } = Enumerable.Repeat("", 9).ToArray();
+
+        public bool CreateMissingDirectories { get; set; } = false;


I would opt for true being the default. This seems to very useful and I do not think it would break any workflows of existing users.

bartekmotyl · 2024-06-30T11:35:47Z

src/SimpleVideoCutter/MainForm.cs

+                    confirmDeletionResult = MessageBox.Show(
+                        "As per the settings, the original file will be deleted. Is this OK? If you choose yes, original files will be deleted without another confirmation.",
+                        "Confirm Deletion",
+                        MessageBoxButtons.YesNo


I would consider adding also a "cancel" option, that would mean the action is not constructed and the file is not being deleted.

src/SimpleVideoCutter/MainForm.cs

bartekmotyl · 2024-06-30T11:53:46Z

src/SimpleVideoCutter/MainForm.cs

+                if (this.shouldNotifyIfCurrentFileIsBeingDeleted)
+                {
+                    DialogResult dialogResult = MessageBox.Show("As per the settings, the original file will be deleted after the cut was saved." +
+                        " Since you are currently playing this file, the program will skip to the next file first." +


As a user, I would probably prefer to only unload the current file, without proceeding to the next one automatically (why next? I usually browse file the the reverse date order so I go to previous one). Or at least do not start playing the next file automatically.

I recently cut about 30 files and enjoyed the jump to the next (I didn't care about the order, could also have been previous) file. Hm, it should be configurable.

Yeah, in the end this is really a detail (whether to unload only or load another file immediately, next or prev). If there is a separate option to "close" file then I think it is not that important.
On the other hand, it can always happen that there is no more files in the folder, thus we end up with "no file loaded" sitaution and this has to be supported.

bartekmotyl · 2024-06-30T11:59:35Z

src/SimpleVideoCutter/TaskProcessor.cs

@@ -189,6 +197,7 @@ public class FFmpegTask
        public FFmpegTaskSelection[]? Selections { get; set; }   
        public long OverallDuration { get; set; }
        public bool Lossless { get; set; }
+        public IActionAfterTaskCompletion? ActionAfterTaskCompletion { get; set; }


Have you tested this?
Currently it fails with exception, because the task is being XML-serialized (cloned) in the FormAddTask constructor (I had the keep option selected)

System.InvalidOperationException HResult=0x80131509 Message=There was an error reflecting type 'SimpleVideoCutter.FFmpegTask'. Source=System.Private.Xml StackTrace: at System.Xml.Serialization.XmlReflectionImporter.ImportTypeMapping(TypeModel model, String ns, ImportContext context, String dataType, XmlAttributes a, Boolean repeats, Boolean openModel, RecursionLimiter limiter) at System.Xml.Serialization.XmlReflectionImporter.ImportElement(TypeModel model, XmlRootAttribute root, String defaultNamespace, RecursionLimiter limiter) at System.Xml.Serialization.XmlReflectionImporter.ImportTypeMapping(Type type, XmlRootAttribute root, String defaultNamespace) at System.Xml.Serialization.XmlSerializer..ctor(Type type, String defaultNamespace) at SimpleVideoCutter.Utils.DeepCloneXML[T](T input) in C:\Users\bartek\Documents\git\simple-video-cutter-mr\src\SimpleVideoCutter\Utils.cs:line 23 at SimpleVideoCutter.FormAddTask..ctor(FFmpegTask task, Boolean selectionsOnKeyFrames) in C:\Users\bartek\Documents\git\simple-video-cutter-mr\src\SimpleVideoCutter\FormAddTask.cs:line 19 at SimpleVideoCutter.MainForm.PrepareTask(Boolean showAddTaskDialog) in C:\Users\bartek\Documents\git\simple-video-cutter-mr\src\SimpleVideoCutter\MainForm.cs:line 745 at SimpleVideoCutter.MainForm.EnqeueNewTask(Boolean showAddTaskDialog) in C:\Users\bartek\Documents\git\simple-video-cutter-mr\src\SimpleVideoCutter\MainForm.cs:line 782 at SimpleVideoCutter.MainForm.toolStripSelection_ItemClicked(Object sender, ToolStripItemClickedEventArgs e) in C:\Users\bartek\Documents\git\simple-video-cutter-mr\src\SimpleVideoCutter\MainForm.cs:line 1132 at System.Windows.Forms.ToolStrip.HandleItemClick(ToolStripItem dismissingItem) at System.Windows.Forms.ToolStripItem.HandleClick(EventArgs e) at System.Windows.Forms.ToolStripItem.HandleMouseUp(MouseEventArgs e) at System.Windows.Forms.ToolStrip.OnMouseUp(MouseEventArgs mea) at System.Windows.Forms.Control.WmMouseUp(Message& m, MouseButtons button, Int32 clicks) at System.Windows.Forms.Control.WndProc(Message& m) at System.Windows.Forms.ScrollableControl.WndProc(Message& m) at System.Windows.Forms.ToolStrip.WndProc(Message& m) at System.Windows.Forms.Control.ControlNativeWindow.WndProc(Message& m) at System.Windows.Forms.NativeWindow.Callback(IntPtr hWnd, WM msg, IntPtr wparam, IntPtr lparam) Inner Exception 1: InvalidOperationException: Cannot serialize member 'SimpleVideoCutter.FFmpegTask.ActionAfterTaskCompletion' of type 'SimpleVideoCutter.Actions.IActionAfterTaskCompletion', see inner exception for more details. Inner Exception 2: NotSupportedException: Cannot serialize member SimpleVideoCutter.FFmpegTask.ActionAfterTaskCompletion of type SimpleVideoCutter.Actions.IActionAfterTaskCompletion because it is an interface.

I would consider saving (into the task) only the name of the selected action, and actually instantiate it when it will be about to execute.

Oh, I never used Shift + E during later development, so I never stumbled upon that. Sorry.

This is actually pretty strange, because I think it worked me for the first time I had started the app in your branch (i.e. I was able to cut) but then it failed every time. And btw. I did not use Shift+E, I just pressed the Cut button.

l3if3rik · 2024-06-30T17:14:53Z

Thanks for taking the time to review all that! And sorry for the PR containing so much stuff. Initially, I was just tinkering around (it's been at least 10 years since I developed with .NET) and didn't even think of making a PR.

l3if3rik · 2024-06-30T17:16:52Z

I will probably address the issues next weekend.

bartekmotyl · 2024-06-30T17:39:47Z

src/SimpleVideoCutter/ChooseOutputDirectoryDialog.cs

+
+namespace SimpleVideoCutter
+{
+    public partial class ChooseOutputDirectory : Form


(I report it here, but this comment applies also to other places)
I have recently cleaned up development branch from any compiler warnings. In your branch I see 133 warnings. For example here:

Severity Code Description Project File Line Suppression State Warning (active) CS8618 Non-nullable field 'originalOutputDir' must contain a non-null value when exiting constructor. Consider declaring the field as nullable. SimpleVideoCutter ChooseOutputDirectoryDialog.cs 24

I cleaned them up in my fork. Though I did not count them, I made much less fixes than 133 (probably between 10 and 20).

The only messages I see now are related to duplicate names in FormSettings.resx, and there are 118 of them. Could they have been introduced by my IDE by accident? I did not consciously edit that file.

Excerpt:

1>FormSettings.resx : warning MSB3568: Duplicate resource name ">>buttonCancel.Name" is not allowed, ignored. 1>FormSettings.resx : warning MSB3568: Duplicate resource name ">>buttonCancel.Type" is not allowed, ignored. 1>FormSettings.resx : warning MSB3568: Duplicate resource name ">>buttonCancel.Parent" is not allowed, ignored. 1>FormSettings.resx : warning MSB3568: Duplicate resource name ">>buttonCancel.ZOrder" is not allowed, ignored. ...

It turned out that those resource names were there before my changes. Can I safely delete them?

l3if3rik · 2024-07-16T18:36:31Z

I would consider adding also a "cancel" option, that would mean the action is not constructed and the file is not being deleted.

Except for this, everything should be resolved now.

UnloadVideo() can be used now with an unload/close button that would have to be added.

bartekmotyl · 2024-07-24T20:39:29Z

Hi @l3if3rik, just a short update from my side - I have seen you pushed lot of changes, but unfortunately I am very busy recently (plus vacation season) so had no time yet to review, and may still need some time to do it. Sorry for the delay and thanks for you effort!

l3if3rik · 2024-07-25T12:44:54Z

simple-video-cutter/src/SimpleVideoCutter/MainForm.cs

Line 775 in 2618a7c

    
           int index = videoFilesArr.IndexOf(Path.GetFileName(fileBeingPlayed).ToLowerInvariant());

This bug flew under the radar for quite some time. The files in videoFileArr have their native mixed case names. The filename being searched is lowercase. I fixed that by doing a case-insensitive search like in GetNextPrevFileInDirectory().

A method like GetCurrentFileIndex() would remove redundancy. It would not have a filename parameter, but use fileBeingPlayed. It would fit in, if GetNextPrevFileInDirectory() also dropped its argument currentFilePath (it's not needed right now).

l3if3rik added 10 commits June 16, 2024 16:32

Add dialog to save in a specific subdirectory by pressing a digit key

d06ecce

Autosize quick subdirectory form, disable manual resizing

a364648

Re-add event handlers, rename methods (convention)

12ca1a2

Use KeyPreview, improve Layout

b39b9f4

Change text box background color, change explanation; refactor

db806a6

Add functionality to delete or move the original file after the cut w…

723456d

…as saved

Assign ActionName to radio buttons tag to reduce redundant string lit…

a7bd4c3

…erals

Change wording

a2e559a

Add option to create missing directories automatically

fdc4fa2

Add buttons to quick sub directory dialog; allow escape key to close

6222835

bartekmotyl reviewed Jun 30, 2024

View reviewed changes

l3if3rik added 14 commits July 2, 2024 18:20

Use abstract class for actions; allows serialization

be1402c

Use "new" keyword

f77600e

Follow naming conventions

a8d5936

Make task readonly

bff0079

Handle potential null references

5c27b13

Prevent PlaceholderFiller being null

8790f6c

Remove debug logging

8feda58

Indicate Windows support

88d6b2f

Declare event handler as nullable

f1ebb97

Adjust wording/variable name

c006500

Use text box instead of combo box

b8e374b

Remove obsolete property

577bc6c

Adjust control name

d9efacb

Move PlaceholderFiller to MainForm

0824ea9

l3if3rik added 10 commits July 11, 2024 07:06

Save in settings whether there should be a deletion confirmation

e610412

After action executed, do not open next file

6c2f4d6

Update GUI after unloading video

673ac10

Rename "EnableButtons" to "UpdateButtonStates"

6390664

Fix message

6598cd0

Remove redundant fallback

23027c9

Refactor action system

af6f4c2

Adjust code to conventions

40bf362

Store settings when leaving edit mode

f2a6958

Fix button states after unloading/opening video

372aac0

l3if3rik added 3 commits July 25, 2024 07:43

Reset file info labels after unloading file

4ffd7e5

Fix displayed index of current file

2416a1c

Determine file index case-insensitively

1efa3e2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved workflow (auto-create directories, quick save to sub directories, move/delete original file) #97

Improved workflow (auto-create directories, quick save to sub directories, move/delete original file) #97

l3if3rik commented Jun 28, 2024

bartekmotyl commented Jun 28, 2024

bartekmotyl commented Jun 30, 2024

bartekmotyl Jun 30, 2024

l3if3rik Jun 30, 2024

bartekmotyl Jun 30, 2024

l3if3rik Jul 16, 2024

bartekmotyl Jun 30, 2024

l3if3rik Jun 30, 2024

bartekmotyl Jun 30, 2024

bartekmotyl Jun 30, 2024

l3if3rik Jun 30, 2024

bartekmotyl Jun 30, 2024

bartekmotyl Jun 30, 2024

l3if3rik Jun 30, 2024

bartekmotyl Jun 30, 2024

bartekmotyl Jun 30, 2024

bartekmotyl Jun 30, 2024

l3if3rik Jun 30, 2024

bartekmotyl Jun 30, 2024

l3if3rik Jun 30, 2024

bartekmotyl Jun 30, 2024

bartekmotyl Jun 30, 2024

l3if3rik Jun 30, 2024

bartekmotyl Jun 30, 2024 •

edited

Loading

l3if3rik commented Jun 30, 2024

l3if3rik commented Jun 30, 2024

bartekmotyl Jun 30, 2024

l3if3rik Jul 3, 2024

l3if3rik Jul 13, 2024

l3if3rik commented Jul 16, 2024

bartekmotyl commented Jul 24, 2024

l3if3rik commented Jul 25, 2024

Improved workflow (auto-create directories, quick save to sub directories, move/delete original file) #97

Are you sure you want to change the base?

Improved workflow (auto-create directories, quick save to sub directories, move/delete original file) #97

Conversation

l3if3rik commented Jun 28, 2024

bartekmotyl commented Jun 28, 2024

bartekmotyl commented Jun 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartekmotyl Jun 30, 2024 • edited Loading

Choose a reason for hiding this comment

l3if3rik commented Jun 30, 2024

l3if3rik commented Jun 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l3if3rik commented Jul 16, 2024

bartekmotyl commented Jul 24, 2024

l3if3rik commented Jul 25, 2024

bartekmotyl Jun 30, 2024 •

edited

Loading