In commit 35bb32c8e21ff8e12a1f7e07378f6e34add6bdc1, I quickly added a mechanism to create a new track.

I think the mechanism will be similar for each object type, so I describe it here:
The intended usage is the following: the user calls the constructor of the object that she wants to construct (for example a track), giving as parameter a pointer to the parent object (here a timeline).
Then the constructed object is responsible for registering itself to the parent object. The registration basically involves performing the MLT operation, as well as some book-keeping (basically track the child pointers).

Note that a similar behaviour will be added for deregistration upon deletion.

Ok, so I start to understand how Qml works with c++ (as implemented in Shotcut). I think this will require a few changes to the current model.
The Qml is a view on a QAbstractItemModel (currently TimelineModel). Each item can be located through a QModelIndex that allows querying the QAbstractItemModel. Clips have a QModelIndex row() property representing the clip's position in a track playlist, while the QModelIndex internalId() is the track number.

When we drag a clip in the Qml view, we get the clip's QModelIndex (so we can easily find the track and playlist's index) and the position where the clip should be moved.

However, in the current model, everything works through the clip id's and it doesn't seem easy to get the id of a clip based on it's position in a track's playlist.

The advantage of the Qml is that there is no duplication of data. The rowCount() and data() methods of the model (as implemented in TimelineModel) directly retrieve infos from MLT's playlists to be displayed, so at this stage, I think it is worth thinking how we can make our model talk efficiently with Qml.

You can check the current pre-alpha timeline2 view from the "View" menu by checking "Timeline2". My code is currently quite messy but I will clean it up once we are sure about the directions we want to take.

Any remarks, comments or ideas on how to improve the model / Qml communication ?

I've finally found time and courage to dive into the QML stuff.

So if I understand correctly, the whole purpose of deriving QAbstractItemModel is to have a tight coupling between Model and View. In particular, that allows to use a QML Repeater to populate the Timeline. It is not clear in my mind how you add new elements (tracks or clips) afterwards, but this should be doable.

My first question is: what prevents us to have the clip unique id (I mean model id), stored as a property of the QML object on creation ? This would allow to make the correct call to the timeline model when required.

My second question is about design. Basically I want to question this idea of using QAbstractItemModel. An orthogonal idea is to create all the elements dynamically through javascript, and then couple the Model and the View using only Signals/Slots mechanisms. What do you think about that? Is it short sighted? My understanding of QML is still a bit shallow so I might be overlooking some issues here.
Here is an example of track creation:

function addTrack() {
    if (trackHeaderComponent == null)
        trackHeaderComponent = Qt.createComponent("TrackHead.qml");

    if (trackHeaderComponent.status == Component.Ready) {
        var dynamicObject = trackHeaderComponent.createObject(trackHeaders);
        if (dynamicObject == null) {
            console.log("error creating block");
            console.log(trackHeaderComponent.errorString());
            return false;
        }
    } else {
        console.log("error loading block component");
        console.log(trackHeaderComponent.errorString());
        return false;
    }
    return true;
}

I have a semi-working implementation of this idea, do you want me to push it so that we can discuss more precisely ? We can always revert the commit if this seems to be a bad idea.

Finally, the last thing I want to question is the separation between the trackHeaders and the actual tracks, which seems to me to be mostly legacy. What exactly prevents us to define a track as a Rectangle, with one sub-Rectangle corresponding to the Header, and one other corresponding to the actual track content? We can then play with appropriate MouseArea to deal differently with the events in both areas.

So after more experimentation, I think that my point about alternative design without QAbstractItemModel is mostly invalid.

Currently, the data organisation is as follows: each track corresponds to a row of the root index (which basically represents the timeline itself). Then each of these row has sub-row corresponding to the clips.
I changed things a bit so as to make the internalId being the unique ID of the element we are dealing with. Storing the trackId is not necessary since we can retrieve it easily (and efficiently).
I also made some modifications to use the model API and not the underlying Melt's functions.

Notice that the commit broke the view. This is expected because shotcut is built with the assumption that a blank is a clip, and our model is built without that. Some tweaking of the QML will be required.

