Overhaul of models, views, data management

[ferdnyc]

As invited by @jonoomph (careful what you wish for), this PR against the in-progress emojis branch builds on the model/view changes there, extending some nice performance improvements into a fairly extensive overhaul of the entire way data is managed for project asset lists.

TODO for merge

• remove the add_file() implementations from title_editor.py, animated_title.py, that won't work anymore

There are probably some bugs, still, but I've finally got it to a point where it doesn't keel over as soon as I start testing it, so I wanted to get it out there in the world. The major benefits are:

Speed

I just imported a set of (what I subsequently discovered was) 776 image files, mostly PNGs, into a new project as Project Media. It wasn't instantaneous, but it took... maybe a minute? A bit more? The way the OpenShot code through 2.5.1 implemented that sort of thing, it would've quickly ground to a halt and taken possibly an hour or more, with each individual file causing a several-minutes-long rebuild of the full model on each addition. With the new code, not only did the import run at ~ 10 files/second right through to the end, but I was able to delete a random 7 of them instantaneously.

Reduced data code duplication

add_file() and get_image_sequence_data() or whatever have moved out of the two view classes and into the files_model. Not only does that elmiinate duplication, but it allows for some strreamlining — for example, the method is now called add_files(), and it takes an entire list of files (like from a File Open dialog) instead of having to be spun over them one-by-one

Improved user experience

My focus was also on eliminating some long-standing pain points for the users

Selection is now preserved in all lists, always almost always

There's a persistent QItemSelectionModel that's created with each MODEL class, and applied to both of the views (shared between them) when they're set up. You can switch modes and etc., you still won't lose your selected items.

(I notice that selections are lost if the selected items are filtered out of the list, then restored to visibility again. So, that's a bug that I'd like to fix, if possible.)

Image Sequence import is less brain-dead

One of the advantages to add_files() is that it can be smarter about groups of files, when discovering for image sequences. Now, when you import a group of images that could be a sequence:

1. OpenShot will prompt you ONCE to import that sequence
2. If you say No, the code tracks which import files fall under the same sequence pattern, and won't ask you again about any of the rest of them.
3. If you say yes, the code now removes the rest of the matching images from your import list, so that if you happen to try to import a 30-image sequence by selecting all 30 images (which users do all the time), you'll end up with only one imported image sequence item, with the other 29 images automatically and silently excluded.

There's a bunch more stuff, too — see the commit messages, and beyond that, the code.

I'm hopeful that, among many others, this...
fixes #3372, fixes #2557, fixes #2341, fixes #2031, fixes #1288

[ferdnyc]

There are probably some bugs, still,

(I just wanted to reiterate that in big, flashing letters on the side of a mountain, so to speak.)

This is a big set of changes, and I'm not going to pretend that I think every line is bug-free, nor will I lie and say that I've personally tested every possible scenario myself before submitting. I haven't. Not even remotely. That's why I opened this PR, to get other eyes on the code / other testing experiences, so hopefully the remaining bugs can be shaken out.)

[jonoomph]

@ferdnyc Wow! Nice work on this! I'll grab this branch and do some testing today for sure! I'll let you know what I find shortly.

[ferdnyc]

If there's one thing that makes me the most nervous, it's the multiple inheritance in files_model.py that makes FilesModel derive from both updates.UpdateInterface and QObject.

But it was necessary to transfer the ModelRefreshed signal there (the other model classes now singly-inherit from QObject for the same reason), and I really think it's the right way to go. By just moving that signal, along with promoting the mimeData() implementation to live in the outermost proxy class instead (which is where it really needs to be called by the view), I was able to do away with all of the unnecessary chasing of proxy class ModelIndexes back to the source model (when the proxy already has all of the data we need), and I eliminated the custom base model classes entirely for both Files and Transitions.

Swapping those inner model classes over to just the stock QStandardItemModel made me feel like I was on the right track; our data models are fairly uncomplicated (as they should be), no reason they can't store what they need Qt's pre-built model classes.

I still want to make another pass through the table structure for the base models, in fact, and experiment with defining some User Roles so that more data can be moved from hidden columns over to the first ("main") cell in the row — like I did with the Emoji model.

Unless we're either: (1) displaying, or (2) sorting by, a piece of data, there's no reason for it to have a separate column of its own — the lookups get far simpler if it's all stored in the same table cell (under different Roles). Then it can all be accessed using the same ModelIndex, without having to do a lot of row() / sibling() lateral lookups.

[ferdnyc]

If there's one thing that makes me the most nervous, it's the multiple inheritance in files_model.py that makes FilesModel derive from both updates.UpdateInterface and QObject.

