D26447: Dynamically show and hide based on whether or not any vaults are configured

This revision was automatically updated to reflect the committed changes. Closed by commit R845:f8c6924510ea: Dynamically show and hide based on whether or not any vaults are configured (authored by ngraham). REPOSITORY R845 Plasma Vault CHANGES SINCE LAST UPDATE

D26447: Dynamically show and hide based on whether or not any vaults are configured

ngraham added a comment. Thanks! REPOSITORY R845 Plasma Vault BRANCH dynamically-show-and-hide (branched from master) REVISION DETAIL https://phabricator.kde.org/D26447 To: ngraham, broulik, ivan, #plasma, #vdg Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus,

D26447: Dynamically show and hide based on whether or not any vaults are configured

ivan accepted this revision. ivan added a comment. This revision is now accepted and ready to land. That is why I put the "or in addition to" part. Anyhow, I don't mind this to go in as is. REPOSITORY R845 Plasma Vault BRANCH dynamically-show-and-hide (branched from master)

D26447: Dynamically show and hide based on whether or not any vaults are configured

broulik added a comment. > why this is a `count` It's only consistent with how we usually do it elsewhere, and I think for a list model having a `count` in general makes more sense/is more generic/more useful than an `isEmpty` property REPOSITORY R845 Plasma Vault REVISION DETAIL

D26447: Dynamically show and hide based on whether or not any vaults are configured

ngraham added a comment. In D26447#588486 , @ivan wrote: > For some reason, hiding this icon when there are no vaults was deemed undesired before. I don't recall why as I think it is a good idea. :) > > Maybe it was always shown for

D26447: Dynamically show and hide based on whether or not any vaults are configured

ivan added a comment. For some reason, hiding this icon when there are no vaults was deemed undesired before. I don't recall why as I think it is a good idea. :) Maybe it was always shown for discoverability purposes... don't know. Is there a reason why this is a `count` instead of

D26447: Dynamically show and hide based on whether or not any vaults are configured

ngraham updated this revision to Diff 72841. ngraham marked 3 inline comments as done. ngraham added a comment. Address more review comments (eventually I'll get this right...) REPOSITORY R845 Plasma Vault CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26447?vs=72840=72841

D26447: Dynamically show and hide based on whether or not any vaults are configured

broulik added a comment. +1 INLINE COMMENTS > vaultsmodel.h:36 > Q_PROPERTY(bool hasError READ hasError NOTIFY hasErrorChanged) > +Q_PROPERTY(int rowCount READ rowCount NOTIFY rowCountChanged) > The property name should stay `count` to avoid conflict with `rowCount` which us

D26447: Dynamically show and hide based on whether or not any vaults are configured

ngraham updated this revision to Diff 72840. ngraham marked 2 inline comments as done. ngraham added a comment. Address review comments REPOSITORY R845 Plasma Vault CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26447?vs=72839=72840 BRANCH dynamically-show-and-hide (branched

D26447: Dynamically show and hide based on whether or not any vaults are configured

broulik added inline comments. INLINE COMMENTS > vaultsmodel.cpp:103 > > +if (vaultKeys.size() > 0) { > +emit q->countChanged(vaultKeys.size()); You probably want to remember the old count and emit when it's different. The model could have been full and become

D26447: Dynamically show and hide based on whether or not any vaults are configured

ngraham created this revision. ngraham added reviewers: broulik, ivan, Plasma, VDG. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. ngraham requested review of this revision. REVISION SUMMARY Currently, the Vaults icon is always visible in the System Tray. However most