Timeline cleanup and performance #1864

DylanC · 2018-07-16T22:12:03Z

Attempting to upgrade Angular safely and remove any dud code. Also hope to make some performance improvements.

DylanC · 2018-07-17T07:51:51Z

Comment from Ferdnyc on #1859

Still unsure is using JS here a great idea. I had thoughts of a C++ timeline today or Python but it seems like a huge project to do a replacement on what has been implemented already.

nod Tho the question then becomes, in C++ or Python with what toolkit? Qt I guess is the obvious answer because it's what the rest of the code is built in, but TBH building the Timeline in traditional, QWidget-based Qt... does sound like kind of a chore. I definitely can see reasons for going down the Angular+jQuery JS path, rather than attempting a QWidget-based implementation. I can't imagine that'd be very enjoyable at all.

One thing that I think might be really interesting to explore, though, is the idea of a Timeline built using a more modern Qt approach. Interface building in Qt today isn't just the QObject/QWidget-based toolkit, they also offer QML for markup-based interface design. Which I imagine might offer a fairly painless migration/conversion from the current Angular-based Timeline... in fact, like everything else this decade, QML also has a JavaScript API to access pretty much everything.

I know you said you're not sure JavaScript is the right approach... but right now the Timeline implementation is external to the entire rest of the interface (and they're totally disconnected from each other, as a result) so both sides wind up jumping through a lot of hoops to achieve even the current (low)
level of integration. Whereas the impression I've gotten is that a QML Timeline would be a fully integrated component right from the start. (Hopefully that'd mean far less sprawling JavaScript code, and less of it too.)

I know nothing about QML other than what I've read, but Qt seem to treat it as The Future™ most of the time... though how much of that is justified, and how much is just marketing telling people what they think they want to hear, I know too little about QML to even hazard a guess.

Wikipedia says...

Qt/C++ integration

QML does not need Qt/C++ knowledge to use and it can be easily extended via Qt.[28][29] Any C++ class derived from QObject can be easily registered as a type which can then be instantiated in QML.
Familiar concepts

QML provides direct access to the following concepts from Qt:

    QObject signals – can trigger callbacks in JavaScript
    QObject slots – available as functions to call in JavaScript
    QObject properties – available as variables in JavaScript, and for bindings
    QWindow – Window creates a QML scene in a window
    Q*Model – used directly in data binding (e.g. QAbstractItemModel)[30][31][32]

DylanC · 2018-07-17T07:59:09Z

@ferdnyc - Makes more sense to continue the conversation here instead.

nod Tho the question then becomes, in C++ or Python with what toolkit?

I did mean to say with Qt on this. So it would be C++ with Qt5 or PyQt5.

TBH building the Timeline in traditional, QWidget-based Qt... does sound like kind of a chore

Yes, I was thinking of a GraphicsView widget and drawing QRects for the clips. The more I was thinking about it the more it sounded like a horrible task and something else would fare better.

Whereas the impression I've gotten is that a QML Timeline would be a fully integrated component right from the start. (Hopefully that'd mean far less sprawling JavaScript code, and less of it too.)

Yeah I feel maybe QML is the best fit in terms of easy coding and reducing the code to support the bridge between Qt and JS. At the moment when I try to work on the timeline code it just seems overwhelming and I always feel like starting again from scratch but that would be even more work and probably a harder task.

If I could draw out and document the requirements of everything the timeline has to support then maybe starting a new timeline implementation would be best.

ferdnyc · 2018-07-19T07:43:02Z

Yes, I was thinking of a GraphicsView widget and drawing QRects for the clips. The more I was thinking about it the more it sounded like a horrible task and something else would fare better.

Yeah, I think the win there is that Angular+jQuery at least bring a scene graph and nice, highly advanced animation features. Reading more about QGraphicsView, it does apparently have its own scene graph (QGraphicsScene) and shape primitives like QGraphicsRectItem and QGraphicsPathItem, as well as QGraphicsTransform for animation effects, so maybe it's more capable than I realized.