But it was necessary to transfer the ModelRefreshed signal there

...And it seems to work just fine, is the real point. I said it makes me nervous, not that it causes any actual problems. Even though I kept half-expecting to hit some. But despite my misgivings, it all seems to work out as it should.

(Though, that new Qt aspect to the outer FilesModel custom class IS the reason I moved the QMessageBox-displaying pieces out of its add_files() and get_image_sequence_info() methods, when I moved them from the view code. The simplest approach I could think of was to make them little helper methods provided by main_window, instead — so that's what I did. Qt didn't really seem too keen on a QObject subclass popping up gui elements all by its lonesome. And it wouldn't have made any sense to make FilesModel a QWidget-derived class.)

[SuslikV]

@ferdnyc don't forget that proxy models can be sorted by name, date etc. Indexes wouldn't be preserved. Proxy needed only for sorting, nothing else. Unsorted is also an option.

[jonoomph]

@ferdnyc Testing Feedback (so far):

• Error on Details Views (Effects/Transitions: dragging an item onto a clip, but only drag from the non-icon columns):

main_window:INFO Switch to Details View
exceptions:ERROR Unhandled Exception
Traceback (most recent call last):
File "/home/jonathan/Downloads/openshot-qt-emojis-mods2/src/windows/views/effects_treeview.py", line 70, in startDrag
  drag.setPixmap(icon.pixmap(QSize(self.drag_item_size, self.drag_item_size)))
AttributeError: 'NoneType' object has no attribute 'pixma

However, the Files Details view still works when dragging on non-icon columns.

[jonoomph]

Edit Title breaks (on both Details and Thumbnails) views with the following error:

 main_window:INFO title editor add confirmed
  exceptions:ERROR Unhandled Exception
Traceback (most recent call last):
  File "/home/jonathan/Downloads/openshot-qt-emojis-mods2/src/windows/main_window.py", line 403, in actionEditTitle_trigger
    index = self.filesView.indexAt(event.pos())
AttributeError: 'bool' object has no attribute 'pos'

[jonoomph]

Accidentally started a review... ughh 😆

[jonoomph]

@ferdnyc What if this doesn't find a valid ID, and thus does not return a value?

[ferdnyc]

Up to the callers. if it's being called in an assignment context, (f = self.current_file_id()), then f will be None. I generally test it with an if not f: after.

If it's being passed to some other function (do_something_with(self.current_file_id()), then that function needs to be able to deal with a possible None argument.

[jonoomph]

Same question, what if this doesn't get a value returned? And what if current_file() doesn't return a File object?

[SuslikV]

disadvantages:

1. Progress of loading isn't visible. UI remains in "not responding" state until task complete.
2. There are two identical FilesModel added to listeners that maintains separately.
3. When emoji drag canceled - new file object still appears in the Project Files list.

All 3 published solutions that modifies files model: #3497, #3366, #3264 has bottle-neck in thumbnails creation. This is most resource demanding task. Only one solution uses asynchronous update of generated thumbnails (total time required to draw all new icons is the same). Other two waits for thumbnails during model creation.

To fix p.2:

openshot-qt/src/windows/views/properties_tableview.py

Line 597 in 06196de

self.files_model = FilesModel(self)

to

self.files_model = self.win.files_model

[ferdnyc]

@ferdnyc don't forget that proxy models can be sorted by name, date etc. Indexes wouldn't be preserved. Proxy needed only for sorting, nothing else. Unsorted is also an option.

Well, exactly. That's why everything has to be looked up on the fly, by pulling up the row() of an index in the proxy model and querying for its sibling() columns for other data. The row numbers will change as things are sorted and filtered, but e.g. the ID of a given file will always be in column 5 of its row() — in the files model or any of its proxies. (Assuming that row exists at all, since filtered proxy models won't contain all possible rows.)

But it would be even easier if, when we got the ModelIndex for a file entry, the ID lived at that same index under a User Role. Then, getting it is just a matter of index.data(self.ID_ROLE).

(The reason I store a QPersistentModelIndex on the base model at row creation is, that index is guaranteed to remain valid throughout the lifetime of the row. So, later on we can go back and use it to quickly find and delete the same row. Propagation of changes to the base model is automatically enabled for proxy models.)

[ferdnyc]

disadvantages:

1. Progress of loading isn't visible. UI remains in "not responding" state until task complete.

Agreed, and while it's much faster now (and loading 766 PNG files isn't something a sane person should be doing anyway), you are left wondering what's happening. Some sort of progress would be good — even if it's just that the Project Files list keeps itself scrolled down to the last item added, so you can watch it populating.

2. There are two identical FilesModel added to listeners that maintains separately.

