Marker / Effects search

[ThomasWilshaw]

Adds filtering to marker and effect lists. I also had a go at saving the generated list of markers to save have to recalculate it every frame. It does improve performance but there may be a btter way to do it. If you think it's worth doing I can add the same to the effects list to.

Fixes #114

[timlehr]

@ThomasWilshaw can you resolve the conflicts on this one? Looks like a great change.

[ThomasWilshaw]

I'll try and have a look today, it looks like the conflicts are fairly small. It's been a while since I wrote this so I'll have ot refresh my understanding of it and I'd really appreciate a review, there may be better/cleaner approaches to this.

[ThomasWilshaw]

Merge conflicts fixed but I think I need to make sure it's all playing nicely with tabs and I'm not sure if I'll have time to do that today.

[timlehr]

I had a look just now and noticed a couple of things:

• the code indeed segfaults with 11 when multiple tabs are open, the marker inspector is visible and we switch tabs
• add some more details on how to use the text filter

Very nice addition otherwise. Thank you so much 👏

[ThomasWilshaw]

That should be the crash fixed, I had to change the filter state info to be stored per tab. I need to do this for the Effects filter too if you are happy with the approach?

R.e. documenting the usage. ImGui's text filters are not a standard widget (as far as I can tell) so the normal tooltip approach doesn't work. Given it's quite a chunk of text anyway I've added it as a collapsable panel under the filter box. Does that work from a design/UX point of view?

[timlehr]

Awesome! I just played around with it and it seems to work great now. I am personally fine with the usage collapse section since it's collapsed by default and doesn't take much space.
I'd say let's get this merged and then make a separate PR for the Effects section? Are you good with this @jminor?

[ThomasWilshaw]

Haha sorry I pushed this and then saw your message. This last commit moves the Effects search stuff to the tab data an should be working. It was half done already and works exactly the same as the Marker code so it's probably okay to keep in this PR? I'm happy to remove the commit if not though :)

[ThomasWilshaw]

Also that last commit did fix a bug with the Marker filter checkboxes, they weren't saving their state per tab. Sorry that was probably too many changes in one commit.

[timlehr]

Just gave it another look, works great. I don't see a problem combining this, given the similarity of the code. Thanks so much! For @jminor reference, here is how it looks like on the Effects tab.

[jminor]

This looks great! My only usability concern is the space taken by the "Filter Usage" even when collapsed. Could the it be moved to a "(?)" icon with a tooltip?

Like this one in the ImGui demo:

[ThomasWilshaw]

@jminor I've moved the tooltip to a (?) symbol as requested. Do you think we need the label "Filter" next to the text box?

[jminor]

This is looking really good. I like the tooltip approach. Thanks for making that change.

I found two bugs:

1. If you use Edit->Delete to remove a clip with effects on it while the Effects tab is open, Raven crashes because the cached filter results reference the deleted object. I think this could be fixed by setting the flag to rebuild the filter cache when the timeline changes. At the moment, the delete operation is the only one that matters I think?

2. The - prefix to exclude results from the filter isn't working as expected.
   Example:

[ThomasWilshaw]

I think that should be both things fixed. I updated the filter logic and split it into seperate functions (one for Markers and another for Effects). I also added a state_change bool to the TabData struct that can be set by anything that changes a tabs state. The existing but previously empty function AppUpdate now handles state change logic. If the state change flag is set I force the marker and effects lists to reload when they are next drawn. Is that what that function was orgionally intended for and is this a suitable clean way of doing things? It should be fairly extendable in the future.

These changes introduced a random bug which I have also fixed so I can test things. An empty string was being used as a label, however this is not allowed in DearImGui as the stack uses widget item names as keys. The correct way to set an empty label is to use "##".

[ThomasWilshaw]

Sorry for the bump but I wondered if you have had a chance to look at this @jminor ?

[jminor]

I just re-tested. I still see a crash when I delete a clip that holds a marker. Otherwise this looks great.

Repro steps:

• Download this OTIO: https://github.com/jminor/otio-time-warp-test-suite/blob/main/time_warp_test_suite.otio
• Open it in Raven.
• Select the Markers tab
• Select a clip which contains a marker
• Press the Delete button in the toolbar

Stack trace:

Thread 0 Crashed::  Dispatch queue: com.apple.main-thread
0   raven                         	       0x1027d26a8 opentimelineio::v1_0::Composition::range_of_child(opentimelineio::v1_0::Composable const*, opentimelineio::v1_0::ErrorStatus*) const + 216
1   raven                         	       0x1027e7594 opentimelineio::v1_0::Item::transformed_time(opentime::v1_0::RationalTime, opentimelineio::v1_0::Item const*, opentimelineio::v1_0::ErrorStatus*) const + 264
2   raven                         	       0x1027b6684 DrawMarkersInspector() + 4556
3   raven                         	       0x1027ab574 MainGui() + 1452
4   raven                         	       0x1027ce6cc main + 880
5   dyld                          	       0x1810a1d54 start + 7184

[ThomasWilshaw]

Thanks and sorry I missed that. The latest commits should fix that crash as well as ensuring both JSON edits and adding markers cause the lists to update correctly.

I'm not quite sure why all the checks seem to be failing. I notice there's been a dependancy update, do I need to rebase this PR to make that all work again?

[ThomasWilshaw]

Actually looking at the auto PRs from dependabot it seems all the actions are failing since the minzip-ng update