I've got a branch up and running at https://github.com/tshikaboom/geary/commits/768975-service-information which could help us get there.

This is not complete but seems to be working quite well. Geary.ServiceInformation has been created as an abstract class, with LocalServiceInformation deriving from it. Credentials are just Geary.Credentials though, I haven't looked at how to integrate CredentialsMediator so far.

I took the liberty to rip out some configuration stuff into a helper class of its own; AccountInformation lost a good few lines of code from that.

The last commit (porting everything I could find to ServiceInformation) is huge though, would you like me to split it somehow?

I've just started looking into the CredentialsMediator bit. While trying to integrate SecretMediator into the newly created LocalServiceInformation class, it looks like it doesn't find the Gtk symbols (because I moved SecretMediator from the client to the engine, which does not depend on Gtk), these need to be present to get the password dialog to show up.

What would be the rough way to go here? I could just add Gtk as a dependency to the engine, although that would miss the point of separating the client from the engine..

Hey Oskar,

Thanks for picking this up again, it's looking good!

(In reply to Oskar Viljasaar from comment #1)
> I've got a branch up and running at
> https://github.com/tshikaboom/geary/commits/768975-service-information which
> could help us get there.
> 
> This is not complete but seems to be working quite well.
> Geary.ServiceInformation has been created as an abstract class, with
> LocalServiceInformation deriving from it. Credentials are just
> Geary.Credentials though, I haven't looked at how to integrate
> CredentialsMediator so far.

No problem about using Credentials for now, it's a good way to start.

> I took the liberty to rip out some configuration stuff into a helper class
> of its own; AccountInformation lost a good few lines of code from that.

Looks good, AccountInformation looks a lot less scary now.

> The last commit (porting everything I could find to ServiceInformation) is
> huge though, would you like me to split it somehow?

You mean 798049c? If so, then nah, it's fine if it's just doing the same rename everwhere. One suggestion I would make though based on that is it looks like calling the two ServiceInformation properties "imap_information" and "smtp_information" looks a bit redundant. I'd call them just "imap" and "smtp", so then you would have call chains looking like "account_info.imap.credentials", which is nicely succinct.

A couple of other random comments for now:

 - Don't forget to also add the new source files to po/POFILES.in, even if they don't have any translatable strings
 - I can't see which commit in the branch adds the two ServiceInformation properties to AccountInformation, did I miss it?
 - Likewise I can't see LocalServiceInformation::get_values being called anywhere, but I suspect ether it or the LocalServiceInformation::refresh_settings is redundant?
 - We're trying to to move to using "this" as a prefix for all instance property access, e.g. using "switch (this.service) {" so that it's immediately obvious that the thing is an object property/field instead of a local/global var
 - AccountInformation probably doesn't need the "using Geary.Config;" line anymore, right?

(In reply to Oskar Viljasaar from comment #2)
> I've just started looking into the CredentialsMediator bit. While trying to
> integrate SecretMediator into the newly created LocalServiceInformation
> class, it looks like it doesn't find the Gtk symbols (because I moved
> SecretMediator from the client to the engine, which does not depend on Gtk),
> these need to be present to get the password dialog to show up.
> 
> What would be the rough way to go here? I could just add Gtk as a dependency
> to the engine, although that would miss the point of separating the client
> from the engine..

Ah yep, this is perhaps where it get tricky. We need to keep something like the SecretMediator/CredentialsMediator split since we want to keep the engine free of any UI code, and also allow clients apps to provide whatever auth and config mechanism they want. So the trick will be to allow the engine to obtain new instances of concrete objects for creds & service info from the client.

At the moment GearyController simply constructs an instance of SecretMediator and passes that to Geary.Engine::open_async. It could instead pass through some factory class or delegate implementation that the Engine can then call to construct new credential mediators, something like this:

  public delegate CredentialsMediator new_credentials(string type);

Where the value of the type param might be say "local" for the local service info.

Something to keep in mind however is we also want to do something similar for ServiceInformation instances - again so the client can provide its own implementations to support GOA/UOA/etc. So maybe if LocalServiceInformation was moved into the client code tree it could then construct a SecretMediator instance itself, which it solves that immediate problem.

This means that the code that loads and saves local accounts would also need to be moved into the client tree, but that's fine since the controller can just load them on startup and pass them as collection through to Geary.Engine::open_async. This would then in the future also allow the client to create accounts for other backends such as GOA/UOA, without requiring the engine to know anything about them - which is exactly what we want. Saving of local accounts would also need to be taken care of by the controller as well, but that's fine.

What do you think?

(In reply to Michael Gratton from comment #3)
> You mean 798049c? If so, then nah, it's fine if it's just doing the same
> rename everwhere. One suggestion I would make though based on that is it
> looks like calling the two ServiceInformation properties "imap_information"
> and "smtp_information" looks a bit redundant. I'd call them just "imap" and
> "smtp", so then you would have call chains looking like
> "account_info.imap.credentials", which is nicely succinct.

Ok.

> 
> A couple of other random comments for now:
> 
>  - Don't forget to also add the new source files to po/POFILES.in, even if
> they don't have any translatable strings
>  - I can't see which commit in the branch adds the two ServiceInformation
> properties to AccountInformation, did I miss it?

This is done in the last commit actually, that's why I was thinking of splitting it up; although there should be no functional changes

>  - Likewise I can't see LocalServiceInformation::get_values being called
> anywhere, but I suspect ether it or the
> LocalServiceInformation::refresh_settings is redundant?
>  - We're trying to to move to using "this" as a prefix for all instance
> property access, e.g. using "switch (this.service) {" so that it's
> immediately obvious that the thing is an object property/field instead of a
> local/global var
>  - AccountInformation probably doesn't need the "using Geary.Config;" line
> anymore, right?

This was exactly the kind of feedback I was looking for :-)
I'll fix up everything and send out some patches.

> [CredentialsMediator and account saving]

That sounds good and logical! I'll do it in a subsequent batch when I get the time and motivation to do that, it doesn't seem trivial looking at it right now. Thanks for clearing that stuff up, I can see now why the client might want to pass its own implementation instead of shoving everything in the engine.

Created attachment 345717 [details] [review]
AccountInformation: separate out configuration

Separating the configuration stuff out into Geary.Config makes it a bit tedious to get the config keys in AccountInformation though, everything has to be prefixed with Geary.Config (without the "using Geary.Config;" statement), is that okay coding style-wise? I didn't break up code into multiple lines at the moment, I could do that though.

Created attachment 345718 [details] [review]
Introduce ServiceInformation classes

Created attachment 345719 [details] [review]
Port AccountInformation over to ServiceInformation

What remains to do is to move LocalServiceInformation to the client side, and properly separate out account saving and loading. (Before getting to tackle the CredentialsMediator side of this bug)

I disambiguated the function names in ServiceInformation, does that look better? For now I just moved that code over from AccountInformation to keep the same flow, there may be a better way to do that.

Review of attachment 345717 [details] [review]:

This mostly looks fine. The only comment I'd make is that that there seems to be a lot of unrelated whitespace changes, but just so long as they're all removing trailing whitespace then I guess that's good. This is true of the other patches as well.

Having to use Geary.Config.FOO everwhere is a bit annoying, but that's the convention and I don't mind keeping it explicit. On thing you could get away with is just using "Config.FOO" though, since AccountInformation and Config both share the "Geary" prefix.

I'm happy to accept this as-is, but feel free to update the patch if you feel like doing either of the above.

Review of attachment 345718 [details] [review]:

This also looks mostly good, just a few things to fix up and one question below.

::: src/engine/api/geary-local-service-information.vala
@@ +80,3 @@
+
+        primary_email = Geary.Config.get_string_value(
+            key_file, Geary.Config.GROUP, Geary.Config.PRIMARY_EMAIL_KEY, "");

The primary email is just a setting, but it's an account-wide thing, so should it be back in AccountInformation? It could maybe be passed through to this method as a param if it's really needed for the line below.

::: src/engine/api/geary-service-information.vala
@@ +5,3 @@
+ */
+
+public abstract class Geary.ServiceInformation : GLib.Object {

Shouldn't basically all of the properties declared below be protected set?

@@ +16,3 @@
+    // Used with SMTP servers
+    public bool smtp_noauth { get; set; default = false; }
+    public bool smtp_use_imap_credentials { get; set; default = false; }

It would be nice to not have these on the IMAP props, but I don't think it's worth the complexity of having a SMTP-specific subclass for the moment.

@@ +22,3 @@
+    public abstract void load_credentials() throws Error;
+
+    public abstract void save_settings(ref KeyFile key_file);

There's a bit of asymmetry between ::save_settings being passed a KeyFile as an arg, but ::load_settings and ::load_credentials having to create their own. Is there a reason why that's the case? TBH I'd prefer passing it in per save_settings, but aren't sure if you have some specific reason for not doing it in the other two.

Review of attachment 345719 [details] [review]:

Again, just a few things to clean up below.

::: src/engine/api/geary-account-information.vala
@@ +120,3 @@
+     * and configuration. */
+    public ServiceInformation imap;
+    public ServiceInformation smtp;

These should probably be private set, right?

@@ +204,3 @@
 
+        imap.load_credentials();
+        smtp.load_credentials();

Missing "this".

@@ +225,3 @@
         if (service_provider == ServiceProvider.OTHER) {
+            imap.load_settings();
+            smtp.load_settings();

Ditto here. Actually, most of the references to the smtp or imap properties below are missing it.

@@ +287,3 @@
         this.ordinal = from.ordinal;
+        this.imap = from.imap;
+        this.smtp = from.smtp;

Here we probably need to have add a ::copy method on the service info class and call it on the instances here, since we need copies of that information: It is used when modifying the account settings, so we don't want to modify the live settings while the user is noodling around with it.

@@ +747,3 @@
         key_file.set_integer(Geary.Config.GROUP, Geary.Config.ORDINAL_KEY, this.ordinal);
+        key_file.set_value(Geary.Config.GROUP, Geary.Config.IMAP_USERNAME_KEY, this.imap.credentials.user);
+        key_file.set_boolean(Geary.Config.GROUP, Geary.Config.IMAP_REMEMBER_PASSWORD_KEY, this.imap.remember_password);

These should be saved in LocalServiceInformation, right? They are already loaded there, and for GOA/UOA the login name and the decision to remember the password (if it's even possible not to remember it) will be obtained from those services, so that seems like it would make sense.

@@ +750,3 @@
+        if (smtp.credentials != null)
+            key_file.set_value(Geary.Config.GROUP, Geary.Config.SMTP_USERNAME_KEY, this.smtp.credentials.user);
+        key_file.set_boolean(Geary.Config.GROUP, Geary.Config.SMTP_REMEMBER_PASSWORD_KEY, this.smtp.remember_password);

Ditto for these.

::: src/engine/api/geary-credentials.vala
@@ +1,1 @@
+ /* Copyright 2016 Software Freedom Conservancy Inc.

Void change here? I'd suggest dropping this from the patch.

(In reply to Oskar Viljasaar from comment #5)
> Created attachment 345717 [details] [review] [review]
> AccountInformation: separate out configuration
> 
> Separating the configuration stuff out into Geary.Config makes it a bit
> tedious to get the config keys in AccountInformation though, everything has
> to be prefixed with Geary.Config (without the "using Geary.Config;"
> statement), is that okay coding style-wise? I didn't break up code into
> multiple lines at the moment, I could do that though.

As mentioned in the review, you could just drop the "Geary" prefix and it should still work. Another alternative would be to convert Config into a class that either encapuslates a KeyFile instance or extends it directly. You'd then be able to call methods directly without having to specify "Geary.Config" several times per line.

(In reply to Oskar Viljasaar from comment #8)
> What remains to do is to move LocalServiceInformation to the client side,
> and properly separate out account saving and loading. (Before getting to
> tackle the CredentialsMediator side of this bug)

Yep, sounds good. This isn't going to land in time for 0.12 but I will look into creating a WIP branch and pushing these patches there if that helps? Some of the other devs might be interested in basing work off it.

> I disambiguated the function names in ServiceInformation, does that look
> better? For now I just moved that code over from AccountInformation to keep
> the same flow, there may be a better way to do that.

Much better, ta!

Hey Oskar, thanks a lot for these patches, it improves the accounting engine quite a bit! Anyway, are you still interested in improving these patches? I would like to incorporate them in the work for bug 714104 (as Mike probably predicted in his previous comment).

Anyhow, great work :-)

Sorry for the late reply, I just saw it yesterday.

I just got through my own exams, I'll see what I can do the next few days/weeks since I've got the time now :-)

(In reply to Oskar Viljasaar from comment #14)
> Sorry for the late reply, I just saw it yesterday.
>
> I just got through my own exams, I'll see what I can do the next few
> days/weeks since I've got the time now :-)
Hey, no worries! Exams are finally done here too, so I get the feeling ;) .

Anyway, if you want to talk about the patch or other Geary-related stuff, I'm always available on #geary (irc.gnome.org) as nielsdg.

Created attachment 355607 [details] [review]
Miscellaneous whitespace changes.

This separates out all whitespace changed I found from the other patches.

Created attachment 355608 [details] [review]
AccountInformation: separate out configuration

Created attachment 355609 [details] [review]
Introduce ServiceInformation classes

I think I've addressed all of Mike's comments, but I couldn't get around doing the 'protected set' bits in ServiceInformation, because AddEditPage wants to directly modify bits set in this class. What would be a good solution to that?

Created attachment 355610 [details] [review]
Port AccountInformation over to ServiceInformation

I think I've reached a point where these could be reviewed again.

- Geary.ServiceInformation now has a set_password method with two args, the second being a bool for remembering the password. Would that be a sensible thing to do?

- There is a CredentialsMediator per ServiceInformation, practically speaking they usually are the same per account (SecretMediator), I found that practical to avoid circular calls from ServiceInformation to its corresponding AccountInformation if the mediator was to be in there.

- There is a new credentials_method string, I would propose to use that as a way to differentiate between different credential providers. That would be written in geary.ini, so LocalServiceInformation would usually get "libsecret" written in that (but not always, Geary should be able to load local account files with GOA credentials in the future too).

Created attachment 355613 [details] [review]
Port AccountInformation over to ServiceInformation

Two small edits I missed included this time around.

Review of attachment 355608 [details] [review]:

Looks good. My initial rection was that since it's a bunch of constants and util functions that putting it in src/engine/util would have been best, but if the constants get used elsewhere then keeping it in src/engine/api would make sense, and config is a pretty central part of the API. Pretty happy with this as-is.

Review of attachment 355609 [details] [review]:

Pretty happy with this as-is, as well.

Review of attachment 355613 [details] [review]:

Likewise, this is pretty good.

Oskar, thanks heaps for these patches, they are pretty good. As above, I would be pretty happy to commit them as-is. I guess the only thing left to coordinate here is, as Niels mentioned, coordination with the substantial changes in Bug 714104.

Niels, what do you think? I guess the options here include rebasing these patches off your work on Bug 714104 or vice versa.

Either way it would be useful to have both available as git branches, so I've just pushed these patches as wip/768975-service-info based off current master. I'm also happy to help out with rebasing one or the other.

NB: I just pushed commit 2ab6b38 to the branch clean up a few issues I did find with the patches when trying them out. Oskar, can you have a look at that and let me know if you agree with the changes?

Yes! I'll look at these when I get a moment. (I would like to promise to myself "have it done by Sunday/Monday")

I already found one or two minor nitpicks when reading my code again, I'll try to post some follow-up patches in here.

I was esp. thinking of how to abstract away credentials and how to retrieve them from the right place. I was thinking about this wrt OAuth2 and plain text password management in Bug 772721, and also GOA, since the latter supports both OAuth2 and the usual IMAP/SMTP user/pass combo.

Per that, I don't know if the "method" key in ServiceInformation would be granular enough, but it was a start to that when I was writing these patches.

Perhaps a "provider" key coupled with a "method" key would be more logical? (It changes the current definition of "method" proposed in my patches)
So we would get
credentials_provider = PROVIDER_LIBSECRET;
credentials_method = METHOD_PASSWORD;

That would be generic enough to support GOA (PROVIDER_GOA) with OAuth2 (METHOD_OAUTH2) or password-based auth, and also local plaintext (PROVIDER_PLAINTEXT) storage.

Would that be over-engineering? I'm willing to cook up some patches if it seems sensible enough to do.

All in all, maybe don't push those patches to master right away ;-)

(In reply to Oskar Viljasaar from comment #27)
> Yes! I'll look at these when I get a moment. (I would like to promise to
> myself "have it done by Sunday/Monday")

Yeah that would be great, cheers!

> I was esp. thinking of how to abstract away credentials and how to retrieve
> them from the right place. I was thinking about this wrt OAuth2 and plain
> text password management in Bug 772721, and also GOA, since the latter
> supports both OAuth2 and the usual IMAP/SMTP user/pass combo.
> 
> Per that, I don't know if the "method" key in ServiceInformation would be
> granular enough, but it was a start to that when I was writing these patches.
> 
> Perhaps a "provider" key coupled with a "method" key would be more logical?
> (It changes the current definition of "method" proposed in my patches)
> So we would get
> credentials_provider = PROVIDER_LIBSECRET;
> credentials_method = METHOD_PASSWORD;
> 
> That would be generic enough to support GOA (PROVIDER_GOA) with OAuth2
> (METHOD_OAUTH2) or password-based auth, and also local plaintext
> (PROVIDER_PLAINTEXT) storage.
> 
> Would that be over-engineering? I'm willing to cook up some patches if it
> seems sensible enough to do.

That's probably a good way to do it, yeah. So I guess LocalServiceInformation would default to using PROVIDER_LIBSECRET and either METHOD_OAUTH2 or METHOD_PASSWORD depending on if it's a GMail account or not, and some hypothetical GoaServiceInformation class would use PROVIDER_GOA and an appropriate method depending on what kind of GOA configuration it represents?

Created attachment 361619 [details] [review]
ServiceInformation: differentiate between auth methods and providers

Created attachment 361620 [details] [review]
ServiceInformation: document all properties of this class

(In reply to Michael Gratton from comment #28)
> (In reply to Oskar Viljasaar from comment #27)
> > Yes! I'll look at these when I get a moment. (I would like to promise to
> > myself "have it done by Sunday/Monday")
> 
> Yeah that would be great, cheers!
> 
> > I was esp. thinking of how to abstract away credentials and how to retrieve
> > them from the right place. I was thinking about this wrt OAuth2 and plain
> > text password management in Bug 772721, and also GOA, since the latter
> > supports both OAuth2 and the usual IMAP/SMTP user/pass combo.
> > 
> > Per that, I don't know if the "method" key in ServiceInformation would be
> > granular enough, but it was a start to that when I was writing these patches.
> > 
> > Perhaps a "provider" key coupled with a "method" key would be more logical?
> > (It changes the current definition of "method" proposed in my patches)
> > So we would get
> > credentials_provider = PROVIDER_LIBSECRET;
> > credentials_method = METHOD_PASSWORD;
> > 
> > That would be generic enough to support GOA (PROVIDER_GOA) with OAuth2
> > (METHOD_OAUTH2) or password-based auth, and also local plaintext
> > (PROVIDER_PLAINTEXT) storage.
> > 
> > Would that be over-engineering? I'm willing to cook up some patches if it
> > seems sensible enough to do.
> 
> That's probably a good way to do it, yeah. So I guess
> LocalServiceInformation would default to using PROVIDER_LIBSECRET and either
> METHOD_OAUTH2 or METHOD_PASSWORD depending on if it's a GMail account or
> not, and some hypothetical GoaServiceInformation class would use
> PROVIDER_GOA and an appropriate method depending on what kind of GOA
> configuration it represents?

That's exactly what I had in mind! Initial account discovery may have to be a bit more intelligent, but with those keys it should be feasible. On a first run GoaAccountInformation should be getting the account's details (server host, port etc) from GOA, after that it should be only relying on the config file for that kind of stuff, and only use GOA for credentials management.

It goes without saying (ugh, I'm still writing this message) that these last few go on top of wip/768975-service-info. Testing did not seem to have any ill effects on Geary.

(In reply to Oskar Viljasaar from comment #31)
> (In reply to Michael Gratton from comment #28)
> > (In reply to Oskar Viljasaar from comment #27)
> > > Yes! I'll look at these when I get a moment. (I would like to promise to
> > > myself "have it done by Sunday/Monday")
> > 
> > Yeah that would be great, cheers!
> > 
> > > I was esp. thinking of how to abstract away credentials and how to retrieve
> > > them from the right place. I was thinking about this wrt OAuth2 and plain
> > > text password management in Bug 772721, and also GOA, since the latter
> > > supports both OAuth2 and the usual IMAP/SMTP user/pass combo.
> > > 
> > > Per that, I don't know if the "method" key in ServiceInformation would be
> > > granular enough, but it was a start to that when I was writing these patches.
> > > 
> > > Perhaps a "provider" key coupled with a "method" key would be more logical?
> > > (It changes the current definition of "method" proposed in my patches)
> > > So we would get
> > > credentials_provider = PROVIDER_LIBSECRET;
> > > credentials_method = METHOD_PASSWORD;
> > > 
> > > That would be generic enough to support GOA (PROVIDER_GOA) with OAuth2
> > > (METHOD_OAUTH2) or password-based auth, and also local plaintext
> > > (PROVIDER_PLAINTEXT) storage.
> > > 
> > > Would that be over-engineering? I'm willing to cook up some patches if it
> > > seems sensible enough to do.
> > 
> > That's probably a good way to do it, yeah. So I guess
> > LocalServiceInformation would default to using PROVIDER_LIBSECRET and either
> > METHOD_OAUTH2 or METHOD_PASSWORD depending on if it's a GMail account or
> > not, and some hypothetical GoaServiceInformation class would use
> > PROVIDER_GOA and an appropriate method depending on what kind of GOA
> > configuration it represents?
> 
> That's exactly what I had in mind! Initial account discovery may have to be
> a bit more intelligent, but with those keys it should be feasible. On a
> first run GoaAccountInformation should be getting the account's details
> (server host, port etc) from GOA, after that it should be only relying on
> the config file for that kind of stuff, and only use GOA for credentials
> management.

Thinking about it, in that case I don't know if LocalServiceInformation would become a kind of "catch-it-all" class when parsing existing accounts when Geary is starting up. At the moment Geary is parsing existing accounts by creating LocalServiceInformation instances (since I didn't intend any functional changes in the above patches).

I guess LocalServiceInformation will have to be somehow aware in all cases if credentials come from somewhere else besides libsecret, and set itself up accordingly..? Or GoaServiceInformation will have to parse the config file by itself? Maybe creating a generic "parse-config-file" class used by ServiceInformation itself (or its derivatives) would be a good idea? Ah, I'm getting lost in my ideas.

Review of attachment 361620 [details] [review]:

These look good, I've left a few style comments below if you feel like fixing them, but I don't think we have a documentation comment style guide fixed at the moment so no big deal.

::: src/engine/api/geary-service-information.vala
@@ +18,1 @@
     public const string METHOD_PASSWORD = "password";

These two public consts could have been doc comments as well ("/** ... */")

@@ +23,1 @@
     public string host { get; set; default = ""; }

And I tend to use also one-line doc comments for property comments like this that don't require multi line descriptions, i.e.:

  /** The server's address. */

@@ +60,1 @@
     public Geary.CredentialsMediator? mediator { get; set; default = null; }

For longer doc comments, they should generally be broken into a single-line summary and then a separate paras for the extra detail. E.g.:

/**
 * The credentials mediator used with the account.
 *
 * It is responsible for fetching and storing the credentials if
 * applicable.
 */

Review of attachment 361619 [details] [review]:

Looks good, best to use enums like as suggested below though.

::: src/engine/api/geary-service-information.vala
@@ +8,3 @@
+    public const string PROVIDER_LIBSECRET = "libsecret";
+
+    public const string METHOD_PASSWORD = "password";

I didn't pick up on this before, but these probably want to be values of two different enums so that the properties on ServiceInformation are strongly typed, don't they? enum ServiceMethod and enum ServiceProvider, or something?

(In reply to Oskar Viljasaar from comment #31)
> 
> That's exactly what I had in mind! Initial account discovery may have to be
> a bit more intelligent, but with those keys it should be feasible. On a
> first run GoaAccountInformation should be getting the account's details
> (server host, port etc) from GOA, after that it should be only relying on
> the config file for that kind of stuff, and only use GOA for credentials
> management.

(In reply to Oskar Viljasaar from comment #33)
> 
> Thinking about it, in that case I don't know if LocalServiceInformation
> would become a kind of "catch-it-all" class when parsing existing accounts
> when Geary is starting up. At the moment Geary is parsing existing accounts
> by creating LocalServiceInformation instances (since I didn't intend any
> functional changes in the above patches).
> 
> I guess LocalServiceInformation will have to be somehow aware in all cases
> if credentials come from somewhere else besides libsecret, and set itself up
> accordingly..? Or GoaServiceInformation will have to parse the config file
> by itself? Maybe creating a generic "parse-config-file" class used by
> ServiceInformation itself (or its derivatives) would be a good idea? Ah, I'm
> getting lost in my ideas.

Hmm, I'm not too sure about this - LocalServiceInformation might only want to be used for non-SSO services since there's no point duplicating the config, and we probably need Geary to react dynamically to configuration changes as they happen. 

So I am imagining that GoaServiceInformation would probably want to use a GOA client library or D-Bus interface to load the current config from GOA on startup, then react appropriately to dynamic config changes as the user makes them. Under that scenario, there's no point saving the GOA service config to disk locally under ~/.config/geary since it will never need to be read.

Having said that we still need to store Geary-specific config for these services like additional email addresses and signatures, but I imagine that will be stuff solely from AccountInformation. So each GOA account would still have a Geary config file loaded by AccountInformation, and that could then specify that the account uses GoaServiceInformation rather than LocalServiceInformation, which can then be constructed and go off and load the config from GOA.

This is all conjecture on my part however since I haven't really looked into GOA's client API, so do let me know if this sounds infeasible. In any case, GOA related stuff is for Bug 714876 - lets get this polished off and committed so we can make a start on GOA support proper over there.

Created attachment 362389 [details] [review]
ServiceInformation: differentiate between auth methods and providers

This uses two enums now. I used Geary.ServiceProvider for inspiration.

Created attachment 362390 [details] [review]
ServiceInformation: document all properties of this class

I think I double-checked everything? Slowly getting there.

Created attachment 362404 [details] [review]
ServiceInformation: differentiate between auth methods and providers

A few namespace usage fixes included in this time.

Created attachment 362405 [details] [review]
ServiceInformation: document all properties of this class

Rebased against the new patch.

FWIW i have got a branch up in https://github.com/tshikaboom/geary/commits/wip/768975-service-info where I've successfully moved account loading, saving and deleting client side. LocalServiceInformation has also been moved client side. It is a bit crude for the moment, but it works.

I'm going to try to polish it next week and send it here.

Review of attachment 362405 [details] [review]:

Looks good, ta! Pushed to the wip branch as commit 031ec3d.

Review of attachment 362404 [details] [review]:

I've also pushed this to the wip branch as commit e001e48

(In reply to Oskar Viljasaar from comment #41)
> FWIW i have got a branch up in
> https://github.com/tshikaboom/geary/commits/wip/768975-service-info where
> I've successfully moved account loading, saving and deleting client side.
> LocalServiceInformation has also been moved client side. It is a bit crude
> for the moment, but it works.
> 
> I'm going to try to polish it next week and send it here.

That would be excellent! I've made some initial comments over there and started watching it, but do ping me here about it so I know when you are looking for actual feedback.

Hey Oskar, just wondering where you are at with this and the work you were doing over on github? Are you happy if I merge this at least to master?

I'd like to get this in for 0.13, do you think you have the time to look at it again soonish? No worry if not, I can look at polishing it off.

Hello! I'm drowning in exams at the moment, so I won't be able to do anything this month, and it will take too much time for me to get my head back in that code. You can merge these patches all right.

I'd have liked to finish this series, but oh well, life got in the way :)

Cheers!

I ended up taking your github branch, adding a few fixes and polish and have merged it to master as commit c29c8ba6.

Thanks for your work on this Oskar!