Submitting SubtitleComposer for KDE Review

[Mladen Milinkovic]

On 10/13/21 23:03, Albert Astals Cid wrote:
> I think the tests are somehow not correctly flagged as tests, running ctest will only run the appstream check and src/tests/test-subtitle, but not test-core-rangelist and the rest.

Fixed in ec9ffba - I'm pretty sure this was working fine at some point in (not so distant) past.


> The first text format change doesn't seem to trigger the "file has changed and we should enable saving" logic. i.e. i have written a new subtitle line that says "HOLA" and saved the subtitle. Now if i select all the text and press the strikeout button, the save button does not get enabled, if i press the strikeout button again, the save button correctly gets enabled.

I believe this was happening sometimes due to "relatively scary valgrind warning" below... looks like it's not happening 
anymore - could you please confirm?


> If i close a video while it's playing, the Play button will still be enabled (if i stop the video it will not)

Fixed in 663d209


> Opening a .srt i just created and editing one of the subtitle lines i get this relatively scary valgrind warninghttps://ghostbin.com/YGnmL

Fixed in 663d209. QUndoStack::push(action) can merge and delete action, in those cases it ended with invalid read 
immediately afterwards.


> When opening an existing .srt, there is a few Subtitle::insertLine calls that end up calling Subtitle::processAction with the if(app()->subtitle() != this) situation. I think all those Actions leak, because you just call redo on them but they are not deleted by anyone, no?

Yes they were leaking fixed them with 911b94b.

There are still some definite leaks after closing application:
  - libfontconfig/QTextDocument (FcFontRenderPrepare https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=655989)
  - QXcbGlxWindow::createVisual() calling radeon_dri.so and amdgpu_winsys_create()
  - Breeze::WidgetStateEngine::registerWidget calling QObject::connect (might be related to 
"QCoreApplication::postEvent: Unexpected null receiver" messages at application shutdown)
  - KF5WidgetAddons (KSelectActionPrivate::init())

There are also some memory errors that seem caused by KIO/KUrlRequester.

There are some possible leaks related to QTextDocument and rendering, will investigate ASAP if it's due to something 
that SC does wrong.

> The "Report bug" incorrectly links tohttps://invent.kde.org/multimedia/subtitlecomposer/-/issues  instead of bug.kde.org

I didn't change the bug report url to bugs.kde.org yet as it doesn't seem possible to file Subtitle Composer bugs there?
Would prefer to change it right before SC gets included there if it's necessary. There are SC binaries that get 
generated pretty often that people are using - I'd like them to have a bug report url they can use to report bugs.

> 
> Cheers,
>    Albert

Thank you!
-- 
Mladen Milinkovic
GPG/PGP: EF9D9B26