Olive: Code Quality

[jonno85uk]

I have been looking through the code and using various tools and have identified various issues. Namely memory leaks and maintainability.

I have forked the project and have created a branch solely to address the issues I have found in https://github.com/jonno85uk/olive/projects/2

The branch can be found here https://github.com/jonno85uk/olive/tree/quality

If there is 1 thing to be taken, I strongly suggest to stop manually allocating memory and use smart pointers https://en.cppreference.com/w/cpp/header/memory
https://wiki.qt.io/Smart_Pointers

[itsmattkc]

Hi, I really appreciate this and am happy to refactor things with your help. If you have anything you need me to change or want to PR I'd be happy to properly look over it.

[jonno85uk]

No problem. To begin with i'm going to look at memory leaks and potential lockups and crashes as these are the things that have bugged me a lot in the past with other NLEs. I would be doing the same for those NLEs except I don't like the UX of any of them.

[itsmattkc]

Crashes are high priority for me so if you run into any, feel free to file a bug report. I haven't been as stringent on memory leaks as I probably should have been, so that would be a great help too. I'm glad you like the software enough to want to help improve it :)

[winfr34k]

Great! I wanted to open an issue on this a couple of days ago but never came around to it.

On this note, there are also a couple of warnings listed when building the software. In general, a couple of things should probably be added to the project to maintain good code quality:

• Determine compiler options and turn them on or off depending on their necessity (maybe even add -Werror?)
• Share your code style so that other people who wish to participate in this project can follow it which should lead to more uniform code in the future and therefore increase readability and maintainability
• Add an automated test suite
• Add some rules to the project so that merges have to go through a linter / static analyser(s) and run tests to ensure stability when new code is merged

As far as I can see, there are currently no tests in this project. I have no experience of using C++ for writing automated tests but I think this could be a worthwhile addition to the code base to help against regressions. The project is quite young so I can see why you didn't include them until now. Generally, tests can help you to structure the code in a clean way and to make sure a new change to the code is not breaking without running manual tests (which will become a very long winded task at the rate that this software is growing).

What are your thoughts on this (@jonno85uk, @itsmattkc)?

[jonno85uk]

@winfr34k
Right now I am using "-std=c++11 -Wextra -Wshadow -Wnon-virtual-dtor -pedantic" and the compiler output is, ahem, noisy.

Right now, to be frank, going through the code more this evening, I think the project has tipped over into the "umaintainable" category. I'll be going over it for most of this weekend and i'll hold my final judgement until then but as of now, i'm not hopeful. Sorry.

[jonno85uk]

To browse the code to see how its intertwined, I highly recommend using Sourcetrail.
https://www.sourcetrail.com/
You'll have to install bear and run $bear make; in order to get the compile_commands.json file required.

[winfr34k]

I really, really hope that this is not "unmaintainable" or that we can get this software out of this state in a community effort. Olive is one of the best FOSS video editors and I would really like the project to succeed.

[jonno85uk]

After a weekend of going through the code base I'm not sure where to go from here.

The project isn't "unmaintainable", at least by the creator. Correct me if I'm wrong but it would seem the author is more familiar with c than c++.

Contributions by the community is achievable but I can only foresee small contributions due to the nature of the code.

I'm trying to be reasonable here, this personal project clearly is a work that means something to the author and has been written with a urge for features and has achieved a lot in a short period of time.

Due to this I've tried to reduce my level of usual review/scrutiny I have become used to in my day job (automotive, misra).

So, no I don't think this project is unmaintainable, just perhaps difficult to sustain.

My work over the weekend looks pretty difficult to merge but I have identified some commits for minor features I've seen requested in other issue tickets.

If someone wants to start testing code I highly recommend starting at the files sequence.h, clip.h, media.h and effect.h

[jonno85uk]

Oh, i should add. The fork/branch I created is in a broken state.

I resisted the urge to not rip up the code for a while and just hunt down the memory leaks but couldn't help myself. I'm currently having issues in project.cpp and not storing raw ptrs in QModelIndex.

So don't use it for now.

[itsmattkc]

After a weekend of going through the code base I'm not sure where to go from here.

It depends, I welcome code improvements and am not opposed to rewrites, however it would probably need to be collaborative or at least I would need active direction (more familiar with C is probably accurate). So it depends how involved you wish to be - if you'd like to make small suggestions and PRs over time or completely overhaul things (which would probably necessitate a semi-feature lock for some time to avoid conflicts).

[jonno85uk]

(which would probably necessitate a semi-feature lock for some time to avoid conflicts).

Perhaps a branch for v1.0 for bug fix only and master-branch for continued feature work, then merge/rebase v1.0 into master at some point and then branch for v2.0. If that makes sense? I'm thinking a linux-kernel like strategy.

[jonno85uk]

Oh well. Seems this isn't a concern of as i'm still seeing the same things happening. I'll close this issue.

I fear you're coding yourself into a corner. All the best.

https://en.wikipedia.org/wiki/Technical_debt