----------------------------------------------------------- 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 > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
