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
signature.asc
Description: OpenPGP digital signature