Hello, On Mon, Jan 25, 2021 at 02:18:51PM +0100, Harald Sitter wrote: > 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.
https://invent.kde.org/plasma-mobile/plasma-phonebook/commit/98d768493ab7ab1360d454983bb99e339751d91c > 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 https://invent.kde.org/plasma-mobile/plasma-phonebook/-/commit/a89acbfc9e4227a7cf0d91f5ecdee8cf105710f8 > > 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)? https://invent.kde.org/plasma-mobile/plasma-phonebook/-/commit/e7c1e54d71d0c3cdd36950d0f8b326df44aca115 > > Many files don't have license info, see point 15 of the license policy > [1] :( > > Actual qml **source** files have no license info! Licensing is being sorted out as I write this email. I will reach out to Fabian, original author of some of these files for clarification > > DetailListItem.qml isn't used. https://invent.kde.org/plasma-mobile/plasma-phonebook/-/commit/4add44d12f9d43ab4f8eba181f3ad5b681429dc6 > > 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. This needs looking into. > > 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> :( https://invent.kde.org/plasma-mobile/plasma-phonebook/-/commit/27a94cdda11ecc5b6ed0269917a2643d5f632518 > > # 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`. https://invent.kde.org/plasma-mobile/plasma-phonebook/-/commit/1007e7b2f0c916bdf9f74b2ae63a8d494cd807c9 > > [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 > -- Bhushan Shah http://blog.bshah.in IRC Nick : bshah on Freenode GPG key fingerprint : 0AAC 775B B643 7A8D 9AF7 A3AC FE07 8411 7FBC E11D
signature.asc
Description: PGP signature
