Emojis and Transform Improvements #3366
Conversation
…ts, and a handful of test emojis from OpenMoji.
…stom sort and filter function (which takes into account the group: common, extra, user) and the filter text. Also, moved the models outside of the widgets for Effects, Transitions, and Emojis.
…onger delete the tree/list view widgets either. They both always exist, and share the same data, and toggle visibility back and forth. Added wait cursor when adding/importing files.
…th with a shared model, and toggle visibility to the user.
…, and toggle between list and tree view.
…dding to timeline)
…custom Proxy class for filtering. Also enabled locale aware sorting on listview and treeviews.
… This allows them to be cross-platform paths, and relative/magic paths for OpenShot which always work.
…mojis" into separate templates though, since there are thousands of them, and they are not critical for OpenShot to be usefully translated.
…ws (since treeviews share the proxy model, they already get the updates)
- Rotation added - Shear added - Improved paint logic - Improved update logic - Ability to zoom in and out of the video preview widget!
…rm tool. Added custom cursor rotation, as the transform is happening in real-time. Feel great, very polished.
…ing cursors during a drag. Fix shear_left and shear_top to take scale into account.
|
I'm disappointed. You want to store 3000+ of small files on user PC? Edit: maybe it doesn't matter. Let it exist. |
…). Both shear and rotation need to know the origin, and share the origin point. Updated center origin display to a circle with a cross through it.
|
PR #3377 is a PR against this branch (a PR against a PR, one of my favorite "Git is confusing and weird" tricks), with a bunch of tweaks I came up with. Part of the reason being that it's impossible to review this PR, because the GitHub web interface completely breaks down trying to show a diff for 6000 PNG and SVG files. IMNSHO, the commit(s) to add those in should really be broken out into a separate branch/PR, for sanity's sake. But the suggestions I had were also too involved to leave as review comments, it was easier to implement it and file the PR-PR once I had it working. However, I did place a review on MY PR with a running commentary of my reasoning behind the suggested changes. Please do look #3377 over, if you have a chance. |
This PR also contains at least two major change sets that are completely unrelated (the emoji feature and the improvements to Transform), plus I think I saw some fixes to the existing models. The latter of which, especially, are desperately needed and will be great to have, but really should be their own PR. Some of the non-emoji changes appear to overlap with other outstanding PRs, for one thing. So then everything depends on what order things merge in, and that's how we end up accidentally losing code. (Something that's happened several times already, when these massive-change branches are merged.) |
Honestly, that gives me pause as well, and now it's up to 6000 files with the cache. (Which I have to say feels dubious to me.) QtSVG is fast for rendering SVG icons, that's its entire reason for existence. I have a hard time believing cached PNG conversions really make that much of a performance difference. Maybe if the SVG thumbnails were being manipulated, or converted to PNG on the fly... but not if they were just being rendered as straight SVG loaded using ...I mean, I wouldn't have thought, anyway. I'm willing to accept that there are unexpected issues that caused performance problems with the SVGs loaded directly. I just wonder if the slowness wasn't due to other things, like the earlier filtering code, rather than the image format. |
| def lessThan(self, left, right): | ||
| """Sort with both group name and transition name""" | ||
| leftData = self.sourceModel().data(left) | ||
| leftGroup = self.sourceModel().data(left, 257) # Strange way to access 'type' column in model |
There was a problem hiding this comment.
@jonoomph
I have to say, this terrified the living crap out of me, when I came across it while working on these. I absolutely do not understand how it works, even sort of. "Strange" doesn't begin to cover it. Where did that even come from?
If I'm understanding it, though, the equivalent in non-magical Qt API should be something like:
leftGroup = self.sourceModel().sibling(left.row(), 2, left).data()
# or even possibly
leftGroup = left.sibling(left.row(), 2).data()There was a problem hiding this comment.
I'm almost tempted to think that it works because it's overflowing an old-school, single-byte char field.
(255 + 2 = 257 after all.) Thus reaching the second column that way.
But that's still crazy, possibly even crazier than whatever's actually happening here.
There was a problem hiding this comment.
@ferdnyc Ahhh, yes, that makes sense. I found this using a debugger (almost positive about that, because how else would I find the index... lol)
There was a problem hiding this comment.
@jonoomph Maybe we fix just this one, before the merge? 🤣 I'll look at fixing it now, in fact.
There was a problem hiding this comment.
Fixed. For a little extra flexibility, I made it respect sortRole() for the main column (only), instead of forcing the default Qt.DisplayRole. So the sort role can be changed, if needed at some future point, and it'll still work.
There was a problem hiding this comment.
Oops, looks like I was too slow by less than 5 minutes. 😆 I'll do a new PR, nbd.
There was a problem hiding this comment.
Ooops, missed this comment.
There was a problem hiding this comment.
SON OF A BITCH!!!!
Five months later, I finally discovered the source of those phantom 257 data items!
It seems that, unlike most of Qt's pre-packaged Model/View classes, QStandardItem.data() and QStandardItem.setData() both DEFAULT TO Qt.UserRole + 1 when not given an explicit role:
QVariant QStandardItem::data(int role = Qt::UserRole + 1) const
Returns the item's data for the given role, or an invalid
QVariantif there is no data for the role.Note: The default implementation treats
Qt::EditRoleandQt::DisplayRoleas referring to the same data.See also
setData().
void QStandardItem::setData(const QVariant &value, int role = Qt::UserRole + 1)
Sets the item's data for the given role to the specified value.
If you subclass
QStandardItemand reimplement this function, your reimplementation should callemitDataChanged()if you do not call the base implementation ofsetData(). This will ensure that e.g. views using the model are notified of the changes.Note: The default implementation treats
Qt::EditRoleandQt::DisplayRoleas referring to the same data.See also
Qt::ItemDataRole,data(), andsetFlags().
I guess those handwaves about "The default implementation treats..." are their explanation for it, but it's still a pain in the ass. Classes like QAbstractItemModel— and, hell, evne QStandardItemModel! — return the actual Qt.DisplayRole and Qt.EditRole for data() and setData() respectively.
So, the long and short of it is, WE'RE the ones shoving all of those role-257 data items into the data models, anytime we call setData() or data() on an instance of QStandardItem!
(And now that I know this, I can adjust my user-defined roles to start from higher than Qt.UserRole + 1. Which would've been really nice to know like 5 or 6 hours ago, insead of spending them pointlessly fighting with disappearing model data. 😡)
There was a problem hiding this comment.
This is entirely @N3WWN 's fault. Stupid properties table keyframe-status code!
- properties_tabelview will pull data directly from files_model and transition_model on the get_app().window object. - A connection to both models' ModelRefreshed signals that forces a menu data reset replaces the previous update_model() calls
- When passed, OpenShot will attempt to load QAbstractItemModelTester into the files, emoji, effects, and transition models (base and all proxy models). This requires Qt 5.11 or higher. - By default, if the environment variable `QT_LOGGING_RULES` is not set, it will be set to "qt.modeltest.debug=true", which causes quite a lot of console spew. It can be set to something less verbose before launching OpenShot, e.g. "qt.modeltest.warning=true".
- Switch from TitleFileName-N for numbered filenames, to Windows-esque "TitleFileName (N)", because the previous value would trigger the image sequence import logic
I had meant to respond to this, and I never did. I think, for now, "Emojis" is fine for the panel name. If it was going to expand to cover additional data sources, at that point I think it would be worth revisiting the question, and perhaps creating a set of subpanels, one per "category" — one of which would be the current "Emojis" (and still named as such, so it wouldn't really cause any confusion). In fact, one option for this going forward, especially if additional sources are added, would be to roll it all back into Project Files — which, instead of being "a list of files imported into the project", could become a metadirectory of media source bins, one or more of which represents user-imported files, with several others being application-defined (and perhaps configurable as to whether they show up at all). The whole "media library" thing is a pretty common pattern for NLE suites. In fact, tonight I stumbled across another possibility for a media library source: QtAwesome. (Which, despite the name, is actually PyQtAwesome. Except that it's PyQt5/PySide2 agnostic.) It's basically a PyQt wrapper around FontAwesome and other types of clipart-ey libraries. (It can even load external font libraries, so there's some possibility we could convert the OpenMoji SVG collection into a compatible type of font and leverage the QtAwesome library to do efficient loading.) In the world I'm imagining, the user would be able to specify one or more disk locations as "library" folders that are listed as separate subtabs/sublists of Project Files, and are always scanned for all valid media files (silently ignoring anything that's not supported), with the contents made available for inclusion in any project they work on. Files loaded from those paths, just like files from the Emojis library or any other source, would still be imported into the project data, but only at that point, so the (This also relates, indirectly, to an old request I just saved from being closed, because I hadn't seen it before and it's actually a really good idea. As well as another very recent one that, in a different way (a better way, in terms of
|
That could also play into an improvement in our recovery logic. Once we have something counting the actual uses of each "project file" item, when doing missing-file recovery and we encounter files that aren't actually used in the project, we might as well just silently remove instead of bothering the user about them. |
Overhaul of models, views, data management
|
@ferdnyc if you attempt to implement everything that users asks for - it will be unusable "nightmare" rather than nice video editor, believe me. |
That meaningless tautology aside, the fact is that SOME user requests are objectively good ideas. And showing an "Are you sure?" dialog before deleting a project file that's actively in use on the Timeline... that's just about the best example I can imagine of one of those. |
|
@ferdnyc Okay, I think I'm ready to merge this beast, and then we can get things back to normal on |
|
💥 |
|
I have one question: How do you end the new transform mode? I'm finding myself rather stuck in it, since it's surprisingly resilient — even moving the playhead completely off the clip being transformed doesn't end it. (It shows the transform rectangle for that clip, overlaid on whatever other clip is currently visible.) Seems like there needs to be a way to say, "Okay, I'm done transforming now." Am I missing something? |
|
Okay, that makes sense. Unselecting the clip removes the transform from the screen. However, we could add an ESC and/or ENTER keypress that removes the transform (or unselects the clip, which would remove the transform). |
Take in mind that viewing Transform rectangle (boundaries of the Clip) is useful for positioning the Clips (even if Timeline's cursor placed out of the Clip). See: #2420 (comment) |
|
Sure, the user can leave open the transform boundaries as long as needed. But for a user who is "done" transforming a clip, I think it might be an intuitive "guess" to hit ENTER when done, and then get the satisfaction/confirmation of the rectangle disappearing. But that's all the ENTER key would do, remove the selection. It's not required. |
|
In particular, since moving the playhead while transform is active will create more keyframes at the destination frame, being able to say "stop doing that" is definitely useful. I'm starting to wonder if transform shouldn't be a toolbar-button-toggled mode, like the razor tool. |
I don't get it. What I see in my builds that the Transform Rotation cursor only shown after I selected nothing in the Timeline (after clip were animated). But no new keyframes are created and no Transform bounding box is shown in the Preview. The "rolling stone" animation is much easier to do with this new Transform. I can see that the Playback is may be active during Transform, but really - you still need to drag items in the Preview... This is expected. So what is wrong here? |
Sorry, I want clear. Once you've activated Transform, it stays active even if you move the playhead, which means it can still create new keyframes via mouse movements in the preview. Additional manual actions are still required, it's true. But it does mean that you're always only one click away from accidentally modifying your properties. (As well as just having that blue rectangle in the preview, which is annoying if you don't need it.) Having to deselect the clip to make it go away seems like an unintuitive interface; we should at least also accept standard keypresses like Esc.
I don't know what that is, but I don't believe what I'm suggesting would change anything about how transform can be used. I only want users to have additional options for exiting the mode if they choose.
Wrong? Nothing, per se. I'm just suggesting that things can be enhanced, without detracting from current behavior. Or maybe even improving it. If transform were a toolbar mode, then when it's active, every click on a clip would activate transform immediately. Maybe that's what the new preference checkbox does, so really I'm just suggesting that the checkbox be moved out of Preferences and onto the toolbar -- and that, when it's deactivated, transform is exited if it was active. If someone chooses to, and doesn't mind the blue rectangle, they can leave it active all the time and transform will be as accessible as possible. Whereas for other users who only occasionally need to activate it, it would provide a clear and simple way to exit the mode when they're finished. Best of both worlds, I hope. Unless I'm missing a use case? (The one thing not covered in my current thinking is what should happen with multiple selection. I'm not even really sure what currently happens.) |
|
Usually, I clicking in the window where I see the object. In OpenShot I need to click on the Timeline instead (even if the clip only single object) to move something in the view. Of course, it is intuitive interface I simply don't understand its complexity... Edit: ...as soon as I need to click in the Timeline to move something - at least I will try to click somewhere again to disable the Transform. This, surely looks logical. |
It is interesting thought. Let's call it "Editing Mode" feature or "Animate" button. It would be strange if it only take control over the Transform and not over other edits... |
|
@jonoomph I'm just following up #1040, where the transform changes here were hoped to have sorted out problems with a rotated clip. With the build as a whole, my user experience (I'm quite new to this) was that the Christmas lights had arrived; I don't understand why the emojis have such prominence on the starting screen. Can you imagine what a new user will think - where their eye will focus? Even if someone actually wants to use this particular image library subset, it's too difficult to find the right image. Try a search for 'weather'..... Sorry! It's one of those cases where 'more' != 'better' |
|
There is another aspect about the OpenMoji library - it is Creatuve Commons licensed. GPL3 is listed as one way CC compatible, but according to the user guide Openshot is "GPL3 or later", which from my reading is not compatible. Also, any derivative works must be CC licensed, according to my reading. That means the user must provide attribution with his video. I could be wrong and I hope I am, but this does need serious thinking about. OpenMoji is not a public domain resource. Sorry, again. |
NOTE: This is a massive PR, with lots of new files. This might not work great in GitHub. 😢
Emojis!
Integrated
3000900+ emoijs from OpenMoji project. This started out a bit experimental, and I'm still very open to suggestions and feedback on it. This PR adds a new "Emoji" dock, and allows for users to drag and drop nice and clean vector emojis onto their project. It's grouped into many categories with a dropdown filter, and is searchable. Added new @emoji/ path prefix. Also supports custom emojis in /.openshot_qt/emojis/.Models and Views
Also, I've refactored our models, listview, and treeviews, so we have 1 primary model owned by the MainWindow, and we use proxyModels for filtering. The listview and treeview share the same proxy model, and are always filtered identically. This also handles some custom sorting of transitions.
Performance of the emojis is questionable, especially when filtering a category and searching text (at the same time). I have some ideas to improve this. However, with no category filter, the proxy model filters extremely fast, and is real-time. Once a category is selected though, it iterates and queries our model (in Python) too much, and is noticeably slow.Fixed the performance with adding the category name to the text, and create a custom regex for Qt to crunch on. Waaay faster!Transform
A major refactor of the Transform tool in OpenShot, related to OpenShot/libopenshot#496:
Screenshot doesn't actually show the new cursors and cursor rotation, but it's cool! Trust me!
Translations
Emoji and Transition translations have been split off into their own POT template file. This is still a work-in-progress, but the idea is to de-prioritize these 2 templates as 'nice to have', but not really part of the 'core' openshot-qt translations. This reduces the # of 'core' translations for a good openshot-qt experience, but still allows for 100% translation for those who really want it.