UI: Refactor editable list modification

[CodeYan01]

Description

Previously would unnecessarily recreating the data array for every modification (adding/removing/reordering/editing). This clears the fields in the associated data array that other code/scripts would add, such as an ID field for each item that could be used to distinguish each item from their duplicates in the list.

I initially wanted to remove EditableListChanged since I no longer needed to recreate the data array, but it no longer saved the item selection whenever the list is modified, so I kept the function and just changed it to update the selected state in the data array. Note that item selection is only saved when the list is modified, keeping the previous behavior.

Motivation and Context

I am writing a playlist plugin, similar to VLC Video Source, but uses the ffmpeg source. In addition, I wanted to fix the limitations of the vlc source, namely: (1) restarts playback from the first file everytime the settings is opened and saved (even without actually changing anything; (2) saving the currently playing file even if OBS is reopened. this behavior can be changed by the user. Because of this, I need to add an ID field to each item that is added in the editable list, so that i can find the currently playing file in the list, without the issue of duplicated items (string matching would only return the position of the first file). This also lets me edit the currently playing file, which means the file would change, but the position of the currently playing file is the same. String searching would not allow that. In addition, comparing by ID will be faster than string searching. However, whenever the list is modified, the ID field that i add to each item in the array gets cleared. Note that if the settings are saved without modifying the editable list, the ID field is kept. With this change, my plugin works now. The concept behind my plugin may also be used later on in the vlc source.

Additional context: some OBS users like me want to use a playlist and add/remove files while it is playing, or even just toggle the Loop option without the source restarting. This change enables that.

How Has This Been Tested?

I tested all the modification functions (add/remove/edit/reorder) one at a time and saved the settings. I reopen the Properties window to check if the list is correct. If there was a mistake in the modification functions, the editable list should show how the items are stored in the array in the reopened window. I am using Windows 10 64-bit, 22H2, and trying my plugin against the latest commit. I have also tried doing the functions with multiple files, such as removing and adding multiple files, and even reordering items, both continuous selections and separate selections

Types of changes

• Tweak (non-breaking change to improve existing functionality)

Checklist:

• My code has been run through clang-format.
• I have read the contributing document.
• My code is not on the master branch.
• The code has been tested.
• All commit messages are properly formatted and commits squashed where appropriate.
• I have included updates to all appropriate documentation.

[CodeYan01]

Converted to draft at the moment as I have noticed a regression with adding files to an empty editable list in a script

[CodeYan01]

Found the issue. The array was never initialized for the given setting name, so this is the only change I needed to add.

I have verified that new editable lists using scripts or sources (VLC Source and my own plugin) are now working.

[CodeYan01]

Found the issue. The array was never initialized for the given setting name, so this is the only change I needed to add.

I have verified that new editable lists using scripts or sources (VLC Source and my own plugin) are now working.

[CodeYan01]

I apologize for not figuring out how to reference lines changed as I wanted, creating this mess of messages I can't delete.

This change is the new addition that I am referring to.

Previously, empty editable lists would not save any entries added, because the obs_data_array_t is not initialized. This is solved.

[CodeYan01]

Fixed merge conflict by reverting 3a610c6 changes to properties-view.cpp and .hpp, which removed the parameters from EditListReordered.

[CodeYan01]

Added link to the plugin i was working on that relies on this PR, incase it is necessary for reviewing.

[PatTheMav]

Usually "pure" functions and types are preferable over mutable types, as they are less prone to create invalid states of data, especially as replacing the reference to existing data with the reference to updated data is a single operation and ensures that before and after the modification the list is always in a complete, workable state and holders of the old data don't end up in undefined states.

By that token the current implementation is safe, this PR makes it inherently unsafe. It also seems to require that the location in the Qt model (identified by row number) is strongly linked to the index of the internal array (whereas in current code the array is weakly linked to the data model and updated atomically upon model changes).

What would make more sense IMO (and from how I understand your use case) is that you use the property list not as the core of your data, but use it as an input to the data model in your plugin.

So whenever you get a property update, you take the new input, use whatever deterministic hashing function you use to create IDs, then update your internal data accordingly and only act on the internal data.

Using the properties array as your data source sounds like a bad idea, especially as it puts requirements internal to your plugin onto simple property data.

[CodeYan01]

What would make more sense IMO (and from how I understand your use case) is that you use the property list not as the core of your data, but use it as an input to the data model in your plugin.

So whenever you get a property update, you take the new input, use whatever deterministic hashing function you use to create IDs, then update your internal data accordingly and only act on the internal data.

Using the properties array as your data source sounds like a bad idea, especially as it puts requirements internal to your plugin onto simple property data.

What I want is that when a user edits an item in the editable list, I am aware of which item that is. So let's say we have 5 items in the list. Item 1 is currently playing. Item 1 is the same file as Item 3. Moves item 1 to 4. If the video ends, the next item should now be item 5. However, with the current implementation, there is no way of telling how items are moved. If the strings are the same, how would you figure out which item is the currently playing file? item 3 or 4? Even with monitoring the modified callback, how would you tell whether the item was moved to item 3 or item 4 (since they are the same string, and it is a requirement to know which file in the playlist it currently is). Moreover, using the modified callback does not take into account saving settings or cancelling it. Imagine monitoring the modified callback and maintaining a secondary order of the playlist items, that you would apply if the user presses OK. But now the user presses Cancel. Either way, the same update function will be called so there is no way to know whether you're supposed to apply those reordering changes or not. I don't see how you can update the internal data without knowing how the user interacted with the editable list.

[CodeYan01]

And as I said in the PR, if you edit the currently playing file, you'd need a way to find that it was an edit, not a remove-then-add-new operation. And monitoring modified callbacks for each change, reading every string, is a bit too much, isn't it?

[PatTheMav]

Properties exist on API level, and there is no assurance that they are only interacted with via the UI - they can be interacted with programmatically and also via websockets if I'm not mistaken, so you cannot assume that there will always be changes as they would occur when handled by Qt.

You always have to assume that you might get just a single update for a changed list, without being able to trace single steps in it - lots of people use OBS in an almost "headless" way, so your code needs to take that into account.

Thus tying your internal data state to changes of the presentation layer is somewhat dangerous, because you mix up concerns (the UI layer should not need to care about how the presented data is processed, the processing layer should not need to care about how the data is interacted with).

To achieve your goal, you simply would have to keep a "playlist state" in your plugin, with each item uniquely identified by a UUID (decidedly not a hash based on filename), created whenever an item is added.

The property list represents this list of UUIDs, so whenever an update occurs (via API, UI, or other means) you get a property update with a sorted list of UUIDs. You then update your internal state with the new order, but you can also uniquely identify the place of the currently playing element in the list and are also able to differentiate duplicate entries.

[CodeYan01]

You always have to assume that you might get just a single update for a changed list, without being able to trace single steps in it - lots of people use OBS in an almost "headless" way, so your code needs to take that into account.

I already do with this PR + my plugin. I only use the obs_source_info.update and scan the array from the editable list. If there is no id field, i assign one, which lets me figure out which items are new (no id yet), moved or edited (with ids). Thus I never have to monitor the modified callbacks. But what you are suggesting (imo) implies that single steps need to be traced.

To achieve your goal, you simply would have to keep a "playlist state" in your plugin, with each item uniquely identified by a UUID (decidedly not a hash based on filename), created whenever an item is added.

The property list represents this list of UUIDs, so whenever an update occurs (via API, UI, or other means) you get a property update with a sorted list of UUIDs. You then update your internal state with the new order, but you can also uniquely identify the place of the currently playing element in the list and are also able to differentiate duplicate entries.

You lose me here. I am still confused as to how you think this is possible. To demonstrate my point, here is an example python script to print json we get from settings when settings are updated.
playground-editablelist-only.txt

Now if I add 4 items (where the third is a duplicate) like this, it will print out this:

And assuming that's the first time it was updated, the plugin can accept all those as new items and assign IDs to the internal state as you say. And we can say that the current file index is 0.

Now if we move item 0, it will print this:

If you as the plugin author would read this data from obs_source_info.update, how would you figure out which one is the current file now? The data in the obs_data_t for the duplicates are exactly the same. Sure, in your internal state the two entries would be different, but when you get the new data from the update function, there is no way to match which item is which.

Note that we can't rely on the selected field either, because (1) if the OBS_PROPERTIES_DEFER_UPDATE is set, that field is completely useless, and (2) even if that property isn't set, third-party code modifying the settings will need to know when to change the selected field for each item, and (3) clicking Cancel on the properties window without deferred updates also makes the selected field useless.

With my method utilizing the settings object still keeping custom data along with the entries, we can assign id fields to each item in the editable list, which enables us to know where an item has moved to, so we can match them with the internal state.

[PatTheMav]

Ah, my bad - I was under the wrong assumption that you can be in control of data associated with list item entries, but as the property UI element exists today, it only contains description and UI state.

Now, it is quite easy to add UUIDs to those list items (so any code interacting with the editable list might benefit from unique identifiers) and avoid the issue of pushing little data updates for every change of the list:

1. Instead of adding the QStringList files to the widget, you add each item and add the UUID for each as custom data (that's what it's for):

    for (QString file : files) {
        QListWidgetItem *item = new QListWidgetItem(file);
        item->setData(Qt::UserRole, QUuid::createUuid().toString(QUuid::WithoutBraces));
        list->addItem(item);
    }

2. You add the UUID to the data for the property:

obs_data_set_string(arrayItem, "uuid", QT_TO_UTF8(item->data(Qt::UserRole).toString()));

3. You read the UUID from the data to uniquely identify each item:

const char *uuid = obs_data_get_string(item, "uuid");

It's super simple, ensures that updates are atomic (you get a single update for an updated list, no matter how it was updated). And this would also benefit other sources based on a file list as well (e.g. the image slideshow).

The reason why I'd prefer this solution is because it fixes the issue as I understood it ("I need to be able to uniquely identify duplicate items in a list widget"), except if the requirement is a different one ("I need to be able to track every UI change in my plugin") and keeps complexity down (or more accurately moves the complexity to where it's needed - namely the plugin itself).