But still, as much as some users may dislike them 😉 , I have to wonder if even something as simple as drawing the clips with rounded corners would be nearly as easy in QGraphicsView as it is in the current timeline code, where all it requires is the border-radius attribute in the timeline CSS:

.clip_top { 
  display: -webkit-flex; 
  background: -webkit-gradient(linear, left top, left bottom, color-stop(0%,rgba(255,255,255,0.20)), color-stop(100%,rgba(255,255,255,0))); 
  border-radius: 8px; 
  overflow: hidden; 
}

There are also a lot of little animations we take for granted. (The snapline when Clips are in alignment, the scrolling of the Playhead when the user clicks the ruler, the scrolling of the snapline from one alignment point to another, etc...)

At the moment when I try to work on the timeline code it just seems overwhelming

I hear that. I went hunting for #1454 (the clip-resizing display bug), and I came away utterly confused about everything that was going on in there, but without any fix for that bug. I know I found the code that must contain the bug, I just couldn't find the bug in the code, mostly because I didn't understand half of it. But that bug even shows up in the Timeline debug interface (opening src/timeline/index.html in Chrome), so I know it's a JS bug.

I also went looking for my own #1383 (about the fact that Markers are draggable, and add their icon as an image clip when dragged to the Timeline), which should be a really elementary thing, but I'll be damned if I could figure out why the things are considered draggable, or how to make it stop. They're not draggable in the debug interface, so that makes it even weirder. Though, I do notice that the cursor changes to a hand when you attempt to drag them, which feels like a clue.

P.S> QGraphicsView vs. QT Quick / QML on the QT Blog. (It's actually titled "Should you still be using QGraphicsView?", but having read it I feel like a more accurate title would be "In defense of QGraphicsView", since that's how it comes off.)

DylanC · 2018-07-20T22:06:54Z

@ferdnyc

There are also a lot of little animations we take for granted. (The snapline when Clips are in alignment, the scrolling of the Playhead when the user clicks the ruler, the scrolling of the snapline from one alignment point to another, etc...)

Yeah I think for now on the whole a move to something else would be too much change too soon.

P.S> QGraphicsView vs. QT Quick / QML on the QT Blog. (It's actually titled "Should you still be using QGraphicsView?", but having read it I feel like a more accurate title would be "In defense of QGraphicsView", since that's how it comes off.)

This makes it a bit more confusing as to which is better in this case. 😆

Soo.. just a quick update. I've updated Angular to the latest version and like before this has broken the dragging behavior. You can drag a clip after selecting it first but not if you just click and start dragging right away. I've seen this in my previous attempt to upgrade Angular.

However, since we also include Jquery and Jquery UI it could possibly be something in there. I do suspect its more an Angular issue but my (fun) investigations will continue..

DylanC · 2018-07-20T22:29:37Z

@ferdnyc - Fixing dragging wasn't bad and I feel like I've fixed this before. Next up is snapping (oh dear lord). Clip snapping seems to completely not work now but where there is a will there is a way. 🤣

DylanC · 2018-07-21T14:09:31Z

@ferdnyc - Is there a chance you could test this if you have time? I want someone to test and maybe find something broken before I merge this PR. Seems to work fine inside Openshot and in Chrome from my testing so far.

ferdnyc · 2018-07-21T16:01:33Z

@DylanC Sure, I'll give it a spin in the next hour or two.

ferdnyc · 2018-07-21T16:36:05Z

@DylanC — LGTM! Very cool. I couldn't really detect any differences from 2.4.2 release, which I gather is the goal (at least so far)? All the same functionality, all the same bugs. 🤣

