lmms icon indicating copy to clipboard operation
lmms copied to clipboard

Add ability to drag files to and from external applications

Open AW1534 opened this issue 9 months ago • 16 comments

Adds the ability to drag files in and out of LMMS. This required a bit more refactoring than I expected.

https://github.com/user-attachments/assets/3f4d81ce-5e42-4d7a-b74b-1a557c951faa

I should note that this doesn't just work to and from the desktop, I've tested with firefox, CLion, dolphin and discord. It also works with more than just sample files (projects, presets, midis, vsts)

AW1534 avatar Apr 16 '25 01:04 AW1534

To hell with CodeFactor.

AW1534 avatar Apr 16 '25 16:04 AW1534

once #7627 is merged, I'd like to add the ability to drag sample clips out of LMMS too. That could be for a different PR though.

AW1534 avatar Apr 18 '25 15:04 AW1534

I tested this briefly, and it seems to work! I'm using nautilus on GNOME to drag files in.

Things I noticed (these aren't that important though):

  • Dragging in .xml files which are not presets causes a dummy instrument(?) to be added, and when you click on it, LMMS crashes. This is sort of also in master, but it gives you a warning first.
  • You can't drag in .sf2 files (I think this is in master too)
  • You can't drag in vst effects into an effect chain

regulus79 avatar Apr 18 '25 20:04 regulus79

Dragging in .xml files which are not presets causes a dummy instrument(?) to be added, and when you click on it, LMMS crashes. This is sort of also in master, but it gives you a warning first.

Okay, this should be a relatively simple fix.

You can't drag in .sf2 files (I think this is in master too)

Thats actually not in master, it's is a regression- Nice catch!

You can't drag in vst effects into an effect chain

This is out of scope as it isn't in master, but it's definitely a good idea for another PR.

AW1534 avatar Apr 18 '25 20:04 AW1534

I tested this again, and those two issues do appear to be fixed!

regulus79 avatar Apr 18 '25 21:04 regulus79

In Windows MSVC, dragging instruments to the Song-Editor doesn't work Edit: I used the right-click menu to insert Mallets, OBS didn't capture the context menu

https://github.com/user-attachments/assets/bd10b2d7-b436-43b6-b8da-29b5f7439f61

headquarter8302 avatar Apr 19 '25 02:04 headquarter8302

I would like a few things changed. Also I'm testing this and When I drag a sample clip into AudioFileProcessor, it doesn't load the audio for me. Firstly create a sample clip and load audio, then create an empty AudioFileProcessor and try to drag the clip into the processor. Dragging in files from outside works nicely. What else to test?

huh, I don't know why I'm just now seeing this comment, sorry.

Master doesn't let me drag sample clips into AFP either. In fact, I think master behaves worse, as it accepts the drop, but just has weird behaviour. So I think this is out of scope. However, I was planning to add that once your sample exporting PR is merged.

AW1534 avatar May 10 '25 14:05 AW1534

@szeli1 fixed. The issue was being caused by the hasFormat() check

AW1534 avatar May 11 '25 17:05 AW1534

@szeli1 any idea why the builds might be failing on windows?

AW1534 avatar May 11 '25 23:05 AW1534

@szeli1 any idea why the builds might be failing on windows?

Add LMMS_EXPORT to the problematic functions, the plugins can't access them without LMMS_EXPORT.

szeli1 avatar May 14 '25 06:05 szeli1

@szeli1 Thanks, that worked! This should be ready for merge now.

AW1534 avatar May 14 '25 17:05 AW1534

this is a really weird issue, but I see no reason why my PR would cause this. Are you using a custom build to test master and AppImage to test my PR? if that's the case, you should test master with the AppImage to see if you get the same issue.

I am unable to reproduce this on the AppImage or custom build

AW1534 avatar May 18 '25 17:05 AW1534

@szeli1 do the keyboard shortcuts work? (super + right)

AW1534 avatar May 20 '25 20:05 AW1534

@szeli1 do the keyboard shortcuts work? (super + right)

I believe it didn't work for me when I tested it.

szeli1 avatar May 20 '25 20:05 szeli1

@szeli1 do the keyboard shortcuts work? (super + right)

Wait this happened to me in an independent branch, it seems like this is an unrelated bug

szeli1 avatar May 20 '25 21:05 szeli1

I think this is ready for merge.

AW1534 avatar Jun 10 '25 18:06 AW1534

Tested on Windows 11 26100.4484, no issues here from the most common workflows (dragging samples in/out sidebar, dragging samples into Song-Editor track buttons, dragging samples into Song-Editor tracks itself).

Although I find the behaviour of overlapping samples in sample-track instead of overwriting them if there's zero space between the start of the track and the next sample a bit unexpected, because it's different from dragging a sample into the sample-track's track itself

headquarter8302 avatar Jul 03 '25 10:07 headquarter8302

Although I find the behaviour of overlapping samples in sample-track instead of overwriting them if there's zero space between the start of the track and the next sample a bit unexpected, because it's different from dragging a sample into the sample-track's track itself

Could you please provide a video? I'm unclear on what you're describing. Also, is this behavior different to master?

AW1534 avatar Jul 12 '25 23:07 AW1534

Do you also drag/drop a project file (.mmpz, .mmp) into the MainWindow > Song? Or, did the video demo just missed it?

anytizer avatar Jul 13 '25 07:07 anytizer

Do you also drag/drop a project file (.mmpz, .mmp) into the MainWindow > Song? Or, did the video demo just missed it?

yes, anything thats currently draggable from the filebrowser will be draggable from external applications.

AW1534 avatar Jul 13 '25 09:07 AW1534

Could you please provide a video? I'm unclear on what you're describing. Also, is this behavior different to master?

It's the same as master, I just checked

headquarter8302 avatar Jul 20 '25 07:07 headquarter8302

Why are my commits in here?

sakertooth avatar Jul 20 '25 13:07 sakertooth

Why are my commits in here?

huh... good question. it appears i attempted to merge your search revamping PR into one of my branches and accidentally merged it into this one.

I'm not going to have access to a computer until next week, so ill fix it then (although I have no idea how i would go about doing that)

AW1534 avatar Jul 20 '25 14:07 AW1534

@sakertooth I've cleaned up the commits and done some testing to ensure it works as usual.

AW1534 avatar Aug 02 '25 15:08 AW1534

@messmerd Since your last review, all mimetypes but presetfile midifile and projectfile are generated dynamically, by looping through every plugin. I don't think its possible for the remaining 3 to be dynamically generated.

Snippet of Clipboard.cpp:

static std::map<std::string, std::vector<std::string>> mimetypes =
{
	{"presetfile", {"xpf", "xml", "xiz", "lv2"}},
	{"midifile", {"mid", "midi", "rmi"}},
	{"projectfile", {"mmp", "mpt", "mmpz"}},
};

Is this more in line with what you were thinking?

AW1534 avatar Aug 04 '25 13:08 AW1534

As I mentioned before, I'd like to see some of the mime type stuff refactored so that plugin-specific information is no longer present in the core LMMS application. Without that, our plugins are not truly plugins, are they?

You've approved this PR while plugin keys are still present in the code.

Maybe plugin descriptors should contain a mime type in addition to the file extensions. That could then be queried through PluginFactory like file extensions can.

This did happen. I think it is nice that plugins define the extensions, so this is a good change.

For example, Sf2Player would provide both "sf2,sf3" and "soundfontfile" in its descriptor. Or maybe use FileItem::FileType::SoundFont instead.

The existence of this code makes the changes not really "plugin-specific"

	bool isAudioFile(const QString& ext)        { return isType(ext, "samplefile"); }
	bool isProjectFile(const QString& ext)      { return isType(ext, "projectfile"); }
	bool isPluginPresetFile(const QString& ext) { return isType(ext, "pluginpresetfile"); }
	bool isTrackPresetFile(const QString& ext)  { return isType(ext, "trackpresetfile"); }
	bool isSoundFontFile(const QString& ext)    { return isType(ext, "soundfontfile"); }
	bool isPatchFile(const QString& ext)        { return isType(ext, "patchfile"); }
	bool isMidiFile(const QString& ext)         { return isType(ext, "midifile"); }
	bool isVstPluginFile(const QString& ext)    { return isType(ext, "vstpluginfile"); }

FileItem::FileType and the mime type seem to have an almost identical job, so there's probably also a simplification waiting to be made there - maybe a QString("filetype_%1").arg(static_cast(fileType)) mime type similar to what tracks use. Idk. Doing stuff like this is probably necessary to remove all the plugin-specific code from Clipboard, TrackContainerView::dragEnterEvent, TrackContainerView::dropEvent, and elsewhere.

Usually file types need human readable names, so I think marking them with a number complicates things.

I believe there is a much cleaner and more maintainable design waiting to be implemented, and rather than going along with the original poorly-made design by adding additional plugin-specific data hardcoded into the main application, I think this PR provides the perfect opportunity to truly fix the problem.

Keys are still hardcoded into the application, only extensions differ which is an improvement, but doesn't achieve the main goal, it doesn't truly fix the problem.

If it turns out to not be possible to move all the plugin-specific info into the plugins, then I think quarantining it to the FileItem::FileType enum within the main application would be an acceptable compromise.

This didn't happen.

My main issue is that I would like to replace string keys with enum keys. This means keys like "vstpluginfile" must be defined for example in clipboard, so the design mistakes are kept. You've approved this PR @messmerd while these design mistakes are still present.

szeli1 avatar Aug 26 '25 12:08 szeli1

If this was done, the caller of getFile would need to know every single supported extension. then we run into the same issues of hardcoding that this PR solved.

I've mocked up a system of completely dynamic file types.

There are no functions like isVST() and no string literals like "*.sf2".

allejok96 avatar Oct 05 '25 13:10 allejok96

What I've learned the past days is that the clipboard system builds on unspoken rules and is in need of refactoring. Maybe we could add drag and drop without having to refactor... Idk @AW1534 would know. But at the moment, adding a lot of isFiletype() functions feels like a refactor in the wrong way.

allejok96 avatar Oct 05 '25 13:10 allejok96