Save screenshots to filesystem

[Daedren]

This PR adds a feature I kinda built for my own needs, sharing it as people here might like it as well.

A couple new options are added in Settings, one for how to store screenshots (The current Clipboard implementation vs storing in your filesystem) and a folder location for the screenshots if applicable.

I took a particularly opinionated take on the file naming, based on MPV. Allowing user customization would take a bit more effort but is obviously possible.

I'm particularly new to Electron development so I have a few things I'd ask you to pay closer attention to:

• The ScreenshotLocation enum, is there a better way to do this considering we don't have access to Node here.
• Despite being marked as supported for structured cloning, I couldn't get the blob to be copied over the IPC, as it always errors with "Can't be cloned". I passed it as an array buffer instead.

Obviously suggestions and criticism welcome. Thank you for the work on Miru, it's great. I'm on Discord as Raichvent if need be.

[ThaUnknown]

this is a good PR, partially except for the enum, you're probably a hardcore c++ dev, but it's overkill in this case, overall it's a good PR but i don't think I'll accept it in it's current state, this is at no fault of your own but more because of how I'm designing the app

when making the screenshot feature I also debated on saving screenshots to FS, but decided against it. there are a few reasons but main one is definitely simplicity. one of the principles i designed this app with is something i call myself "perfect by default" which TLDR says that there should be as little settings as possible because if a setting exists then it implies that the feature that setting references is flawed by default. another reason is that it adds complexity to the app, sure it might be tiny and for an mpv user it's nothing but the average user has ~2 braincells. another big one is definitely because this uses native APIs and the end goal of Miru is to move away from electron and make it a website (like it was originally when it was private) once BT over WebRTC is widely adopted, which is likely soon-ish. last one is the fact that this changes the behavior of the player module, which i plan on making a standalone public module like videojs, płyt, etc as it's as of now the most powerful video player out there, doing this means that I need to unify it's behavior on the web and electron and once i do migrate to the external module I'll need to spend extra time to undo these changes

this isn't a no. if you somehow redesign this feature to be compliant with these requirements I'll definitely accept it, i know it's asking a lot but that's how much thought i put into everything i designed and I'm not really willing to make exceptions.. I'm free to answer more questions and continue to reviewing this

[Daedren]

No worries, it's your project after all. In regards to the Player, dependency injection could alleviate these web and electron differences and keep the Player independent. Not much work as the screenshot logic was already pulled out of it in this PR.

Though in regards to settings complexity, clipboard is a good sane default (In that future web version, perhaps navigator.share for mobile users) but yeah I don't know how you could make the settings simpler than the current toggle + folder path picker.

Thanks for the quick answer in any case.

[ThaUnknown]

dependency injection

yes something along those lines is the plan but it still requires a fucking insane amt of work, I've started a while back, but got stalled due to how i handle subtitle parsing, file streaming etc

simpler than

you're thinking about this wrong and it's kind of narrow minded, take a step back

what you did is found a problem (no screenshot saving) and found a solution (node FS) and you're now worrying about the complexity and implementation of that solution (stateful keybind), instead think of a completely different solution, and i can think of like 3

for example make a separate keybind alongside the current ss one which instead of saving it via node FS, downloads it with the browser download GUI which prompts the user where to save the screenshot to top of remembering the last save location, and it's an interface the user is likely already familiar with, and boom you've solved most of your problems

[Daedren]

yes something along those lines is the plan but it still requires a fucking insane amt of work, I've started a while back, but got stalled due to how i handle subtitle parsing, file streaming etc

That's great, doing this with the screenshot module would keep it from adding burden your work in the future. (If it's even worth keeping it separate, depending on how this discussion ends up)

you're thinking about this wrong and it's kind of narrow minded, take a step back

what you did is found a problem (no screenshot saving) and found a solution (node FS) and you're now worrying about the complexity and implementation of that solution (stateful keybind), instead think of a completely different solution, and i can think of like 3

for example make a separate keybind alongside the current ss one which instead of saving it via node FS, downloads it with the browser download GUI which prompts the user where to save the screenshot to top of remembering the last save location, and it's an interface the user is likely already familiar with, and boom you've solved most of your problems

Showing the pop-up would interrupt the stream (as it shows in your face), that's kind of a bad user experience, no? Though the chosen folder could technically be saved after the first time to prevent this in following screenshots. I also do think an extra hotkey would add far more entropy on the 2~ braincell users than the setting, but I'm not opposed to it.

[ThaUnknown]

yes something along those lines is the plan but it still requires a fucking insane amt of work, I've started a while back, but got stalled due to how i handle subtitle parsing, file streaming etc

That's great, doing this with the screenshot module would keep it from adding burden your work in the future. (If it's even worth keeping it separate, depending on how this discussion ends up)

you're thinking about this wrong and it's kind of narrow minded, take a step back what you did is found a problem (no screenshot saving) and found a solution (node FS) and you're now worrying about the complexity and implementation of that solution (stateful keybind), instead think of a completely different solution, and i can think of like 3 for example make a separate keybind alongside the current ss one which instead of saving it via node FS, downloads it with the browser download GUI which prompts the user where to save the screenshot to top of remembering the last save location, and it's an interface the user is likely already familiar with, and boom you've solved most of your problems

Showing the pop-up would interrupt the stream (as it shows in your face), that's kind of a bad user experience, no? Though the chosen folder could technically be saved after the first time to prevent this in following screenshots. I also do think an extra hotkey would add far more entropy on the 2~ braincell users than the setting, but I'm not opposed to it.

again, kinda narrow-minded, this is just an example for a solution, not one you need to adhere to, solve this however u want to

[Daedren]

I've kind of lost the time to work on this for now. I'll close this PR so it doesn't bother you until further notice.

Thank you for the help and comments though.