(And I just found another one, or at least it's new to me: When you have a locked track and you're dragging clips up and down between tracks, you don't have to try very hard to get a clip to "land" on the locked track and stay there — it's really still on the track it moved from, according to its Properties, it only appears to be on the locked track. But looks like the drag auto-reset on a refused drop sometimes doesn't fire, or can get interrupted by new drag activity before it has a chance to restore the original position.)

But like I said, that's only new to me — exact same behavior as 2.4.2 release, so nothing to do with this PR at all. Everything here looks A-OK from where I'm sitting, in both the debug timeline and the OpenShot interface. 👍

ferdnyc

(LOL. I'm submitting a review, as if I can even really read Angular code. Don't believe the lies.)

ferdnyc · 2018-07-21T16:38:48Z

src/timeline/js/controllers.js

-	// {
-	// 	timeline.qt_log(layer);
-	// }
- };


What will we do without this function??

(Which reminds me, one of us should just delete src/timeline/media/js/debug.js entirely, as it contains equally critical code.)

@ferdnyc - Yup, I considered deleting that multiple times already. Might as well just get rid of it.

@ferdnyc - That debug file has been removed btw.

ferdnyc · 2018-07-21T16:39:05Z

src/timeline/js/controllers.js

-		   },
+	    num : 24,
+	    den : 1
+	   },


Die, tabs, die!

(BTW, this still doesn't quite line up, since lines 36-39 start with a single tab and then switch to spaces, while 35 and 40-44 are indented entirely with spaces. But, honestly, this file's tabs-vs-spaces insanity is... clinical, so maybe that's a whole 'nother PR of its own.)

@ferdnyc - No worries, I will fix it before merging if possible.

ferdnyc · 2018-07-21T16:46:37Z

src/timeline/js/directives/clip.js

-
-				},
-
+				}


TBH, I don't hate trailing commas on multi-line (multi-page) lists like this. It's not illegal, and it saves you when someone adds another member after the last one and forgets to append the comma. But removing it keeps 'em honest, I suppose.

@ferdnyc - I'd only leave it there if its very likely we will be adding something soon. IMHO.

ferdnyc · 2018-07-21T16:51:11Z

src/timeline/js/directives/clip.js