An alternative would be to extend the QListWidget itself to make it a PlaylistWidget that would actually have a concept of "currently playing item". That would require the plugin to inform UI about playback state (so it could be stored on the list item, similar to the UUID) so that the information would be retained with the list update.

That one might also make sense given that this widget is mostly used for playlists, so you could even mark the currently playing item in the list with an icon or other marker. Using an UUID is a bit of a copout as it enables the "uniquely identifiable" part while keeping the UI element itself blissfully unaware of what its items are doing, but short of moving the concept of playback state into the UI element itself, it's the simplest solution.

[CodeYan01]

Thank you for the detailed solution. Would you prefer that I open a new PR following the UUID solution and close this?

As for the alternative, I personally would like that as well, especially being able to mark the current file (which the image slideshow and vlc video source would benefit from), but i feel like that would require someone to make an RFC about it, and I am not confident with my ability to do such big QT and api changes. One issue with marking the current file is that there are folders. Imo folders would benefit from a QListWidget with a second level of hierarchy, so the folder items will be displayed (and can be marked as the current file, rather than marking only the parent folder).

[PatTheMav]

Thank you for the detailed solution. Would you prefer that I open a new PR following the UUID solution and close this?

As for the alternative, I personally would like that as well, especially being able to mark the current file (which the image slideshow and vlc video source would benefit from), but i feel like that would require someone to make an RFC about it, and I am not confident with my ability to do such big QT and api changes. One issue with marking the current file is that there are folders. Imo folders would benefit from a QListWidget with a second level of hierarchy, so the folder items will be displayed (and can be marked as the current file, rather than marking only the parent folder).

