lmms icon indicating copy to clipboard operation
lmms copied to clipboard

Added Send to new SlicerT instance right click option (#6999)

Open mirk0dex opened this issue 1 year ago • 3 comments

I had to implement the loadFile() method in SlicerT, and also add a "pluginPreference" parameter to some FileBrowser methods, overloading the existing ones.

Still unfinished as you can see from the comments.

mirk0dex avatar Feb 17 '24 00:02 mirk0dex

I haven't implemented key shortcuts yet because I can't figure out where the code for that is. Anyways, I temporarily picked the "S" key for this new option. If you have better ideas please let me know.

Edit: I just realised about Ctrl + S... I changed it to T.

Edit 2: Even T isn't good. If you have plugins or folders starting with it, the file browser will scroll down/up. I'll try tab.

Edit 3: Tab doesn't do anything for some reason, but CTRL + Tab works. Let's try "Shift + Tab".

Edit 4: Nope

Edit 5: I've finally settled with "/", at least for now. Thanks to Agemixer on DS for the suggestion.

mirk0dex avatar Feb 17 '24 13:02 mirk0dex

We should have way so that plugins can expose some detail that allows them to accept samples from the file browser, then we could apply the same code to add send options for any number of plugins it could work for.

sakertooth avatar Feb 26 '24 00:02 sakertooth

We should have way so that plugins can expose some detail that allows them to accept samples from the file browser, then we could apply the same code to add send options for any number of plugins it could work for.

Isn't loadFile() OK?

mirk0dex avatar Feb 27 '24 15:02 mirk0dex

Requesting review from @DanielKauss

Rossmaxx avatar May 29 '24 03:05 Rossmaxx

This seems like a pretty useful change, so no problem there. I gave it a quick look over and it seems mostly fine. The only thing I don't like is having so many duplicate functions. Can't we use default arguments for that? Or if that's not possible for some reason, just change the original function calls to include the boolean? And having both loadFile and updateFile seems unnecessary, isn't it possible to simply rename updateFile? Sorry if what I say is wrong, I can't look at it in more detail at the moment, and it's mostly personal preference anyway.

DanielKauss avatar May 29 '24 22:05 DanielKauss

I don't think we can keep adding shortcuts every time we have a new plugin to handle a particular file type. Suppose something like #6994 now gets merged - we have to pick yet another key, hardcode another plugin name in the core, etc. Then there's also the issue of how to handle third-party plugins. I think it would make sense as a more extensible long-term approach either to allow the user to assign a preferred plugin to each file type, or to specify their own shortcuts for the plugins they need (or both).

DomClark avatar May 29 '24 23:05 DomClark