Bug 54151

Summary: Flatten McdStorage into McdPluginAccountManager
Product: Telepathy Reporter: Simon McVittie <smcv>
Component: mission-controlAssignee: Simon McVittie <smcv>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: normal    
Priority: medium CC: jonny.lamb, smcv, venkythegeek, william.jon.mccann, xclaesse
Version: unspecifiedKeywords: patch
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Bug Depends on:    
Bug Blocks: 35896    
Attachments: [1/5] McdMaster: fail to build if umask() is missing
[2/5] McdPluginAccountManager: fold interface methods into their implementations
[3/5] McdStorage: remove _mcd_storage_store_connections
[4/5] McdStorage, McdPluginAccountManager: squash into one class
[5/5] plugin-account.c: rename to mcd-storage.c

Description Simon McVittie 2012-08-28 09:51:20 UTC
+++ This bug was initially created as a clone of Bug #35896 +++

McdPluginAccountManager is a concrete class in MC. It implements the McpAccountManager GInterface, which is the API presented by MC to account storage plugins so they can call back into MC.

The reason Mcp... is a GInterface is to present a rather limited API to plugins, so we're not prevented from making changes to the rest of MC's internals.

McdPluginAccountManager also implements McdStorage, a GInterface used by the rest of MC's internals. This is, as far as I can tell, pointless: it's the only implementation of that interface, and every module that could access McdStorage can equally well access McdPluginAccountManager directly.

I think we should flatten McdPluginAccountManager (class) and McdStorage (interface) into a single class. To reduce the diff churn, its name should be the name under which most of MC uses it, which is McdStorage.
Comment 1 Simon McVittie 2012-08-28 09:52:51 UTC
Created attachment 66210 [details] [review]
[1/5] McdMaster: fail to build if umask() is missing

We ought to be able to rely on umask() for files created since 5.2.2,
at least on Unix.

---

See Bug #35896. Not really part of this bug, but it's easy to do and stops us having to chmod() things created by Bug #35896.
Comment 2 Simon McVittie 2012-08-28 09:53:14 UTC
Created attachment 66211 [details] [review]
[2/5] McdPluginAccountManager: fold interface methods into  their implementations

Nothing except McdPluginAccountManager implements McdStorage, and nothing
ever will or should, so we can squash the two together.
Comment 3 Simon McVittie 2012-08-28 09:53:36 UTC
Created attachment 66212 [details] [review]
[3/5] McdStorage: remove _mcd_storage_store_connections

I'm not sure why this was in McdStorage at all. It's a bit of a layering
violation: AccountManager has a Storage, so Storage shouldn't call into
AccountManager. We can fix the layering violation by adding a signal
to McdAccount.
Comment 4 Simon McVittie 2012-08-28 09:53:57 UTC
Created attachment 66213 [details] [review]
[4/5] McdStorage, McdPluginAccountManager: squash into one  class

The Storage name stays, to make the diff more readable. The header 
is mcd-storage.h but the implementation is still plugin-account.c,
for the same reason.
Comment 5 Simon McVittie 2012-08-28 09:56:14 UTC
Created attachment 66214 [details] [review]
[5/5] plugin-account.c: rename to mcd-storage.c

---

Here's `git show -M`, which is easier to review: as you can see, it's just a rename and the necessary changes to src/Makefile.am to compensate for it.

commit 4ba55e2ebbb080f37d731a750bf7c4880c560ea5
Author: Simon McVittie <simon.mcvittie@collabora.co.uk>
Date:   2012-08-27 19:04:16 +0100

    plugin-account.c: rename to mcd-storage.c

diff --git a/src/Makefile.am b/src/Makefile.am
index 0d09b7d..b9cd9df 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -159,6 +159,7 @@ libmcd_convenience_la_SOURCES = \
        mcd-slacker.c \
        mcd-slacker.h \
        mcd-transport.c \
+       mcd-storage.c \
        mcd-storage.h \
        plugin-dispatch-operation.c \
        plugin-dispatch-operation.h \
@@ -166,7 +167,6 @@ libmcd_convenience_la_SOURCES = \
        plugin-loader.h \
        plugin-request.c \
        plugin-request.h \
-       plugin-account.c \
        request.c \
        request.h \
        sp_timestamp.h \
diff --git a/src/plugin-account.c b/src/mcd-storage.c
similarity index 100%
rename from src/plugin-account.c
rename to src/mcd-storage.c
Comment 6 Xavier Claessens 2012-08-28 10:36:11 UTC
This makes perfect sense, and the patches looks good to me.

+1
Comment 7 Simon McVittie 2012-08-28 15:16:49 UTC
Fixed in git for 5.13.1, thanks

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.