qt: use `text/uri-list` for internal dragging instead of input items
We already handle text/uri-list
, since we support dragging from
external sources. However, internally, this route is not used and
rather input items are considered for dragging.
This change does not affect internal intra-view move, but affects inter-view insertion.
This also makes it possible to drag media library items that are collection of media outside the application.
Based on !5899 (merged).
Merge request reports
Activity
added MRStatus::NotCompliant label
added 93 commits
-
8929eb3d...ff31bb14 - 82 commits from branch
videolan:master
- d3ab3a03 - 1 earlier commit
- 18cbded0 - qt: add "url" role to MLPlaylistModel
- d86ec7cd - qt: add "url" role to MLFoldersBaseModel
- 01f611f8 - qt: add "url" role to MLAlbumTrackModel
- e2ec01f3 - qt: add "url" role to NetworkMediaModel
- c4026c0b - qt: add "url" role to PlaylistListModel
- 702181ec - qt: add `url` property to PlayerController
- c899448e - qt: add invokable `stringToUTF8ByteArray()` to MainCtx
- d0d32fb9 - qt: provide MIME data in DragItem with type "text/uri-list" as per RFC-2483
- d1a1b0f2 - qt: set uri list for media collection in `MLBaseModel::getDataFlat()`
- 022395c4 - qml: use `text/uri-list` for internal dragging instead of input items
Toggle commit list-
8929eb3d...ff31bb14 - 82 commits from branch
added 1 commit
- 744dd9a8 - qml: use `text/uri-list` for internal dragging instead of input items
added MRStatus::Reviewable label and removed MRStatus::NotCompliant label
There are two advantages that I have observed so far:
- Since input items route is not used anymore,
DragItem
and drop handlers become simpler. - Currently, if a drag is dropped before input items are retrieved, dragging can not succeed. It seems that we need to do this synchronously to guarantee that. We could wait until just before dropping if input items could not be retrieved, but we can not do that when dragging outside the application. Using URI list instead of input items for internal dragging eases this, since the view can resolve the item according to own needs (media library views don't use input items, for example) after dropping.
- Since input items route is not used anymore,
I did not claim it changes, I said "eases" because I thought it would make this less likely as now there is no need to spend time on input items (I assume getting the URL is less costly than creating full-fledged input items).
That being said, the URI is set in
requestData()
handler and that needs to be done before the dragging starts, otherwise the drag item would not be able to represent what is being dragged (which seems to be the case currently). So, if we can make handlingrequestData()
synchronous, then we probably solve these issues. That was on my list to take a look at, but not necessarily in this merge request.We probably need to get rid of the following, it goes against the conventions. I doubt MIME data can be taken into account after drag is started, and this does not ensure that the input items are retrieved until dropping either (that would only apply to internal dragging anyway, what about dragging outside the application?).
Timer { // used to start the drag if it's taking too much time to load data id: nativeDragStarter interval: 50 running: _pendingNativeDragStart onTriggered: { dragItem._startNativeDrag() } }
I assume getting the URL is less costly than creating full-fledged input items
theses input items are not parsed, cost should be negligible
To me the main issue is that you may have attached information on you input item that you won't be able to express as URL, such as media options (force resume on recent video), there was discussion at some point to save and restore more playback data in the medialibrary (selected tracks, etc...), by forcing the conversion to URL you close possibilities.
I think that !5899 (merged) is nice to have, but I would stick to the urls that are immediately available, we should really avoid nesting event loops
Cost is not negligible when having large collection of media, let's say 5000 media items in a playlist.
To me the main issue is that you may have attached information on you input item that you won't be able to express as URL
I did not change how intra-view move works, is this a concern with inter-view copy? How does media library create a media from input item? I assumed it used its URL, making this essentially the same.
we should really avoid nesting event loops
If there is a better way, I would try that. I don't think we can solve the issue I stated without waiting for
runOnMLThread()
completes, whether URL or input item is used.Cost is not negligible when having large collection of media, let's say 5000 media items in a playlist.
the input_item structure is around 25 octets (for 64bit arch), let's round that to 100 for attached metadata, 5000*100 = 500Ko, I think that's acceptable.
is this a concern with inter-view copy?
my main concern is dragging to the playqueue
If there is a better way, I would try that. I don't think we can solve the issue I stated without waiting for
runOnMLThread()
completes, whether URL or input item is used.the hard case are medialib collection (genre/album/ect..) as the individual items are not resolved at the start of the drag, if you actually need synchronous data there, the only one you have are the MLId, so the solution would be to make you drop target (playqueue, playlists, ...) accept MLId and resolve the actual media asynchronously from there.
the input_item structure is around 25 octets (for 64bit arch), let's round that to 100 for attached metadata, 5000*100 = 500Ko, I think that's acceptable.
My concern is more towards the time it takes to create the input items. For example, currently I'm having problems when I try to drag a large collection into play queue quickly. I'm having fewer problems with this merge request, but yes, a further merge request is needed to fix this completely.
my main concern is dragging to the playqueue
If media library actually does attach information to the media library items when it creates the input items, then yes, we can not get rid of the input items route. That being said, we still need to provide URLs in order to enable dragging collection of media outside the application (which this merge request satisfies).
If your concern is purely towards meta-data, I think there is no difference if it is media library who parses that or the playlist. However, there is difference if media library uses data that a media library media has and re-uses that when creating an input item instead of parsing from the file. Is that what you mean?
if you actually need synchronous data there, the only one you have are the MLId, so the solution would be to make you drop target (playqueue, playlists, ...) accept MLId and resolve the actual media asynchronously from there
I don't think that would be a valid approach for drag and drop, even internally. It should not be the dropped view's responsibility to take care of foreign specialized items. Drag and drop can be considered a contract, and a reasonable serialization method should be used. If one side does not obey to the contract, the other side is free to not respect the drop request.
I think
text/uri-list
is the best way since it is already handled, but if it does not satisfy needs due to lack of information when dragging internally, then it should be okay to use input items.MLId
is something too specific.Although I agree that the internal event loop does not look nice, it allows delaying starting dragging without freezing the interface, since the application continues processing the events within the inner event loop. Eventually, by the time
requestData()
handler returns, all data including URLs are there, and the drag can successfully be started.However, there is difference if media library uses data that a media library media has and re-uses that when creating an input item instead of parsing from the file. Is that what you mean?
At the moment, medialib adds information that are already parsed to the input_item, this means that the playlist shoulnd't have to re-parse the file which should save time. it can also set the thumbnail on video file which can't be retrieved by preparsing.
though it seems that the playlist still re-preparse/fetch art on the item, but this seems like a playlist issue
That being said, we still need to provide URLs in order to enable dragging collection of media outside the application (which this merge request satisfies).
that's not a requirement. If this this can't be achieved due to synchronous constrain then we don't do it.
It should not be the dropped view's responsibility to take care of foreign specialized items
Of course handling only intput_itms/urls would be better. that's a tradeoff, if we have a reasonable number of items types (input items, url, mlId) we can handle them explicitly
Although I agree that the internal event loop does not look nice
once it spreads it's a recipe for disaster, it has been stated multiple time by Qt that nested event loop should be avoided. That's a red flag for me.
Since it adds additional information, we probably should not go this way. I will postpone this merge request indefinitely.
However, we need to make sure that the input items are created just before dropping for the drag operation to not fail. And for that to happen, the only thing that comes into my mind is having an inner event loop since the input items are generated asynchronously (from the media library). This is most likely better than freezing the whole UI.
once it spreads it's a recipe for disaster, it has been stated multiple time by Qt that nested event loop should be avoided. That's a red flag for me.
I agree that it should be avoided when possible, but I really don't see any other way. Please let me know if you happen to think of a better way.
I was thinking that, in accept drop function(s), it can be checked if all input items are prepared and if not start a local event loop as I do here and wait for it to be done which should not at least freeze the UI. Preferably with an additional watchdog timer in case it goes wrong.
added Component::Interface: Qt label
changed milestone to %4.0
added MRStatus::InReview label and removed MRStatus::Reviewable label
added MRStatus::NotCompliant label and removed MRStatus::InReview label