That depends on whether you find the UUID-based solution workable for your issue. Also it would probably help if some other maintainer can tell me I'm far too concerned about the number of update calls between UI and the plugin here, so don't take my word as gospel about that.

I've added the necessary change to a local branch, maybe give it a try and report back if it's workable for you: https://github.com/PatTheMav/obs-studio/tree/listitem-uuids

[CodeYan01]

Finally got around to testing it. In theory, of course, having UUIDs in the list items themselves will indeed let me do what I want, and I wrote a branch on my plugin to accommodate your changes to try it.

However, when I tried your build, I noticed a few issues.

1. Reordering items doesn't work, and would simply reset it to the previous order when you reopen Properties window.

obs64_trv7ecvlkU.mp4

Log: https://obsproject.com/logs/wcfCWzSlvnCMWMAI
Such issue and QT error message can also be noticed with the vlc source.

2. The items only have UUIDs when they are added. obs_data_get_string(item, "uuid"); returns the UUID on first update. When I add files to the editable list again, the first files have empty strings for the uuid, and the new files have uuids. Note that if I don't add files, like triggering an update just by changing other properties, the last files added will still have their UUIDs.

Now, something to be considered with my PR vs yours is the possibility for a plugin wanting to add configurable properties for each item. For example, what if you want the second file in vlc source to use audio track 2, the rest track 1, and the user can configure those for each item? With this PR, the author can simply add fields to each list item, sort of how you would add private data to the list items in qt, so it's not just an id field. But of course, if something is going to be complicated like that, they could also simply use the uuids from your solution and build their own data structure and match them, such that the extra fields will be handled by the plugin itself without modying the settings.

