Review Request 124589: Add Disk Quota Plasmoid
Kai Uwe Broulik
kde at privat.broulik.de
Sun Aug 2 12:38:37 UTC 2015
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124589/#review83321
-----------------------------------------------------------
Nice! :)
Just being pedantic here, I've seen it work nicely in action.
applets/diskquota/package/contents/config/main.xml (lines 10 - 12)
<https://git.reviewboard.kde.org/r/124589/#comment57551>
This file looks reminiscent of the notes applet
applets/diskquota/package/contents/ui/ListDelegateItem.qml (lines 50 - 51)
<https://git.reviewboard.kde.org/r/124589/#comment57552>
You can just do width: parent.width
applets/diskquota/package/contents/ui/ListDelegateItem.qml (line 97)
<https://git.reviewboard.kde.org/r/124589/#comment57553>
This is the default anyway
applets/diskquota/package/contents/ui/main.qml (line 31)
<https://git.reviewboard.kde.org/r/124589/#comment57554>
I would prefer if that was a proper switch/if, if there's more than one condition with the ternary operator, especially with long enums, it gets pretty ugly
Plasmoid.status: {
switch (diskQuota.status) {
case DiskQuota.ActiveStatus:
return PlasmaCore.Types.ActiveStatus
…
}
applets/diskquota/package/contents/ui/main.qml (line 65)
<https://git.reviewboard.kde.org/r/124589/#comment57557>
No space between ! and variable
applets/diskquota/package/contents/ui/main.qml (line 67)
<https://git.reviewboard.kde.org/r/124589/#comment57555>
Why not just i18n("Quota tool not found.\n\nPlease install 'quota'.") ?
applets/diskquota/package/contents/ui/main.qml (line 97)
<https://git.reviewboard.kde.org/r/124589/#comment57556>
You might want to re-arrange your properties, so you don't scatter Plasmoid.foo properties all over the file :)
applets/diskquota/package/metadata.desktop (line 21)
<https://git.reviewboard.kde.org/r/124589/#comment57549>
I don't think we want that applet in system tray by default?
applets/diskquota/package/metadata.desktop (lines 22 - 23)
<https://git.reviewboard.kde.org/r/124589/#comment57550>
Can be removed
applets/diskquota/plugin/DiskQuota.h (lines 36 - 42)
<https://git.reviewboard.kde.org/r/124589/#comment57559>
You shouldn't add a WRITE accessor if you cannot actually change the value from QML. This makes them read-only from QML perspective and saves you a headache if you accidentally write to them
applets/diskquota/plugin/DiskQuota.h (line 47)
<https://git.reviewboard.kde.org/r/124589/#comment57560>
In Plasma we use plasma-frameworks coding style, ie. DiskQuota(QObject *parent = nullptr)
applets/diskquota/plugin/DiskQuota.h (line 59)
<https://git.reviewboard.kde.org/r/124589/#comment57563>
Methods called through Q_PROPERTY don't need to be slots, I think only updateQuota() and below need to be.
applets/diskquota/plugin/DiskQuota.h (line 70)
<https://git.reviewboard.kde.org/r/124589/#comment57561>
const QString &
and in some other places below too
applets/diskquota/plugin/DiskQuota.h (lines 111 - 113)
<https://git.reviewboard.kde.org/r/124589/#comment57562>
You could initialize these directly in the header file.
bool m_quotaInstalled = true (or, well, false)
etc
applets/diskquota/plugin/DiskQuota.cpp (line 59)
<https://git.reviewboard.kde.org/r/124589/#comment57564>
if (!installed) {
applets/diskquota/plugin/DiskQuota.cpp (lines 177 - 178)
<https://git.reviewboard.kde.org/r/124589/#comment57565>
You could use an initializerList:
const QStringList args{
QStringLiteral("foo"),
QStringLiteral("bar")
};
applets/diskquota/plugin/QuotaItem.cpp (line 20)
<https://git.reviewboard.kde.org/r/124589/#comment57567>
Unused
applets/diskquota/plugin/QuotaListModel.h (line 36)
<https://git.reviewboard.kde.org/r/124589/#comment57568>
Aren't these protected?
applets/diskquota/plugin/QuotaListModel.h (line 40)
<https://git.reviewboard.kde.org/r/124589/#comment57569>
Since you're not using Q_NULLPTR, you might as well use override here.
applets/diskquota/plugin/QuotaListModel.cpp (lines 43 - 44)
<https://git.reviewboard.kde.org/r/124589/#comment57570>
I'm not sure but I would think that QAbstractItemModel calls this only once and thus no caching is required
applets/diskquota/plugin/QuotaListModel.cpp (line 77)
<https://git.reviewboard.kde.org/r/124589/#comment57572>
So does it usually give us an invalid model index?
applets/diskquota/plugin/QuotaListModel.cpp (line 180)
<https://git.reviewboard.kde.org/r/124589/#comment57573>
Now I see why you need setData
Imho a QStandardItemModel would have been sufficient for all of this :)
- Kai Uwe Broulik
On Aug. 2, 2015, 12:17 nachm., Dominik Haumann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124589/
> -----------------------------------------------------------
>
> (Updated Aug. 2, 2015, 12:17 nachm.)
>
>
> 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/config/main.xml 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/20150802/477b2e65/attachment-0001.html>
More information about the Plasma-devel
mailing list