- Add clips to the project bin
- Select two or more clips
- Drag them to the timeline
- Check that the "Insert Clips" entry got added to the undo history
- Undo the change
- Redo the change
Details
Details
Diff Detail
Diff Detail
- Repository
- R158 Kdenlive
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
Thanks for the diff, it looks good to me, minus some small details that I pointed out inline.
I take this opportunity to mention that we have unit tests in the project (in tests/). They are run using ./runTests. Feel free to add a test for your function if you feel like it :)
| src/timeline2/model/timelinemodel.hpp | ||
|---|---|---|
| 321 | Could you actually move that to TimelineFunctions? The goal is to keep only the "minimal" functions in TimelineModel, so that it does not grow too big. So if your function can be written as a composition of functions that are already in the TimelineModel, then it should go to TimelineFunctions :) | |
| src/timeline2/view/timelinecontroller.cpp | ||
| 1523 | No need to call clipids.toSet() here, just iterate on the list. Even better, I think that you can construct copy the data directly like that: std::unordered_set<int> theSet(clipIds.begin(), clipIds.end()); | |
Comment Actions
I will fix the unordered_set instantiation and add unit tests, thanks for reviewing.
| src/timeline2/model/timelinemodel.hpp | ||
|---|---|---|
| 321 | That was my initial implementation, but since I have to add the "Insert Clips" to the undo stack, the function needs to be in a class where PUSH_UNDO can be called. Maybe the undo stack could be turned into a singleton accessible from everywhere? | |
| src/timeline2/view/timelinecontroller.cpp | ||
| 1523 | Oh, nice, let me change that. | |
| src/timeline2/model/timelinemodel.hpp | ||
|---|---|---|
| 321 | When using it externally, you can simply use pCore->pushUndo | |
Comment Actions
- Merge branch 'refactoring_timeline' of git://anongit.kde.org/kdenlive
- Merge branch 'refactoring_timeline' of git://anongit.kde.org/kdenlive into drag_multiple
- Move the multiple clip insertion to TimelineFunctions
Comment Actions
Oh, my bad sorry. I should have given some feedback on this diff. Since I was rather stressed with the perspective of a 18.04 launch and there was no activity on this since november, I fixed the problem in this commit on the 2nd of february :
https://cgit.kde.org/kdenlive.git/commit/?h=refactoring_timeline&id=5ecd42394298cad227613e2db4711fa837f148c0
However my version is buggy because it doesn't correctly handle the case where one clip fails to insert (undo not manged globally like yours). So I think I will revert my commit and we can go with your versions!
I have a few comments though:
- requestClipsGroup does not seem to be used, I think we can discard it
- When grouping the clips after a multiple insert, we should use the GroupType::Selection I think: m_model->requestClipsGroup(theSet, false, GroupType::Selection);
Let me know what you think of this proposal and thanks for your work
Comment Actions
No worries, I was also busy with other stuff and took me a while to work on the points that alcinos requested. I still haven't found the time to write the tests he requested.
However my version is buggy because it doesn't correctly handle the case where one clip fails to insert (undo not manged globally like yours). So I think I will revert my commit and we can go with your versions!
I have a few comments though:
- requestClipsGroup does not seem to be used, I think we can discard it
It is being used in TimelineController::groupClips() in this change, but I think it is used in other parts of the code too.
- When grouping the clips after a multiple insert, we should use the GroupType::Selection I think: m_model->requestClipsGroup(theSet, false, GroupType::Selection);
That makes sense. I will change it.
Let me know what you think of this proposal and thanks for your work
No problems, glad to help :)
Comment Actions
- Move the multiple clip insertion to TimelineFunctions
- Use the GroupType::Selection when grouping clips while inserting
Comment Actions
Oh I think I made a copy/paste error in my last message. I meant that:
- groupBeingDroppedId is not used in timeline.qml and should be removed
I am reverting my previous patch so that you can commit soon. Thanks
Comment Actions
Seems fine for me. You just need to rebase against current head since I made a few changes to TimelineFunctions, but for me it's ok to commit, would be nice to get that patch in. Alcinos, ok for you ?
Comment Actions
- Move the multiple clip insertion to TimelineFunctions
- Use the GroupType::Selection when grouping clips while inserting
- Remove unused group ID from QML.