Skip to content
Snippets Groups Projects

qt: use `text/uri-list` for internal dragging instead of input items

Open Fatih Uzunoğlu requested to merge fuzun/vlc:qt/dragwithoutinputitem into master
1 unresolved thread

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).

Edited by Fatih Uzunoğlu

Merge request reports

Members who can merge are allowed to add commits.

Merge request pipeline #506077 passed

Merge request pipeline passed for f7eed757

Test coverage 18.46% (-0.02%) from 1 job
Approval is optional
Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Fatih Uzunoğlu added 93 commits

    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

    Compare with previous version

  • added 1 commit

    • 744dd9a8 - qml: use `text/uri-list` for internal dragging instead of input items

    Compare with previous version

  • Fatih Uzunoğlu changed the description

    changed the description

  • Fatih Uzunoğlu added 2 commits

    added 2 commits

    • 566f2b2f - qt: set uri list for media collection in `MLBaseModel::getDataFlat()`
    • 1ee644a4 - qml: use `text/uri-list` for internal dragging instead of input items

    Compare with previous version

  • Fatih Uzunoğlu added 2 commits

    added 2 commits

    • 7ca3bea8 - qt: set uri list for media collection in `MLBaseModel::getDataFlat()`
    • f7eed757 - qml: use `text/uri-list` for internal dragging instead of input items

    Compare with previous version

    • I'm not sure we should prefer uri over input_item for internal use. input item can carry more informations

    • Author Reporter

      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.
    • It seems that we need to do this synchronously to guarantee that

      Using URI doesn't change this.

    • Author Reporter

      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 handling requestData() 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

    • Author Reporter

      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.

    • Author Reporter

      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.

      https://code.videolan.org/videolan/vlc/-/tree/e04bdb364062a46def18a7167d58e9baba0b5592/modules/misc/medialibrary/entities.cpp#L540

      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.

    • Author Reporter

      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.

    • Please let me know if you happen to think of a better way.

      I suggested carrying the MLId for collections.

      It needs to be asynchronous at some point either when beginning the drag or when dropping.

    • Author Reporter

      What happens if the media do not exist in the media library by the time it is dropped? How does vlc_ml_get_input_item() behave if it is fed with an invalid media id?

      If we go with MLId, we should probably use an application specific MIME type for that.

    • for an individual media

      IMediaLibrary::media returns A media instance, or nullptr in case of error or if no media matched, then MediaToInputItem would return NULL as well

    • Please register or sign in to reply
  • Pierre Lamot changed milestone to %4.0

    changed milestone to %4.0

  • added MRStatus::InReview label and removed MRStatus::Reviewable label

  • added MRStatus::NotCompliant label and removed MRStatus::InReview label

Please register or sign in to reply
Loading