Hi Albert, thanks for the review! All the issues you mentioned are fixed in master now.
Dan On Sunday, 16 December 2018 20:14:37 CET Albert Astals Cid wrote: > El dilluns, 10 de desembre de 2018, a les 14:49:28 CET, Daniel Vrátil va escriure: > > Hi folks, > > > > back in May I wrote a Plasma applet that serves as a GUI frontend for the > > pass password manager [0]. I blogged about it [1], but then somewhat > > forgot to make a release etc. Recently I started getting some emails from > > packagers where to get a tarballs so I think it's time to get some > > translations in and start doing official releases. Thus I'd like to ask > > for a review for inclusion in extragear. > > > > The way pass works is it has a directory in which each password is stored > > in a PGP-encrypted file, the name of the file is the name of the > > password. You can also create subfolders to organize the passwords. > > > > The code of the applet is a C++ model that watches said directory and > > exposes its content as a tree. There's also a filter proxy model which > > uses partial string matching code from KDevelop (so you can filter for > > "jd@f" and it matches "john....@fmail.com"). > > > > The applet itself sits in systray, when activated it shows the top-level > > list of folders and password. Code-wise it contains a stack of list > > views, when entering a subfolder it pushes a new listview with content of > > that folder to the stack. Selecting a password decrypts it using gpg and > > puts it into clipboard for 45 seconds. After that it clears it from the X > > clipboard as well as Klipper. > > > > There's a bit of mess regarding focus handling in the QML, but the goal > > was to make the applet fully controllable via keyboard, which wasn't easy > > with my QML skills :) > > You have > > plasma-pass:master$ wcgrep X-KDE-PluginInfo-Name > ./package/metadata.desktop:27:X-KDE-PluginInfo-Name=plasmapass > ./package/metadata.desktop:34:X-KDE-PluginInfo-Name=org.kde.plasma.pass > > Which one is it? I guess the second? Then you also need to update your > Messages.sh to match that one if you change it, see applets/colorpicker in > kdeplasma-addons for example > > > You're using old style connects, use the power of the "new one". > > > PlasmaPass::PasswordsModel::Node has dtor but not copy-ctor, > copy-assignment. Which means that if someone ever does PasswordsModel::Node > a; > PasswordsModel::Node b = a; > things will break since the default implementations will just copy the > children vector and bad things will happen, i suggest you delete the copy > and assignment functions. > > > filterChanged is already a name of an existing function on > QSortFilterProxyModel (i know sucks), so maybe you could rename that signal > to something else? I guess this is not very important though but some > people may get confused when reading the code. > > > In the QML side you do a few == != that would be 0.00000000001% faster doing > === and !==, it's considered better JS code since "weird" promotions are > not done, but your call. > > > I have not actually tested whether the thing works or not, but it's small > enough that i guess it does :D > > Just had time for this quick code review, hope you find it useful :) > > Cheers, > Albert > > > Looking forward to your feedback, > > Dan > > > > > > [0] https://www.passwordstore.org/ > > [1] https://www.dvratil.cz/2018/05/plasma-pass/ -- Daniel Vrátil www.dvratil.cz | dvra...@kde.org IRC: dvratil on Freenode (#kde, #kontact, #akonadi, #fedora-kde) GPG Key: 0x4D69557AECB13683 Fingerprint: 0ABD FA55 A4E6 BEA9 9A83 EA97 4D69 557A ECB1 3683
signature.asc
Description: This is a digitally signed message part.