Now, I like having the uuids already generated in each list item, so plugins won't have to add extra code like i did just to figure out duplicates and stuff.

I will be glad with whichever solution is accepted.

[PatTheMav]

1. Reordering items doesn't work, and would simply reset it to the previous order when you reopen Properties window.

That doesn't seem related to the UUID change, but instead us connecting an event handler to a non-existing signal (the signal exists on the abstract item model, but not the QListWidget).

Using connect(list->model(), &QAbstractListModel::rowsMoved, info, &WidgetInfo::EditListReordered); on line 744 in properties-view.cpp fixes the issue.

2. The items only have UUIDs when they are added. obs_data_get_string(item, "uuid"); returns the UUID on first update. When I add files to the editable list again, the first files have empty strings for the uuid, and the new files have uuids. Note that if I don't add files, like triggering an update just by changing other properties, the last files added will still have their UUIDs.

That just requires adding similar code to line 735:

for (size_t i = 0; i < count; i++) {
	OBSDataAutoRelease item = obs_data_array_item(array, i);
        
        QListWidgetItem *list_item = new QListWidgetItem(QT_UTF8(obs_data_get_string(item, "value")));
        list_item->setSelected(obs_data_get_bool(item, "selected"));
        list_item->setHidden(obs_data_get_bool(item, "hidden"));
        list_item->setData(Qt::UserRole, QT_UTF8(obs_data_get_string(item, "uuid")));
        list->addItem(list_item);
}

This will read the original UUID values from the plugin data and populate the list items with them. As the UI state of the dialog is removed entirely when the properties dialog is closed, the data needs to be restored that way.

Now, something to be considered with my PR vs yours is the possibility for a plugin wanting to add configurable properties for each item. For example, what if you want the second file in vlc source to use audio track 2, the rest track 1, and the user can configure those for each item? With this PR, the author can simply add fields to each list item, sort of how you would add private data to the list items in qt, so it's not just an id field. But of course, if something is going to be complicated like that, they could also simply use the uuids from your solution and build their own data structure and match them, such that the extra fields will be handled by the plugin itself without modying the settings.

I don't think we have the UI code for extra properties attached to specific list items (without the UUID change the list is just a "stupid" list of file or directory paths.

The correct way to do that would be to create a custom List widget with custom ListItem widgets that might carry the required property fields but those would then also require the necessary plumbing to read the structured obs_data properties (same as uuid has now become its own property).

[CodeYan01]

Resolved merge conflict and verified it works using my plugin (as it would restart if the id field i added would disappear).

