-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Drag and drop from external applications v3 #8075
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
AW1534
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code is good but seems to reintroduce hard coded file extensions which was an issue my PR had solved for the most part.
Now I've updated it so where the plugins define their supported extensions, they can also define an icon for each extension and also whether that extension should be previewed in the browser (those were the two cases where extensions were hardcoded). It's not so pretty, and I don't know if it's the best way to do it. But it solves the hardcoding issue. |
I believe your PR didn't solve the issue, and I think the benefit is outweighed by the complexity. |
| Plugin::Type::ImportFilter, | ||
| nullptr, | ||
| nullptr, | ||
| {{"mid", "midi_file"}, {"midi", "midi_file"}, {"rmi", "midi_file"}}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an improvement compared to the old PR
|
Any thoughts on developing a proper asset management system? Might help simplify work on this effort. |
| if (!DragAndDrop::acceptFile(_dee, {FileType::Sample}) | ||
| && !DragAndDrop::acceptStringPair(_dee, {"sampledata"})) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String pair drag still has this major issue: what is "sampledata"? Where did it come from? Are there others?
My refactor put every key into an enum. This has multiple advantages: more optimized, can be read and understood because every possible key is in the enum.
Here is an example why this matters:
new StringPairDrag( "vstpluginfile", f->fullName(),
embed::getIconPixmap( "vst_plugin_file" ), this );QString type = StringPairDrag::decodeKey( _de );
QString value = StringPairDrag::decodeValue( _de );
if( type == "vstplugin" )As far as I know, these types should be the same, but they aren't because the string keys are not well defined.
| DragAndDrop::acceptStringPair(_dee, { | ||
| "instrument", | ||
| QString("track_%1").arg(static_cast<int>(Track::Type::Instrument)), | ||
| QString("track_%1").arg(static_cast<int>(Track::Type::Sample)) | ||
| }); | ||
|
|
||
| DragAndDrop::acceptFile(_dee, { | ||
| FileType::Project, | ||
| FileType::ProjectTemplate, | ||
| FileType::InstrumentPreset, | ||
| FileType::InstrumentAsset, | ||
| FileType::Sample, | ||
| FileType::ImportableProject, | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see 3 kinds of data here:
- file types that are in an enum
- string pairs that are strings
- string pairs for file types that plugins define and they are linked to an enum key, they exist both as an enum key and as strings.
This although fixes the hard coded issue, it is complex. I would expect everything to be an enum, or at least everything in core (even instrument tracks and clips). I would expect the API for these data to be the same, and internally converted to the right mime with the right data. I do not know how well this PR follows these expectations.
|
I believe this PR solves the hard coded keys issue, but it introduces complexity. I will not approve this PR for now, because I didn't do a detailed review of this PR and it is unclear to me, how the string pair drag API works in this PR. |
This is a good idea. Key(s) could be stored in the assets. Maybe we can even go further and replace GUI code that parses StringPairDrag with code that parses different kind of assets directly. This would mean the gui would become more generalized and that could help commands work. |
Good point @szeli1. At a minimum we need two systems: an enum of MIME types used in core, and a table of additional MIME types and their info like file extensions and icons, because that's loaded at runtime. I purposely don't want to touch the string pairs now because the PR would spiral out of control. That being said, we can design for the future... The question is: should the thing I called FileType be called MimeType with the intention of eventually replacing all string pair drag with proper MIME types? For some things like "track:0" a proper MIME type makes less sense though... If we want to keep using string pairs, it means having two separate systems (we can't just store everything as string pairs in the clipboard because that doesn't work with other applications). |
What would this mean @sakertooth ? What are the gains of an asset management system? Would things like one-off imported hydrogen files or project templates be an asset? |
It might help centralizing the file extensions of the kinds of files we support, among other things. Right now, I'm feeling like all the assets are handled separately in the codebase, so maybe it was causing an issue here. |
@szeli1 if we follow saker's suggestion to put all core mimetypes in a single place, would you be willing to accept replacing the string pair mechanism with mimetypes? The clipboard/dnd already assigns a mimetype to all data. Right now dragging a model will use something like mimetype="stringpair", data="automatable_model:12". If we remove stringpairs altogether it would be something like mimetype="lmms-automatable-model", data="12". |
|
And do we need to maintain backwards compatibility for the clipboard? I would assume if a user needs to copy data from one LMMS instance to another they could open two instances of the same version of LMMS. |
When I refactored StringPairDrag, I didn't remove string pairs, just converted every key inside the code to a matching enum key, meaning internally "string" "pairs" were used, but the API required / gave an enum key and a string value instead of a string key and a string value. (I also added an internal function that gave the string equivalent to the enum key, so actual string pairs can be constructed) |
|
I see, makes sense. The reason I asked is because copy/paste/drag/drop to external applications cannot use stringpairs, it must use standardized mime types (the "key" is a string and the value is stored as bytes). That means even if stringpairs were refactored, they can only be used for some drag and drop in LMMS. So if we have to use a second system, maybe go all in on that and kill stringpairs entirely? (Removing stringpairs would be a another PR, but the groundwork could be laid now) |
continued from @AW1534's work on #7849 ... I had to start clean on the changes I made to it
FileTypeshelpers for dynamic loading of file extensionsClipboard.hsplit into 3 namespacesClipboardfor system clipboard helpersDragAndDropfor string pair drag and QDropEvent helpersMimeDatafor QMimeData helpersPluginViewhandles drop events so plugins doesn't have toSampleClipViewincludes the path of the sample so it can be dropped anywhere (even in other apps)I haven't tested it so much