Re: New repo in kdereview: kasts

2021-05-27 Thread Carl Schwan 
Le jeudi, mai 27, 2021 11:05 PM, Albert Astals Cid  a écrit :

> El dijous, 27 de maig de 2021, a les 12:20:20 (CEST), Bart De Vries va 
> escriure:
>
> > Hello everyone!
> > I would like to move kasts to kdereview.
> > https://invent.kde.org/plasma-mobile/kasts 
> > https://invent.kde.org/plasma-mobile/kclock
> > Kasts is a podcast app for Plasma Mobile. It was started as a fork of 
> > Alligator, the Plasma Mobile feed reader. It currently features a play 
> > queue, play position resume/persistence, MPRIS2 support, and suspend 
> > inhibition.
>
> The left bar is super confusing on the desktop, the 4 first elements are 
> "toggles" but Settings and About are not, so if you do About, Settings, 
> About, Settings, About, Settings, About, Settings, Download.
>
> You don't end up in Download, you're still in Settings, and what's worse, now 
> you have to press back like 10 times to remove all those About and Settings 
> pages that are on the stack.
>
> Import podcast defaulting to Planet KDE is a bit weird?
>
> text: (!isQueue && entry.queueStatus ? "· " : "") + 
> entry.updated.toLocaleDateString(Qt.locale(), Locale.NarrowFormat) + 
> (entry.enclosure ? ( entry.enclosure.size !== 0 ? " · " + 
> Math.floor(entry.enclosure.size / (1024 * 1024)) + "MB" : "") : "" )
>
> is a huge text puzzle that needs to be an i18nc call (or many if needed) so 
> translators can rearrange things in the order that make sense in their 
> language + translate MB

That should probably use KFormat instead because it already handles the i18n 
stuff and will also use the correct size units for bigger/smaller downloads 
size.

> Cheers,
> Albert
>
> > Kind regards,
> > Bart De Vries

Re: Qt5PatchCollection versions (broken QWE)

2021-05-27 Thread Albert Astals Cid 
El dijous, 27 de maig de 2021, a les 6:10:47 (CEST), Harald Sitter va escriure:
> On Wed, May 26, 2021 at 7:39 PM Antonio Rojas  wrote:
> >
> > El miércoles, 26 de mayo de 2021 13:19:34 (CEST), Harald Sitter escribió:
> > > Hola
> > >
> > > QWE and QtScript in our kde/5.15 branches currently have unaligned 
> > > versions.
> > >
> > > QWE 5.15.4:
> > > https://invent.kde.org/qt/qt/qtwebengine/-/blob/kde/5.15/.qmake.conf
> > >
> > > QtBase 5.15.3:
> > > https://invent.kde.org/qt/qt/qtbase/-/blob/kde/5.15/.qmake.conf
> > >
> > > This then breaks version alignment expectations. For example QWECore
> > > requires QtWebChannel at the same version but since the versions are
> > > misaligned the requirement check changes.
> >
> > This is because the upstream QWE and QtScripts 5.15 branches are public,
> > unlike the other Qt repos. The versioning issue is discussed here:
> >
> > https://www.qt.io/blog/building-qt-webengine-against-other-qt-versions
> >
> > Lowering the version number would be misleading since this is the real,
> > upstream 5.15 code, which is post-5.15.4 already.
> 
> I suppose then we need to fix the cmake check?

That would be one of the patches that would make some sense even if not coming 
from upstream i guess.

Cheers,
  Albert

> 
> Right now building the lot of our patch collection branches leads to
> skrooge not being able to build because of that defect. That seems
> silly at the best of times.
> 
> HS
>

Re: New repo in kdereview: kasts

2021-05-27 Thread Albert Astals Cid 
El dijous, 27 de maig de 2021, a les 12:20:20 (CEST), Bart De Vries va escriure:
> Hello everyone!
> 
> I would like to move kasts to kdereview.
> 
> https://invent.kde.org/plasma-mobile/kasts  
> 
> 
> Kasts is a podcast app for Plasma Mobile. It was started as a fork of 
> Alligator, the Plasma Mobile feed reader. It currently features a play queue, 
> play position resume/persistence, MPRIS2 support, and suspend inhibition.

The left bar is super confusing on the desktop, the 4 first elements are 
"toggles" but Settings and About are not, so if you do About, Settings, About, 
Settings, About, Settings, About, Settings, Download.

You don't end up in Download, you're still in Settings, and what's worse, now 
you have to press back like 10 times to remove all those About and Settings 
pages that are on the stack.

Import podcast defaulting to Planet KDE is a bit weird?

