Video-hub-app: Allow sorting by creation time

[CrendKing]

Currently the app only allows sorting items by the modified time. Created time is another commonly used property in most file systems. Users may find it useful to sort the library.

Currently the mtime property is indexed in the .vha2 file. If we'd introduce the new ctime, user will have to re-index the library. Fortunately, the thumbnails and filmstrip can be reused.

Also, for folders, the current implementation for modified time is to use the last updated time of all its content items. For created time, I suggest to use the earliest created time of all its content items. However, the more interesting question is, why bother to traverse the folder content instead of directly using the folder's file system mtime and ctime properties?

I have an implementation myself locally (sans translations). I'm not sure if you accept pull request. If not, can you consider implementing this?

[cal2195]

Pull request are always welcome! 😄

Please make one, even without translations! 👍

[CrendKing]

Here you are: https://github.com/whyboris/Video-Hub-App/pull/455

I did with minimum changes. Still, for folders why not just use the folder's mtime and ctime, instead of traversing content in FolderViewPipe::transform()?

[whyboris]

Thank you for the PR, the code looks good! I'll test it out and merge shortly.

I hope I understand your suggestion / question about folder's _mtime_ and _ctime_ -- at the moment the app does not include any information about a folder, only about videos. So there's no place to affix the folder's _mtime_ or _ctime_ to.

Let me know if I've misunderstood your question.

[CrendKing]

My suggestion is, when sorting folders, instead of using its contained video's times, query filesystem for that folder's real mtime and ctime. You don't need to index folders. Just do a real time fs.statSync() on that folder.

Of course the rules of how a directory's mtime and ctime update depends on the underlying file system. But I think this app do not need to concern too much of those details regarding usability. Performance wise, one system call could be way faster than traversing large number of indices.

[whyboris]

I'm afraid that I'd have to implement a very different system if I start including information about the _folders_. As the app is designed (for better or worse), it only watches for information about individual _files_. While what you propose is doable, and perhaps more efficient for some sub-function of the app, I'm unsure the added complexity is worth it at this time.

I'm currently working on allowing the app to add more than one root folder per hub 🎉 #443 -- it's a major-enough rewrite of a lot of code that I'd rather not add anything new besides that this year.

Thank you again for the PR! Looking forward to merging it 😁

[whyboris]

Thank you again @CrendKing for adding this feature 🙇 #455 🎉 🚀 😁

[CrendKing]

No problem. I have several other local changes that suits my own taste. I'm not sure if you guys want any of them.

[whyboris]

Hey @CrendKing -- it's just me over here 😊 and other improvements are welcome.

Could you comment with a short description of some improvements you've made for your own taste? 🙂

[CrendKing]

1. Properly save the window state when close. Currently the "maximized" state is not saved, only the dimension. Which means if I maximize the window and close the app, the next time it's launched, the window seems at the same size but moveable. I found https://github.com/mawie81/electron-window-state the easiest way to solve the problem.

2. In Windows, the close button of a maximize window should always function if the mouse is moved to the top-right corner. This is a main usability and accessibility feature. However, because the app's title-bar component is customized with CSS, currently user needs to move 7 pixels to the left to click the button. Given how small the button is, this could lead to issue. I adjust CSS so that instead of doing right: 7px, use right: 0; padding-right: 7px. Same applies to the top property of all 3 buttons.

3. The "Default" sort order is basically the order when each file is indexed, which has little use to users. I would prefer to use more visible attributes such as their filename or creation time. Unfortunately, the sort order is not saved across restarts. I made it to remember.

EDIT: I read the code a bit more. It seems I was wrong that the "Default" is basically the same as the "currentSort" from settings.json. So the sort order is persisted, which is good. But I think it is better to instead of always show "Default", show the actual sort order in the dropdown. It is much less confusing and user don't need to remember what's the order they last used.

1. Allow user to specify launch arguments of the video player.

[whyboris]

I love all these! If you have the patience to create a PR for each or all at the same time (whichever is easiest) I'll accept all 👍 🙇

Re (3) "Default" sort order -- the original intention (and as far as I remember, the current implementation) is the order you'd see if you traversed your directory by opening the top-most unvisited folder (in alphabetical order) and listed all videos in alphabetical order. So:

  - 123.mp4
  - 456.mp4
def
  - 234.mp4
  - 789.mp4

would become ordered 123, 456, 234, 789 🤔

It gets messed up I think when you import new files, but when you close and re-open, I thought it re-indexes everything to be in the order I described. I thought it was the naturally-expected order -- rather than if the videos were in alphabetical order (ignoring the folders they came from) 🤷

Cheers 🎉 🙌

[CrendKing]

For 1) and 2), see https://github.com/whyboris/Video-Hub-App/pull/457

For 4), see https://github.com/whyboris/Video-Hub-App/pull/458. Unfortunately, I only implemented the background side. To make the feature full, the component HTML needs to be implemented as well. I was too lazy to do that part, but if you insist on me doing that, I can try. I'm just a bit afraid of introducing UI elements that's not fit for the style.

For 3), from what I observe, if I select "Modified time desc" and close the app, the next time I launch the app, "Default" is selected, but the actual sorting order is "Modified time desc". This is because of the currentSort key in settings.json and this.sortType = this.appState.currentSort; in restoreSettingsFromBefore() from home.component.ts. So "Default" is not an actual sorting order but previously used order. Which is confusing, because I don't expect users to inspect settings.json every time they use the app.

If you think what you described in the comment is an unique order and useful, you should give it a proper name instead of ambiguous Default. If it is not unique but maybe identical to Alphabetical, then let's remove Default.

Once that's done, let's implement selecting the currentSort in the sort order drop down.

[whyboris]

Thank you for (1) and (2) -- merged 🚀

(4) is interesting -- I'm unsure what command line arguments could be useful for -- I added a comment in the PR

(3) I see the bug you're describing -- sorry I missed this point earlier. This is definitely a bug and it would make sense for the app to remember the sorting option the user left with last time -- and showing "Default" is an unintended (by me) behavior.

I was commenting more about what I meant by "Default" -- and missed the point about the app restoring the previous sort order while _erroneously_ showing "Default" 😅

I'll come back to fixing bugs in the app once I'm done with the main feature of version 3.0.0 😅 - hoping to release in a few months: #456

[CrendKing]

Thanks. For 3) I'll just wait for 3.0 then.

[whyboris]

I think the fix I would implement for (3) would be simply to update the "current sort order" dropdown after restoreSettingsFromBefore runs -- it's probably just a line or two to add (I believe there are already some methods that update the dropdown) 🤷

Either way -- in version 3 I will have to re-think the "Default" sort anyway -- because the new system that imports videos will run live (skipping the annoying green progress bar that locks up the UI) and thus might not add the "index" correctly 😓

We'll see how it goes 😎

I'm closing this issue, but will reference this conversation in #456 to remember to address this point 😅