Review request: plasma-pass

[Daniel Vrátil]

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 at f" and it matches "john.doe at 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 | dvratil at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20190104/60ea3cd1/attachment.sig>