One question: in the current model, the clip position in a track seems to be stored in a private property (m_position) of the ClipModel.
We should definitely avoid this. In MLT, the position of a clip can be retrieved by querying the playlist, using either clip_info() or clip_start() using the MLT row() as argument.
However, in MLT playlists, blanks count as clips, and since you changed the codes, the current model's row does not match anymore with MLT's playlist row. This seems to add some complexity for some simple operations like getting the start position of a clip. Do you have an idea to allow an easy retrieval of a clip's position with the current model?

So the motivation behind this design is that a clip should be able to exist independantly of a track. Firstly, because it may be not inserted yet, and secondely because a TrackModel might soon become more complicated than a Mlt::Playlist (it'll probably become a Mlt::Tractor to handle same track transitions)

In the modification I pushed, I completely step back from mlt/shotcut row model. From what I understand, the row id of a clip should be constant, and all row ids should be consecutive, starting from 0. The latter constrain makes clip ID unsuitable for the task. Therefore I made something up based on insertion order (the relevant fonction is something like getClipFromRow). The order of the rows has nothing to do with a chronological order, but I don't think that matters, since we only need the clip's duration and position to render it.

That being said, retrieving the clip position should be straightforward. Since the internalId of the ModelIndex is set to be the Clip ID, you can pretty much do whatever you want, for exemple m_allClips[id]->getPosition()
Did that answer your question or did I get you wrong ?

Ok, I was able to adapt the view to fix most issues. However, I am not sure the advantages of the current model over directly querying the MLT::Playlist to get a clip position. The thing that bothers me:
As you mentionned, ClipModel->getPosition() does return a clip position in the playlist. However, this position seems to be stored in an internal m_position variable.
So If I insert a blank at the start of a track, it means we have to update the m_position of all clips on the track. With complex operations, there is the risk that the m_position variable of a clip is incorrectly updated and does not reflect the real position of a clip in playlist, resulting in timeline corruption. Or am I missing something ?

That is a valid concern, and I want to make several points about it

• Firstly, when you use the API, you should use it blindly without being concerned about the internals. What's important is to use it consistently (by that I mean avoid manipulating Melt objects in the class they don't belong to). If this is done correctly, and together with the extensive tests I'm writing, it would be straightforward to change the underlying implementation (for eg request a ClipInfo from the timeline)

• Actually, you have to store something about the clip if you want to make a query to melt. Basically, you can query a playlist based on position or on (melt) index. Either of these options have to be updated at some point to reflect the change in the timeline. I chose position because it seems more natural to me, but if you have a case for the other option I'm willing to hear it.

• Lastly, if complex operations are implemented using well tested primitives, that reduces drastically the risk of corruption

Hmm, thanks for the details. For some reason, I was under the impression that storing the playlist position would be less prone to corruption but I might be wrong. Thinking about it, maybe your choice of storing the position is a better choice to later implement groups.

So I think we can try to move forwards, I will try to implement clip move from the timeline view as a first step. We also should quickly integrate the undo/redo framework with the model.

