Review Request 124589: Add Disk Quota Plasmoid

[David Edmundson]

> On Aug. 6, 2015, 2:12 p.m., Lamarque Souza wrote:
> > applets/diskquota/plugin/DiskQuota.cpp, line 158
> > <https://git.reviewboard.kde.org/r/124589/diff/5/?file=389660#file389660line158>
> >
> >     You should search for quota and filelight programs during startup only. You can send a notification if they are not found so the user knows they are not installed.
> >     
> >     Polling filesystem every two minutes is not extreme bad but it should be prevented if it is not really necessary.
> 
> David Edmundson wrote:
>     Can you think of a way we can tell if it's installed later?
> 
> Lamarque Souza wrote:
>     The plasmoid will search for them at every logon. Why is that not enough?
>     
>     You can also connect a slot to org.freedesktop.ScreenSaver.ActiveChanged(false) signal from ksmserver to check for them when the user unlocks the screen.
>     
>     The point is that current code searches for them even when they were detected two minutes before. That's overkill. If the intention is to be over precautions then I step down here. I still insist in sending a notification to warn the user if the programs are not installed.
> 
> Dominik Haumann wrote:
>     Ok, I would like to implement the following solution:
>     - In the constructor, I check only once if quota exists. If it exists, all is good, and the applet runs as before.
>     - In the constructor, if 'quota' does not exist, I will _not_ launch the timer and instead add a button with the text i18n("Check Again") under the text displayed of this: http://kate-editor.org/wp-content/uploads/2015/08/diskquota-missing.png - Clicking this button would look for 'quota' again, and on success starts the timer and the applet runs as before.
>     
>     Would you accept this solution?
> 
> Kai Uwe Broulik wrote:
>     What about checking when you open the plasmoid?
> 
> Sebastian Kügler wrote:
>     I think the solution with the button is fine. Elegant, and exactly there where you'd expect it.
> 
> Lamarque Souza wrote:
>     I like Kai's suggestion: if the programs were not found yet then search for then when user clicks on system tray icon. That is more automatic then using a button.

I know I said the same as Lamarque in my review, but on reflection I don't think it's worth changing.

Given it's not in the default set, a user has to manually add it.

On finding an error message "you must install quota" the user will either:
 - install quota
 - remove the plasmoid

So we're never realistically going to be in this situation.

Also I did a benchmark:

    QBENCHMARK {
        QStandardPaths::findExecutable(QStringLiteral("quota"));
    }
    
    0.017 msecs per iteration (total: 72, iterations: 4096)

That's one second of overhead every 81.7 days of constant use. (I did the maths)
We've got much bigger places to worry about optimising than a stat call.

I'm fine with you pushing as-is.


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124589/#review83498
-----------------------------------------------------------


On Aug. 3, 2015, 5:34 p.m., Dominik Haumann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124589/
> -----------------------------------------------------------
> 
> (Updated Aug. 3, 2015, 5:34 p.m.)
> 
> 
> Review request for Plasma, Kai Uwe Broulik and Sebastian Kügler.
> 
> 
> Repository: kdeplasma-addons
> 
> 
> Description
> -------
> 
> The disk quota is usually used in enterprise installations where network shares are mounted locally. Typically, sysadmins want to avoid that users copy lots of data into their folders, and therefor set quotas (the quota limit has nothing to do with the physical size of a partition). Typically, once a user gets over the hard limit of the quota, the account is blocked and the user cannot login anymore. This happens from time to time, since the users are not really aware of the current quota limit and the already used disk space.
> 
> Here is where the "Disk Quota" plasmoid helps: It continusouly monitors the disk quota and warns the quota apprpriately.
> 
> A detailed description including screenshots can be found in this blog: http://kate-editor.org/?p=3591
> 
> (I had a KDE4 hack of this plasmoid running at university, and it proved very usable over the years, so it is probably a good idea to have it by default in plasma)
> 
> Issues:
> - the panel icon is larger than the others (some wrong margin?)
> - an icon for the metadata.desktop is missing (the shipped quota.svg file is not available here, it seems).
> - the grid units probably need some more tuning
> 
> 
> Diffs
> -----
> 
>   applets/CMakeLists.txt c60c350 
>   applets/diskquota/CMakeLists.txt PRE-CREATION 
>   applets/diskquota/Messages.sh PRE-CREATION 
>   applets/diskquota/icons/quota.svg PRE-CREATION 
>   applets/diskquota/package/contents/ui/ListDelegateItem.qml PRE-CREATION 
>   applets/diskquota/package/contents/ui/main.qml PRE-CREATION 
>   applets/diskquota/package/metadata.desktop PRE-CREATION 
>   applets/diskquota/plugin/DiskQuota.h PRE-CREATION 
>   applets/diskquota/plugin/DiskQuota.cpp PRE-CREATION 
>   applets/diskquota/plugin/QuotaItem.h PRE-CREATION 
>   applets/diskquota/plugin/QuotaItem.cpp PRE-CREATION 
>   applets/diskquota/plugin/QuotaListModel.h PRE-CREATION 
>   applets/diskquota/plugin/QuotaListModel.cpp PRE-CREATION 
>   applets/diskquota/plugin/plugin.h PRE-CREATION 
>   applets/diskquota/plugin/plugin.cpp PRE-CREATION 
>   applets/diskquota/plugin/qmldir PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/124589/diff/
> 
> 
> Testing
> -------
> 
> Tested combinations:
> - no quota installed: A nice message is displayed telling the user that 'quota' is missing.
> - quota installed, but no quota restrictions set: The applet says "No quota restrictions found"
> - quota installed, quotas active: The applet continuously shows the data. The quota entries are in a QAbstractItemModel derived class, so inserting/removing quotas all works (tested).
> - filelight installed: the item under mouse gets highlighted. If clicked, filelight starts with the correct location.
> 
> 
> Thanks,
> 
> Dominik Haumann
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20151023/df3975a7/attachment-0001.html>