Submitting SubtitleComposer for KDE Review

[Albert Astals Cid]

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.

Cheers,
  Albert

> 
> 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
> 
> 
>