Well the playlist position changes less often (for example if you resize the clip it doesn't, whereas the actual position might), but when it does, it is usually in a rather complicated way (you have to track how many blanks are inserted, and that affects ALL the clips after the position of modification, whereas in normal insert mode their actual position is actually constant)

Ok for the moves. I'll implement it cleanly on the model side and write the tests. From what I've seen, the QML might need to be cleaned up extensively, because most of the operations are done in the backend anyways (for eg. snapping).
Have you thought about my remark about the header organisation ? (one column with all the headers as it is now vs the header integrated to the track)

We can now move a clip in the Qml GUI, which triggers a clip move. However, on undoing, the requestClipMove is not called. Did I miss something?

This function is not meant to be called when undoing. Instead, the lambda function corresponding to the undo movement is called. As far as I can tell, this happens correctly, but then the model has to send a signal "dataChanged" to update the view, which it isn't doing at the moment. I'm on it.

So I tried to fix the problem, but failed so far (see lasts commits).
The problem is actually multifold:

• Firstly, I've changed the code to force a call to the model whenever a drag is made. This ensures that we can't move in a forbidden position (ideally, the view should move the clip by itself, but instead should wait for the signal from the model, which is not currently the case). Of course, I've disabled the creation of the Undo item for these intermediate moves, but that leaves a problem: when we drop the item, a final call to moveClip is issued, this time creating a Redo. But in that case, the move operation applied by the model is a move from the last dragged position to the final position.

Here is a example : Suppose we have a clip in position 13, we want to move it to 10. We drag it slowly, when we reach 12 a move order is issued (13->12), but without redo (we are still dragging), then we move 12->11 without redo, and finally, we drop in 10, so the model executes 11->10 and is told to store the redo of this move. Obviously, when we Undo that, we go back to 11 and not 13 as we would have wished. I hacked around this by issuing a fake order 11->13 and then the real 13->10 (whith redo enabled). I'm not fully satisfied with this, suggestions welcome.

• I've implemented the signal mechanism to tell the view that the model has changed. This is very preliminary, and not expected to work when dragging a clip accross tracks, but should work when we drag it on the same track. Note that I had to implement a custom property "modelStart", because when model.start was bound only to clip.x, updating model.start would not update the position of the clip. So now moving a clip on the same track and then undoing works, but it fails if the track changes. In that case, we have to call the functions related to row addition/deletion (since the clip is deleted from the FromTrack's row and added into the ToTrack's row). But this actually creates a problem, because if we do so the QML Clip element that we are dragging gets deleted. Any idea how to workaround this ?

I don't think we should move the clip in MLT's playlist when dragging. There should be a canBeMoved(int position, int length) on the track that returns true if the playlist is blank from position to position+length.
It's easy to get that info from an MLT playlist using is_blank_at(position) and blanks_from(int clip, int bounded). That might solve the 2 issues you describe. I can work on it a bit tonight.

I tried to implement it as you describe (I had "dry" operations that you can find in git history). But you cannot rely on Melt only. Example : you have a 100 frames clip and you move it 10 to the right. If you call is blank at on the target position, it will be false because the "old" clip is still there. Of course, for one clip mouvement, you can easily work around this, but when you try to move a group things get way more complicated...
In the end, we will end up simulating the mouvements internally, which seems to be over-engineering and corruption prone.
That's why I took the undo approach instead : I assume that the action will succeed until it fails, and if it does, I revert what has already been done using the undo lambda constructed on the fly

I just committed my test version of the allowClipMove function. I really think it is better to ask if space is available on drag rather than really perform an MLT move on every drag move.
The way I make it work should not be too hard to adjust for groups.

I pass the position, length and clip id. Then, we check in the track what space is available, and the clip whose id was passed in the allowClipMove counts as an empty space. When moving a group, we would need to pass a list of ids from the clips that are moved, and all those clips will be considered as blanks.

I agree that with groups, we will have to somehow simulate the move, but it still seems better to me, and the real operation is anyways validated so I don't think it can lead to corruption, but I will test and think about it...

To be precise, it cannot lead to timeline corruption, but there can be a discrepancy between what you are allowed to do vs what you should be allowed to do.

Groups were just an example (easy to pick because already implemented), but this will arise for every single operation that we implement. Complexity is not our friend here

Hmm, maybe you're right, we can try a bit more with the "do it for real" approach. Then we can revert my tests. I won't be able to work on it today but tomorrow I will be there. One thing that is currently problematic with how you implemented it is that - since it performs an operation on the model - it refreshes the view on every drag move, causing flicker and unnecessary redraw of the whole track. So I think the "update" signal should only be sent on mouse release.

So, I've tried hard to implement the "do it for real" approach, but I think it is not possible to solve the problem involved by track change: since the clip gets deleted and reinserted in the other track, the QML object disappear, and that wrecks QML's mouse event system.
The only way I found to circumvent that is to handle all the mouse interactions at the timeline level: you receive a click, find the clip it corresponds to, and when the mouse move you send the requestClipMove, so that the clip appears to "follow" the mouse. It would work for clip movement, but then it'll become really cumbersome for all the mouse interactions. For example, if you want to highlight the clip's handle when the mouse hovers it, it is extremely difficult.

One of the problems I think is that QML cannot deal with "partial" events: for example you'd want the clip object to deal with the click event (Register something like "I'm the clip about to be moved") and then the mouse move should be handled by the timeline object (because it oversees all the possible positions of the clip). That doesn't work because when the clip handles the click, it takes ownership of the rest of the mouse interaction until mouseReleased (at least I didn't find a way to make it otherwise).

Hence, unless you have some input about this issue, I'd advocate to use your simulation approach for movement :)

That being said, movement may be the only tricky case, because AFAICT it is the only one that can involve multiple tracks. So we may be able to use the "do it for real" approach for the other interactions (which I think is a good thing)

I just committed a first draft to allow dragging a clip from bin into timeline. I have 2 problems:

• it breaks the tests, with a warning in the linker that I don't know how to solve
• in timelinemodel.cpp, I added a method, TimelineModel::insertClip that has to use ClipModel::construct. I am not sure how I am supposed to pass the <TimelineModel> pointer, currently passing it from the timelinewidget but I guess there is a cleaner way..

I fixed the insertClip method and incorporated it better in the undo/redo framework. I had to fix some issues on the way, but everything should be fine now.

To reduce unecessary coupling between classes, I removed the pointer to BinController in the TimelineModel, because what it needs is only a producer. So the BinController is stored in the timelineWidget, which is not much better, but this bin management will need to be refactored later.

I also started to implement same-track transitions. Specifically, I replaced the playlist inside TrackModel by a Mlt::Tractor with two playlists. This works fine from the back-end perspective (tests are still passing) but for some mysterious reason breaks the display of the tracks in the QML. The clips render fine but not the tracks. Any idea why?

To try to avoid the same kind of huge class that we had in the previous version of the timeline, I separated all the methods related to QML interaction from the model. That creates a new class TimelineItemModel.
What do you think of such a split ?

We will need some kind of connection between the Bin Clip (ClipController) and the ClipModel that is in timeline. For example, some properties are stored in the Bin clip (master clip) and don't need to be stored inside every instance of the clip in the timeline.

I was thinking about storing a pointer to the ClipController in ClipModel.cpp. For example, in
ClipModel::construct, instead of using MLT::Producer, use the ClipController. This would allow us to access all information / data for a clip from ClipModel. Does that seem ok to you ?

What kind of data are you talking about?

A pointer could be a solution, even though I'd prefer an ID based system. However, what you describe works for clips that are dropped on the timeline, but what about other ways to create clips? For example, when loading a project, we only have the producers, how do we figure which ClipController they correspond to ?

I've pushed a preliminary implementation of the construction of the timeline when we load a project. It seems to be working. Can you review what I did in ProjectManager? I was not sure it was the correct place to implement that logic.

About the relation between Bin clips (currently managed in ClipController) and timeline clips, here are some more infos:

In Kdenlive, the Bin clips are stored in the "main_playist" of MLT. When dragging a clip from the bin to the timeline, we create a cut of the bin producer and insert it in timeline.

Working with cuts from the Bin Clip (also called master clip in Kdenlive), has the big advantage that if you apply an effect on the Bin clip (master producer), the effect automatically applies on all its cuts. The problem with this approach is that there is a bug in MLT/FFmpeg that causes issues when you try to mix audio between 2 cuts of the same producer. It causes audio glitches. To avoid that, we in fact created "track producers" for each clip, but this really complicates the logic and I would prefer to avoid that. This issue only relates to clip with audio, so image, color and title clips are unaffected.

Shotcut on the other end creates a new unrelated copy of the producer for each timeline instance. But then it means that when applying or editing an effect on the bin clip, we need to automatically apply it to all instances in timeline.

So we need a way to trigger an event on all instances of a clip in timeline. Same situation when enabling/disabling proxy for a clip. We need to replace all instances of a clip in timeline with another producer.

ClipControllers can be build at load time, from the main_playlist, and then a property of the timeline clip (kdenlive:id) can identify the clipcontroller responsible for it.

ClipControllers usually store the clip name, markers, the clip type (Title, Image, Slideshow,Video, ...).

I had a quick look at your code in ProjectManager, seems ok to me at this point.