New repo in kdereview: plasma-phonebook

[Harald Sitter]

On 25.01.21 06:48, Bhushan Shah wrote:
> Hello everyone!
> 
> I am back with more Plasma Mobile related repositories in kdereview. I
> want to move plasma-phonebook in kdereview. plasma-phonebook is kirigami
> based phonebook application, it uses kpeople backends like kpeoplesink,
> kpeoplevcard, kpeople akonadi backend etc to fetch and store contacts on
> your system.

I am pretty sure l10n isn't working. I can't see the translations domain
set anywhere.

On the subject of strings I'd suggest checking them all for HIG
compliance. In main.qml alone all strings are wrong as titles and
buttons are supposed to use Title Capitalization
https://hig.kde.org/style/writing/capitalization.html

The desktop file seems to be lacking an actual main category (it's in
the lost&found section in the launcher for me)

What's the use case for the unregistered x-plasma-phonebook mimetype
(registered for by the desktop file)? Perhaps the more relevant question
is why would it should be unregistered instead of inside our vendor tree
(vnd.kde.phone.book or some such)?

Many files don't have license info, see point 15 of the license policy
[1] :(

Actual qml **source** files have no license info!

DetailListItem.qml isn't used.

Are we quite sure that hardcoding colors is the way to go? :O

Header.qml actually has a crash for me because ColorOverlay is inside a
FastBlur that is both parent and source (qt docs about source: "Note: It
is not supported to let the effect include itself, for instance by
setting source to the effect's parent.")

Similarly the FastBlur looks to be falling into that trap of
parent===source, albeit not crashing for me.

Spelling is inconsistent. The desktop file spells it "Phone Book" the
appstream file spells it "Plasma Phonebook".

.appdata.xml is a legacy suffix, you might want to use .metainfo.xml
instead.

On the subject of the appstream file. Missing
<project_group>KDE</project_group> :(

# Probably a bit nitpicky

Failing to store contact changes in AddContactPage lacks actual UI
backing, it merely qwarns.

Various single argument ctors aren't tagged `explicit`.

[1] https://community.kde.org/Policies/Licensing_Policy

> Another thing I want to discuss is, branding, currently it is named as
> Plasma Phonebook which is bit awkward because Plasma is not related to
> app at all and even android port for app exists. Maybe we should name
> this just phonebook?

Is this about the repo/tarball name or the display string?

The former already lacks the plasma prefix in the .desktop file, I'd
argue the appstream name should do the same. System Settings is a
precedent for something similar.

As for tarballs: some distros get grumpy over overly generic source
names, so while I think phonebook is fine it may be less fine for
distros. It probably matters little what the repo/tarball is called though.

HS

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20210125/4aec6ba7/attachment.sig>