New repo in kdereview: kasts

[Harald Sitter]

On Sat, May 29, 2021 at 10:20 PM Bart De Vries <bart at mogwai.be> wrote:
>
> On 27/05/2021 15:09, Harald Sitter wrote:
> > - the source is almost reuse compliant but not quite. needs some extra
> > data files equipped with CC-0 or the like [1][2]. You could then also
> > add an include on
> > https://invent.kde.org/sysadmin/ci-tooling/raw/master/invent/ci-reuse.yml
> > to your gitlab-ci.yml to ensure it stays at complete coverage
>
> We've made all source files reuse compliant with one exception: the dbus
> interface files.  We couldn't figure out which license to apply.
> Any pointers are welcome.

org.freedesktop.PowerManagement.Inhibit.xml can be simply CC0 with no
copyright holder. It's essentially auto-generated introspection data.
example at [1]

org.gnome.SessionManager.xml is a bit more complicated because it
contains exhaustive documentation that could possibly be considered
copyrightable.
I'm guessing it was excavated from gnome-session [2]? If so it's
probably GPL-2+ with gnome as copyright holder, **but** it may make
sense to send a mail to the maintainers to clarify the situation. You
could also replace this file with a rewrite: You really only need a
super trivial definition like
org.freedesktop.PowerManagement.Inhibit.xml, modeling merely the
Inhibit and Uninhibit methods. Neither the documentation nor the other
methods actually serve any purpose here anyway.

[1] https://community.kde.org/Guidelines_and_HOWTOs/Licensing#License_Statements_in_Non-Source-Code_Files
[2] https://gitlab.gnome.org/GNOME/gnome-session/-/blob/master/gnome-session/org.gnome.SessionManager.xml

> > - AudioManager deals a lot with time numbers. it'd aid readability
> > (and probably type safety) if you used std::chrono type and in
> > particular also chrono literals
>
> I'll have to have a closer look at this.  AudioManager is a wrapper
> around QMediaPlayer which is using qint64 for durations and positions.
> As such, the current implementation avoids type conversions by using
> qint64 everywhere for time numbers.
> While I agree that std::chrono might be a more logical choice here, I'm
> afraid it would also introduce a lot more type conversions to the
> underlying QMediaPlayer.

Fair point. I thought there's only the two setters where you interact
with QMP but since you also pass through signals from QMP it gets a
bit more complicated. I would still convert the types to get the
advantages of chrono but I fully understand that this isn't nearly as
neat as I had thought originally. Entirely up to you.

I now noticed your hack in AudioManagerPrivate::prepareAudioStream
though. I would strongly urge you to find another way of dealing with
this. Nested event loops (QEventLoop::exec) have shown to be fairly
crashy in the past, double so for QML application. If any event fires
(e.g. through a timer) that deletes an object or otherwise mutates
expected states out from under the current stack the application will
crash. QEventLoop::exec is really an anti-pattern most of the time
IMO.

HS