@@ -421,8 +407,9 @@ App.directive('tlMultiSelectable', function(){
 						item = findElement(scope.project.effects, "id", id);
 					}

-					if (scope.Qt)
+					if (scope.Qt) {


Ugh, THANK YOU. Conditional blocks without enclosing braces are just bug magnets, they're practically taunting anyone who looks at the code. "Try to add another instruction to me, screw up the program logic!"

@ferdnyc - Yep, its a personal preference of my own too. I HATE when people do single line if statements. Its just not consistent with the rest of them and yeah it can be a magnet for future bugs.

DylanC · 2018-07-23T10:22:22Z

LGTM! Very cool. I couldn't really detect any differences from 2.4.2 release, which I gather is the goal (at least so far)? All the same functionality, all the same bugs

Good to hear. Tbh yeah I think that is all I'm aiming for with this one. Its to make important upgrades for the timeline and just remove some cruft. Should give us a more solid foundation for future changes. I'm planning to tackle some individual bugs with separate PR's.

DylanC · 2018-07-23T15:47:49Z

@ferdnyc - Getting this change in. I'm planning to tackle some timeline bugs in my next PR's. Do you know of any issue numbers worth getting attention?

ferdnyc · 2018-07-23T16:39:52Z

The two I mentioned, #1454 and #1383, are both definitely Timeline code bugs, so that's a start.

There's also #1839, which raised an issue with the ruler that isn't present in the debugger, so that's something going on with OpenShot embedding the Timeline canvas... but it's still related.

Oh, and also on the "straddling the Timeline JS and OpenShot's Python" front, in the course of discussing one issue the conversation got around to the Timeline's current keyboard-/multi-selection functionality which is, to put it mildly, a bit lacking and weird, so that could really use sanity-checking and standardizing.

Oh, and the super-ambitious goal, of course: All those various situations where the Timeline should be autoscrolling to follow the Playhead, but instead just lets it wander out of view.

I'd also throw in, though it wasn't the original focus of #1587, the things that could easily be simple clickable controls on the Timeline, but are currently accessible only from the context menu. The obvious low-hanging fruit on this front is the Locked Track control. When you lock a track, a padlock symbol gets added to the track label section, but the only way to actually perform the lock/unlock operation is via the menu. It seems like a no-brainer that the padlock should be clickable to unlock the track, at a minimum. Or, even better, always visible, and clickable to toggle between locked and unlocked states, so you don't need the context menu at all just to perform the simple act of flipping the state of a boolean property.

...That's just off the top of my head. 👼

DylanC added 3 commits July 16, 2018 22:51

Upgrade Angular to latest 1.x version

4677465

Remove unused function

1f61f27

Fix playhead position undef error

a65ff1d

This was referenced Jul 19, 2018

PyQt or 'Qt for Python' (PySide2)? #1875

Closed

WIP: Timeline style refresh #1883

Closed

DylanC added 2 commits July 20, 2018 23:24

Fix the dragging not working right away

c0f5722

Merge branch 'develop' into develop

af4db7c

DylanC added 2 commits July 20, 2018 23:48

Fix snapping as clips are not initialized unless selected already

a272087

Minor code tidy - no changes

71be242

ferdnyc approved these changes Jul 21, 2018

View reviewed changes

Remove unused debug file

d64c559

DylanC changed the title ~~WIP: Timeline cleanup and performance~~ Timeline cleanup and performance Jul 23, 2018

DylanC added 3 commits July 23, 2018 11:28

Merge branch 'develop' into develop

814522c

Merge branch 'develop' into develop

e46f1e1

Merge branch 'develop' into develop

bc83b22

DylanC merged commit 6a09742 into OpenShot:develop Jul 23, 2018

DylanC mentioned this pull request Aug 26, 2018

Improve waveform display rendering #2029

Merged

ferdnyc mentioned this pull request Sep 30, 2018

v2.4.3 will not start #2144

Closed

ferdnyc mentioned this pull request Jul 22, 2019

QWebkit to QWebEngine Conversion #2512

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Timeline cleanup and performance #1864

Timeline cleanup and performance #1864

DylanC commented Jul 16, 2018

DylanC commented Jul 17, 2018 •

edited

Loading

DylanC commented Jul 17, 2018

ferdnyc commented Jul 19, 2018

DylanC commented Jul 20, 2018

DylanC commented Jul 20, 2018

DylanC commented Jul 21, 2018

ferdnyc commented Jul 21, 2018

ferdnyc commented Jul 21, 2018

ferdnyc left a comment

ferdnyc Jul 21, 2018

DylanC Jul 22, 2018

DylanC Jul 23, 2018

ferdnyc Jul 21, 2018

ferdnyc Jul 21, 2018

DylanC Jul 22, 2018

ferdnyc Jul 21, 2018

DylanC Jul 22, 2018

ferdnyc Jul 21, 2018

DylanC Jul 22, 2018

DylanC commented Jul 23, 2018

DylanC commented Jul 23, 2018

ferdnyc commented Jul 23, 2018

Timeline cleanup and performance #1864

Timeline cleanup and performance #1864

Conversation

DylanC commented Jul 16, 2018

DylanC commented Jul 17, 2018 • edited Loading

DylanC commented Jul 17, 2018

ferdnyc commented Jul 19, 2018

DylanC commented Jul 20, 2018

DylanC commented Jul 20, 2018

DylanC commented Jul 21, 2018

ferdnyc commented Jul 21, 2018

ferdnyc commented Jul 21, 2018

ferdnyc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DylanC commented Jul 23, 2018

DylanC commented Jul 23, 2018

ferdnyc commented Jul 23, 2018

DylanC commented Jul 17, 2018 •

edited

Loading