text: (!isQueue && entry.queueStatus ? "·  " : "") + 
entry.updated.toLocaleDateString(Qt.locale(), Locale.NarrowFormat) + 
(entry.enclosure ? ( entry.enclosure.size !== 0 ? "  ·  " + 
Math.floor(entry.enclosure.size / (1024 * 1024)) + "MB" : "") : "" )

is a huge text puzzle that needs to be an i18nc call (or many if needed) so 
translators can rearrange things in the order that make sense in their language 
+ translate MB

Cheers,
  Albert

> 
> Kind regards,
> Bart De Vries
> 
>

Re: New repo in kdereview: kasts

2021-05-27 Thread Harald Sitter 
On Thu, May 27, 2021 at 12:20 PM Bart De Vries  wrote:
>
> Hello everyone!
>
> I would like to move kasts to kdereview.
>
> https://invent.kde.org/plasma-mobile/kasts  
> 
>
> Kasts is a podcast app for Plasma Mobile. It was started as a fork of 
> Alligator, the Plasma Mobile feed reader. It currently features a play queue, 
> play position resume/persistence, MPRIS2 support, and suspend inhibition.

Hey.
It's amazing. Also in incredible shape, well done!

Some stuff I've noticed:

- 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

- your appdata file has no screenshots defined

- since you require qt 5.15 you can use Qt::Core etc. targets instead
of Qt5::Core. this will make porting to qt6 less work

- the page stack between Settings and About is a bit wonky. they both
push onto layer stack rather the regular stack (I guess?) which seems
not exactly ideal considering from the way these two are presented in
the UI they should behave same as Queue and Episodes etc..
particularly problematic is however that if you switch between
settings and about you keep growing the page stack and the only way to
get back to the "real page" is to manually unwind the stack again

- also on the subject of the navigation pane: I believe the current
page ought to be highlighted in the navigation pane. when I'm drilled
down into a subpage of e.g. the Episodes page I wouldn't know that
this is where I came from other than through unwinding the stack
manually again

- something is wonky with the colors on desktop. in all listviews it
looks like the delegates are inactive/disabled colors for some reason.
e.g. https://i.imgur.com/xkgigs4.png

- I'm not super certain but also on that same page you might need to
turn the entire "by %1" sequence into a translatable string rather
than just "by". I don't think you can presume that every language has
that particular order? you might want to ask on the kde-i18n-doc
mailing list.

- the generic entry delegate hardcodes size formatting, this results
in my system usually talking about MiB everywhere but kasts is talking
about MB (yet indeed it still means MiB). You'll want to use kformat
[3]

- you may want to do a pass over all strings. I'm seeing some action
strings not using title capitalization (e.g. the actions in
GenericEntryDelegate.qml  - "Add to queue") [4]

- I see you are hardcoding a color in GenericHeader.qml, I'm not super
sure that will work correctly across different themes but then I also
don't really see the color actively used :shrug:

- there is a `// TODO: implement` in the main.qml. you might want to do that ;)

- AudioManager::timeString probably can be replaced by KFormat::formatDuration

- AudioManager has lots of commented out qdebugs. I suggest to either
remove them, or turn them into qCDebugs (so they may be
enabled/disabled at runtime)

- 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

- AudioManager uses some "old style" stringy connects
`loop.connect(, SIGNAL(timeout()), , SLOT(quit()));` these
SIGNAL() and SLOT() bits are only evaluated at runtime. To have the
compiler ensure the functions are valid I would very strongly suggest
to change them to function pointers like in the other connect calls.

- AudioManager also has some singleShot(0, lambda) calls. I believe
these are kind of unsafe since `this` might get deleted between the
call and the execution. you'll want to contextualize them
`singleShot(0, this, lambda)`

- QueueModel has an m_audio member that isn't ever set or used from
what I can tell

[1] https://apachelog.wordpress.com/2021/04/06/reuse-licensing-helper/
[2] 
https://community.kde.org/Guidelines_and_HOWTOs/Licensing#License_Statements_in_Non-Source-Code_Files
[3] 
https://invent.kde.org/plasma/plasma-nm/-/blob/9005f2580c37d5dbf32f1d264eb87d73ca723829/applet/contents/ui/ConnectionItem.qml#L258
[4] https://develop.kde.org/hig/style/writing/capitalization/

HS

New repo in kdereview: kasts

2021-05-27 Thread Bart De Vries 

Hello everyone!

I would like to move kasts to kdereview.

https://invent.kde.org/plasma-mobile/kasts  


Kasts is a podcast app for Plasma Mobile. It was started as a fork of 
Alligator, the Plasma Mobile feed reader. It currently features a play queue, 
play position resume/persistence, MPRIS2 support, and suspend inhibition.

Kind regards,
Bart De Vries