Prettify the Accounts KCM and expand KAccounts integration library
ClosedPublic

Authored by leinir on Feb 26 2020, 3:24 PM.

Details

Reviewers
nicolasfella
apol
bshah
ahiemstra
Group Reviewers
VDG
Plasma
Plasma: Mobile
Commits
R155:2fe086801515: Rename the AvailableServices page to AccountDetails
R155:c5f709a41737: Add job to change the account displayname
R155:b9f934f83365: Adjust the look of the "no accounts" view, and fix a minor text mistake
R155:fb1a1424ee5a: Don't allow setting the account display name to what it is currently
R155:8ffb9be596f8: Fix uipluginsmanager compilation
R155:9880cbdabd50: Just use a label (no card) on the account details page as well
R155:509c80d3590b: Unneeded include
R155:c6830c7c5f47: Expose the account displayname change job to qml
R155:8df441b63a4a: Refactor Accounts list and details pages to use the new dialogues
R155:06da5cb9784f: Add self to kcm authors list
R155:6f215a098712: Clean up the plugin code (just for ease of reading and whatnot)
R155:ca3d8cd7ef8b: Expand the services page (should probably be AccountDetails now...)
R155:f3ab5eeef01c: Avoid some asserts on potentially iffy data
R155:4fc39d2c6eaf: Set buttons to nothing special (as it's instant-apply-esque)
R155:53c1b4a9197f: Remove an unneeded item
R155:671b69475c05: Expose the jobs and models to qml, and simplify plugin to only exposing
R155:fed0cb2b12f6: Make the services list prettier
R155:b0dbcc86afb0: Add a bit of documentation (and remove an unneeded include)
R155:e8e7d6e98b78: Add a base documentation file (for more helpful api doc generation)
R155:926ff52c604d: Document the ProvidersModel role names
R155:f78555831c32: Lamdafy some data signal handling, for less publics and easier code
R155:15478b415838: Fix the account removal job (it removed the account but never finished)
R155:ba9033de3530: Equalise the import statement versions and style
R155:0c203fdff333: Also rename the job code itself
R155:e495fdb89322: Add a model displaying information about providers
R155:5df94eccab0a: [WIP] Prettify the Accounts KCM and expand KAccounts integration library
R155:6501d337341e: Merge branch 'master' into prettify-kaccounts-kcm
R155:2e2ab11ebe25: Add RenameAccountDialog component to complement the Removal one
R155:0c767469e6ce: Remove some unneeded qualifications
R155:33ace548b682: Just use enabled: false and don't have active components in the label
R155:44167cfbfa74: Fix include
R155:988158c6261f: Adjust the look of the new account page
R155:11c397c97aa9: Add i18n and jobs to the kaccounts library
R155:dd54adc0abc0: Add RemoveAccountDialog component
R155:fe60be8247e3: Prettify the prettified account details page a touch, for more pretty
R155:4516bc847adf: Allow renaming of an account by display name on the Account Details page
R155:01f573f27d5d: Switch to using our jobs and models in Accounts (and prettify)
R155:76656a26cc1e: Fix a bit of whitespace/naming
R155:1d82b30b65e4: A bit of work on the Account config page
R155:1199fb8927cc: Make the "no accounts yet" indicator more pleasant and functional
R155:04478c7d273d: Add the KCM to readme.md as well
R155:5eb975026b84: Prettify the Accounts KCM and expand KAccounts integration library
R155:cd711440c46f: Naming cleanup, part two
R155:6ef1eb53eb2f: A touch of documentation for the jobs
R155:6b0b4f7004dd: Add a model to expose the list of accounts
R155:8ae8745df2ac: Add a model for exposing the services for a single account
R155:29d31b7d9248: Document the roles in AccountsModel
R155:30a4eb114ff9: More documentaiton for the services model
R155:ff77839be8f7: Rename the job code files to somethingjob (part of larger refactor)
R155:d058ea0cbd8f: A touch of naming cleanup
R155:6d880a95c393: Update ServiceModel account data when displayname is changed
R155:a3a16ab4ec29: Adjust naming of multiple accounts role to be more expected
Summary

This patch represents an effort which aims to expand, beautify, and clean the
KAccounts integration code and visuals. This builds on the existing work, by adding
new models and new job classes to the library, and exposing them through
the QML module, which is used in the KCM as seen in the screenshots below.

New functionality in the library:

  • AccountsModel - A model listing what accounts are currently created on a user's system
  • ServicesModel - A model listing what services (if any) are available in an account
  • ProvidersModel - A model listing what providers are available for new accounts
  • ChangeAccountDisplayNameJob - A job which allows changing the user visible name of an account
  • RemoveAccountJob - A job which removes an account (and all its credentials)

The QML module has been modified to both include the old names of older jobs, as well as the new, more descriptive names.

As for the KCM, please see the screenshots below for what features have been added and modified (the above code all supports the functionality in the KCM):

List of accounts:

List of accounts, when there are no accounts:

The accounts detail page (when clicking an account entry, and there is at least one thing to configure):

Above, when there are no configurable services:

Add new account page (with some existing accounts, but no disabled entries as i haven't got any providers that disallow multiple accounts)

Rename account (also the button to cause this to happen in the background, there)

Diff Detail

Repository
R155 KAccounts Integration
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
leinir updated this revision to Diff 76614.Fri, Feb 28, 11:18 AM

A pre-merging diff, just in case something goes horribly wrong my end

  • Avoid some asserts on potentially iffy data
  • Make the "no accounts yet" indicator more pleasant and functional
  • A bit of work on the Account config page
leinir updated this revision to Diff 76616.Fri, Feb 28, 11:44 AM

(yeah, this took a bit of work...)

  • Merge branch 'master' into prettify-kaccounts-kcm
leinir updated this revision to Diff 76626.Fri, Feb 28, 12:49 PM
  • Lamdafy some data signal handling, for less publics and easier code
  • Add a bit of documentation (and remove an unneeded include)
leinir edited the summary of this revision. (Show Details)Fri, Feb 28, 12:54 PM
leinir updated this revision to Diff 76777.Mon, Mar 2, 3:33 PM
  • Fix a bit of whitespace/naming
  • Add a model displaying information about providers
  • Adjust naming of multiple accounts role to be more expected
  • Adjust the look of the new account page
leinir edited the summary of this revision. (Show Details)Mon, Mar 2, 3:34 PM
leinir updated this revision to Diff 76841.Tue, Mar 3, 11:23 AM
  • Rename the AvailableServices page to AccountDetails
  • Unneeded include
  • Add job to change the account displayname
  • Expose the account displayname change job to qml
  • Update ServiceModel account data when displayname is changed
  • A touch of naming cleanup
  • Naming cleanup, part two
  • Allow renaming of an account by display name on the Account Details page
leinir edited the summary of this revision. (Show Details)Tue, Mar 3, 11:24 AM
leinir updated this revision to Diff 76845.Tue, Mar 3, 1:04 PM
  • Make the services list prettier
leinir edited the summary of this revision. (Show Details)Tue, Mar 3, 1:05 PM
leinir updated this revision to Diff 76850.Tue, Mar 3, 1:51 PM
  • Clean up the plugin code (just for ease of reading and whatnot)
  • Add a base documentation file (for more helpful api doc generation)
  • Add the KCM to readme.md as well
leinir updated this revision to Diff 76854.Tue, Mar 3, 2:32 PM
  • A touch of documentation for the jobs
  • More documentaiton for the services model
  • Document the roles in AccountsModel
  • Document the ProvidersModel role names
leinir retitled this revision from [WIP] Prettify the Accounts KCM and expand KAccounts integration library to Prettify the Accounts KCM and expand KAccounts integration library.Tue, Mar 3, 3:04 PM
leinir edited the summary of this revision. (Show Details)
leinir added a project: Plasma: Mobile.

I'm not a fan of the Card usage. We already have a frame around the area, so I'd say another, different frame around the text is too much. I also don't know any place where we use a style like that

I'm not a fan of the Card usage. We already have a frame around the area, so I'd say another, different frame around the text is too much. I also don't know any place where we use a style like that

i'm personally a bit either-way on it... Right, in which case, i'd really like the @VDG to take a look at what to do when we have an empty list. I'm quite happy doing something else, but as you hint at, we don't really have a defined way to do this, and we end up with inconsistency... Previously, i had just the label, centered in the view, without the card. Happy to go back to that, but also worth taking a look and making sure consistency is the end result :)

I'm not a fan of the Card usage. We already have a frame around the area, so I'd say another, different frame around the text is too much. I also don't know any place where we use a style like that

Some context for this chat as well, this is what i had before switching to the card. Quite like it, but it'd be good to have some sort of centrally stipulated "this is what we do when lists are empty" decision to point to in the HIG somewhere, for example :) The idea is fairly simple: Slightly faded-out to avoid giving the impression of being pushy, centered in both orientations just for visual balance, and containing a short description of why the list is empty, and what can be done to fix the situation. It has been mentioned that the opacity might not be a problem, so here's both:

With opacity 0.7:

Fully opaque:

leinir updated this revision to Diff 76932.Wed, Mar 4, 1:07 PM
  • Set buttons to nothing special (as it's instant-apply-esque)
  • Adjust the look of the "no accounts" view, and fix a minor text mistake
leinir edited the summary of this revision. (Show Details)Wed, Mar 4, 1:08 PM
leinir edited the summary of this revision. (Show Details)

Much better IMO. I don't have a strong opinion on opacity, but I agree this is something that should be documented

ngraham added a subscriber: ngraham.Wed, Mar 4, 3:52 PM

This is looking truly fantastic!!!

Regarding the placeholder label opacity, it's preferred to instead set enabled: false on the label, to ensure that the opacity is consistent. Since that would disable the link in the label, it's probably okay to set opacity: 0.6 instead--or maybe instead, remove the link from the label and put the {Add New Account...} button into the view itself when there are no configured accounts.

This is looking truly fantastic!!!

Regarding the placeholder label opacity, it's preferred to instead set enabled: false on the label, to ensure that the opacity is consistent. Since that would disable the link in the label, it's probably okay to set opacity: 0.6 instead--or maybe instead, remove the link from the label and put the {Add New Account...} button into the view itself when there are no configured accounts.

Hmm... That makes me think, though - perhaps just not have it be active, and instead suggest people do what they'll also have to do next time when there's no label there would be better all 'round. Update incoming :)

leinir updated this revision to Diff 77005.Thu, Mar 5, 10:26 AM
  • Just use enabled: false and don't have active components in the label
leinir edited the summary of this revision. (Show Details)Thu, Mar 5, 10:26 AM
leinir updated this revision to Diff 77011.Thu, Mar 5, 11:25 AM
  • Just use a label (no card) on the account details page as well
leinir edited the summary of this revision. (Show Details)Thu, Mar 5, 11:26 AM
leinir updated this revision to Diff 77085.Fri, Mar 6, 11:15 AM
  • Remove an unneeded item
leinir updated this revision to Diff 77100.Fri, Mar 6, 12:47 PM
  • Don't allow setting the account display name to what it is currently
ahiemstra requested changes to this revision.Fri, Mar 6, 1:11 PM
ahiemstra added a subscriber: ahiemstra.
ahiemstra added inline comments.
src/kcm/package/contents/ui/AccountDetails.qml
21 ↗(On Diff #77085)

Best be consistent and set these all to the same minor Qt version. You probably want to use 2.12.

36 ↗(On Diff #77085)

No abbreviations please.

42 ↗(On Diff #77085)

You shouldn't need to qualify displayName/providerName here, a property binding on an object can access that object's properties directly. (And removes the need for abbreviations in often-used identifiers. ;) )

54 ↗(On Diff #77085)

Not really sure I'm happy with this bit, it seems to me the need to create jobs this way should be handled by something different. Now I, as a user of KAccounts, would need to remember that I need to do this construction to deal with account creation/removal, whereas a separate object that I can instantiate and either connect to with signals or that gives me job instances would be a lot more self-documenting. To me, the jobs are an implementation detail that shouldn't be exposed to QML at all.

src/kcm/package/contents/ui/Accounts.qml
91 ↗(On Diff #77100)

This code is effectively duplicated between two files. It would probably be better to move it into a separate "RemoveAccountDialog" or so.

src/lib/accountsmodel.cpp
35 ↗(On Diff #77100)

Why is this a QObject?

151 ↗(On Diff #77100)

This seems like a fragile construction. Why not expose Account directly and create the ServicesModel from the QML side?

This revision now requires changes to proceed.Fri, Mar 6, 1:11 PM
nicolasfella added inline comments.Fri, Mar 6, 1:19 PM
src/kcm/package/contents/ui/Accounts.qml
43 ↗(On Diff #77100)

Here you are essentially recreating Kirigami.BasicListItem. I guess you have to because of the SwipeListItem, but maybe that's something that should be addressed in Kirigami

nicolasfella added inline comments.Sat, Mar 7, 4:48 PM
src/lib/providersmodel.cpp
23 ↗(On Diff #77100)
#include "core.h"
nicolasfella added inline comments.
src/kcm/package/contents/ui/Accounts.qml
49 ↗(On Diff #77100)

IMO large is too large, but it should use whatever comes out of T12789

leinir marked 2 inline comments as done.Mon, Mar 9, 2:27 PM
leinir added inline comments.
src/kcm/package/contents/ui/AccountDetails.qml
36 ↗(On Diff #77085)

ok

;)

42 ↗(On Diff #77085)

Mostly, i like doing this to avoid the weird issues that invariably pop up when dealing with attached properties and model roles and whatnot, especially when crossing scopes... (but also, some of them are a bit overzealous, so i'll adjust it a bit :) )

54 ↗(On Diff #77085)

i personally quite like how explicit the jobs are, but also, you're right that the way they work in QML is... suboptimal. Perhaps wrapping them in something more QML-friendly would make sense, though i'm also now thinking this is something that needs more thought and wants to be done in a generic fashion at a KJob level, because those things are powerful and useful, and pretty awkward in QML just in general, and that's a terrible shame :) (cue someone jumping in and telling me that's already done and i didn't know, right? ;) if not, this sounds like a thing that wants some effort piled onto it)

src/kcm/package/contents/ui/Accounts.qml
43 ↗(On Diff #77100)

Pretty much that, yes - and definitely wants doing in Kirigami (not sure if BasicSwipeListItem wants to happen, or if it just needs to be in SwipeListItem... thinking the former, as the latter might just well end up being a bit heavy, carrying about those objects that would end up replaced by a lot of apps anyway, in what is already a heavy component...)

49 ↗(On Diff #77100)

Yeah, this wants a central decision before just sort of... picking another one :) For now i'm thinking internal consistency is better, since there's no general consensus (yet!) - once there is, it's an easy fix :)

91 ↗(On Diff #77100)

Thanks for the reminder, yeah, i wanted to do that before and kind of... just forgot, i guess :)

src/lib/accountsmodel.cpp
151 ↗(On Diff #77100)

It's already possible to do it that way around (the DataObject is that object, and ServicesModel will accept that), this role exists to ensure that we don't spam creation of those models (if the base of the application hierarchy contains an AccountsModel instance), which can be a bit on the pricey side.

leinir updated this revision to Diff 77281.Mon, Mar 9, 2:27 PM
leinir marked an inline comment as done.

Address comments by @ahiemstra and @nicolasfella

  • Fix the account removal job (it removed the account but never finished)
  • Add RemoveAccountDialog component
  • Add RenameAccountDialog component to complement the Removal one
  • Refactor Accounts list and details pages to use the new dialogues
  • Equalise the import statement versions and style
  • Remove some unneeded qualifications
  • Fix include
leinir updated this revision to Diff 77354.Tue, Mar 10, 2:23 PM

Chatted with @bshah a bit on the look of the details page... better now :)

  • Prettify the prettified account details page a touch, for more pretty
leinir edited the summary of this revision. (Show Details)Tue, Mar 10, 2:24 PM
bshah accepted this revision as: Plasma: Mobile, bshah.Tue, Mar 10, 2:36 PM

This looks fairly good to me now, further improvements can be done at later smaller chunk sized patches rather then piling things on top of it.

On topic of merging this bit, I would much prefer if commit history is maintained here instead of squashing this (which arc land does by default). So perhaps manual rebase and git push please.

src/lib/servicesmodel.cpp
190 ↗(On Diff #77354)

One minor thing. But then can be separate commit as well, instead of unknown, maybe some kind of icon which represents user should be used.

This revision was not accepted when it landed; it landed in state Needs Review.Wed, Mar 11, 10:08 AM
This revision was automatically updated to reflect the committed changes.
leinir added inline comments.Wed, Mar 11, 2:33 PM
src/lib/servicesmodel.cpp
190 ↗(On Diff #77354)

Had a glance through Cuttlefish, and thinking perhaps simply the straightforward "user" icon... unless the VDG has words on the topic, seems a reasonable choice just for... well, a more pleasant and less stick-out-ey option :) (unknown /is/ a bit... distinct, shall we say)

Super awesome stuff. I submitted a quick follow-up patch for something I noticed while taking a video for the weekly blog post: D28040