Re: Submitting SubtitleComposer for KDE Review
On 11/7/21 20:57, Albert Astals Cid wrote: El diumenge, 7 de novembre de 2021, a les 19:05:44 (CET), Mladen Milinkovic va escriure: Have updated the bug report address to bugs.kde.org (default) in KAboutData and project has been added. Should I do something else to complete the process? From my side I think you fixed everything I reported. I'd say wait a week more in case someone else has time to have a look, but if there's no more movement consider the review done and ask sysadmin to finish the move to its final location. great... thanks 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()) I have found a cause of leak in KSelectActionPrivate::init(), it was also causing "QCoreApplication::postEvent: Unexpected null receiver" messages at application shutdown I fixed it inside SC, will submit a patch to invent.kde.org later.. I'm not sure what's the best why to fix it. -- Mladen Milinkovic GPG/PGP: EF9D9B26
Re: Submitting SubtitleComposer for KDE Review
Have updated the bug report address to bugs.kde.org (default) in KAboutData and project has been added. Should I do something else to complete the process? Cheers, Mladen On 10/16/21 00:28, Mladen Milinkovic wrote: On 10/15/21 22:39, Albert Astals Cid wrote: I think you may have dropped k-c-d from the CC, adding it back. I have hit a wrong button again :) El divendres, 15 d’octubre de 2021, a les 22:18:00 (CEST), Mladen Milinkovic va escriure: 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? Seems to be working now :) 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. I think most of those are not your fault, but if you can spend a bit of time investigating won't hurt :) Sure... will do that at some point soonish. 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. Ok, then should open a request at https://phabricator.kde.org/maniphest/task/edit/form/2/ so that a subtitlecomposer product is created :) And done https://phabricator.kde.org/T14947 Cheers, Albert Cheers, Albert Thank you! Cheers -- Mladen Milinkovic GPG/PGP: EF9D9B26
Re: Submitting SubtitleComposer for KDE Review
On 10/15/21 22:39, Albert Astals Cid wrote:
I think you may have dropped k-c-d from the CC, adding it back.
I have hit a wrong button again :)
El divendres, 15 d’octubre de 2021, a les 22:18:00 (CEST), Mladen Milinkovic va
escriure:
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?
Seems to be working now :)
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.
I think most of those are not your fault, but if you can spend a bit of time
investigating won't hurt :)
Sure... will do that at some point soonish.
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.
Ok, then should open a request at
https://phabricator.kde.org/maniphest/task/edit/form/2/ so that a
subtitlecomposer product is created :)
And done https://phabricator.kde.org/T14947
Cheers,
Albert
Cheers,
Albert
Thank you!
Cheers
--
Mladen Milinkovic
GPG/PGP: EF9D9B26
Re: Submitting SubtitleComposer for KDE Review
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
Submitting SubtitleComposer for KDE Review
Hello everyone, I would like to move subtitlecomposer to kdereview. https://invent.kde.org/multimedia/subtitlecomposer https://subtitlecomposer.kde.org/ Subtitle Composer is an app to create, sync, translate, ... text subtitles for videos and media. Thanks -- Mladen Milinkovic GPG/PGP: EF9D9B26