As for the convo earlier, who should we contact to comment?

[stiven202]

I don't understand, what was this complement? Could you fix that so the playlist doesn't restart? where do I download this modification? the plugin doesn't work.

[CodeYan01]

@stiven202 the linked plugin will only work if you use the custom build of OBS that I linked in the download section of my plugin. https://github.com/CodeYan01/obs-studio/releases/tag/29.1.3-pr8051

This PR allows that to happen, but since it (or an alternative) is not yet merged to OBS, the usual OBS install won't work (and you won't be able to update the custom build normally). As such, on the custom build, VLC Video Source is NOT affected. The only playlist that does not restart is the Media Playlist Source. It works, and I have been using it personally without problems.

[GloriousEggroll]

This PR alongside its plugin are really useful on the OS distribution side because it allows not relying on vlc for playing video playlists, therefore being able to build OBS without vlc as a requirement and thus avoiding some codec hell. It would be nice to get this merged.

[derrod]

This PR alongside its plugin are really useful on the OS distribution side because it allows not relying on vlc for playing video playlists, therefore being able to build OBS without vlc as a requirement and thus avoiding some codec hell. It would be nice to get this merged.

This is on a long list of things that need review, but in any case it'll hae to wait for 31.0 at the earliest.

[PatTheMav]

@CodeYan01 have you tried the fixes I suggested in #8051 (comment) to check if they resolved the issues you reported with the original UUID implementation?

If those indeed fix the issues, I'd suggest that as an alternative to this PR (you can either update this one or open a new one, I'd probably be fine with the former for simplicity's sake).

For the configurable properties part, that would require more tooling around how this list widget actually works (and probably require an entirely new widget to begin with), but in theory it could be "encoded" into the data part (so it would contain this meta data in addition to the UUID which then needs to be decoded in the property code of the plugin). But I'd consider that out of scope for this PR.

[CodeYan01]

I had not tried the fixes yet, but I will soon, and then update as you suggested. Thank you.

…

On Fri, Jun 14, 2024, 9:36 PM Patrick Heyer ***@***.***> wrote: @CodeYan01 <https://github.com/CodeYan01> have you tried the fixes I suggested in #8051 (comment) <#8051 (comment)> to check if they resolved the issues you reported with the original UUID implementation? If those indeed fix the issues, I'd suggest that as an alternative to this PR (you can either update this one or open a new one, I'd probably be fine with the former for simplicity's sake). For the configurable properties part, that would require more tooling around how this list widget actually works (and probably require an entirely new widget to begin with), but in theory it could be "encoded" into the data part (so it would contain this meta data in addition to the UUID which then needs to be decoded in the property code of the plugin). But I'd consider that out of scope for this PR. — Reply to this email directly, view it on GitHub <#8051 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APSLKZJIJFIYROBY6ULK2OLZHLWT7AVCNFSM6AAAAABJJBV4V6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRYGA3TAOJWGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[xhorntail]

I had not tried the fixes yet, but I will soon, and then update as you suggested. Thank you.
…

Any updates on this?

[GloriousEggroll]

Any word on this? 31 came and went, looks like the UUID issue was resolved since then too. It's been 6 months

[RytoEX]

Any word on this? 31 came and went, looks like the UUID issue was resolved since then too. It's been 6 months

It is unclear to me if #11126 superseded this or if this was meant to be cleaned up after that to add additional functionality. My quick read of #11126 suggests that that enables the same thing as this, but leaves the handling of additional metadata to third-party code.

[CodeYan01]

I can close this since #11126 is merged, but yeah, the difference between this PR and the other one is that this allows for extra metadata to be saved, so I'm not quite sure whether this should be left open. Please let me know what to do.

As for my plugin, I have been gradually getting back into development as I got busy from work, and I had issues building obs several times so i was delayed, and I decided I'd postpone some big changes in order to quickly release a version compatible with OBS 31 (the commit is already there, just have to retest the plugin and fix a bug report).
EDIT: already updated the plugin

Huge thanks to the OBS team for merging #11126