Review request: plasma-pass

[Albert Astals Cid]

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/
> 
>