Good catch, I missed/forgot that properties was creating its own files model. It needs to not.

IIRC both the Title Editor and the Animated Title Editor have their own add_file implementations (I think my fix for that in the blender view was the one I had to cancel?), for this to work they'll need to switch to using add_files() in the model. I think that explains the error @jonoomph saw with the title editor.

3. When emoji drag canceled - new file object still appears in the Project Files list.

I noticed that one too, however with the new persistent model indexing it should be trivially fixable. It just needs to request deletion of the new entry on drag abort.

[ferdnyc]

I also feel like "Add to Timeline" (which right now constructs its own model out of the selected files) should be able to work directly off the SelectionModel for the main model, instead. That'll make its code a lot simpler, it won't need a model implementation of its own at all. It'll just be another type of view on the central files model.

[ferdnyc]

Edit Title breaks (on both Details and Thumbnails) views with the following error:

 main_window:INFO title editor add confirmed
  exceptions:ERROR Unhandled Exception
Traceback (most recent call last):
  File "/home/jonathan/Downloads/openshot-qt-emojis-mods2/src/windows/main_window.py", line 403, in actionEditTitle_trigger
    index = self.filesView.indexAt(event.pos())
AttributeError: 'bool' object has no attribute 'pos'

Oh! No, that's my fault. I know how to fix it, but it ultimately comes down to an issue with our action triggers: All of the functions are specced wrong. Every single one.

They all have the form foo_triggered(self, event). But there's no event arg to a QAction::triggered. The event on all of those functions should be checked, instead. That's why it's a Boolean, and why you can't get the pos() of a triggered(). (In an event handler, like the context menu event of a view, you can use indexAt() to look up the index of the item under the mouse pointer.)

I'll fix the title editor, and fix all of our triggered functions to foo_triggered(self, checked) as well.

[ferdnyc]

and fix all of our triggered functions to foo_triggered(self, checked) as well.

... Maybe AFTER the emoji branch is merged. If I do it here, it'll make it even harder to merge with develop. If I do it on develop, ditto.

[ferdnyc]

The two commits I just pushed should fix both Edit Title, and dragging from non-icon columns for all views. (Had that one half-fixed in my source tree, turns out; just forgot to commit it! And I decided to change all of the view files to use the same code; cleaner — even if it is still copy-pastey.)

[ferdnyc]

I was correct in thinking that both title editors had their own add_file() implementations; those are going to have to come out. So I've set this WIP and added an outstanding TODO item for at least that change.

[ferdnyc]

Properties table migrated

I just finished getting the Properties table off of its own FilesModel() and TransitionsModel() copies, and over to using the shared self.win.files_model / self.win.transition_model. A connection to both models' ModelRefreshed signal takes the place of the previous update_model() calls, and means the context menus will only refresh when necessary.

QAbstractItemModelTester

The other commit I just pushed is a new launch.py command-line option (--test-models) which, when enabled, will link Qt's QAbstractItemModelTester onto each of the base and proxy models of the Files, Emoji, Effects, and Transitions lists. This will only work with Qt 5.11 or higher, and will silently do nothing useful with earlier versions. The model tester automatically monitors model transactions for access violations, unsafe index use, etc.

By default, if the environment variable QT_LOGGING_RULES is not already set, when --test-models is used the launcher will set it to "qt.modeltest.debug=true" which causes quite a lot of console output. (Multiple lines for every single transaction on every single model, as well as the proxies they bubble to.) So it's advisable to apply some less-verbose value, like QT_LOGGING_RULES="qt.modeltest.warning=true", before launching OpenShot with --test-models.

[ferdnyc]

Ohh-kay. I now have both title editors using add_files() from the files model, instead of their own implementations.

Along the way, I changed the naming pattern for title SVG files from "TitleFileName-N.svg" (where N is a counter) to the very Windows-ey "TitleFileName (N).svg". This is for extremely pragmatic reasons: With the previous naming, once you had more than one title add_files() would offer to import them as an image sequence 🤣

And I incorporated some of my changes from #3357 (but most of it is still on hold)

[ferdnyc]

@jonoomph At this point I believe I've got things here working well enough that I'd suggest we merge this to at least the emojis branch quickly — as soon as you're comfortable with it, basically.

There are still a few more things I should do, and even more things I'd like to do, but they're relatively small and can just as easily be done directly on that branch.

The emojis branch is going to be a bear to merge into develop no matter what... probably best not to let this two-stage craziness live on any longer than we have to.

[jonoomph]

@ferdnyc I've been merging develop into emojis quite frequently, to minimize the crazy. Okay, let me take a quick look at this one again! Thanks!