| Summary: | CM should know the account path of its connections | ||
|---|---|---|---|
| Product: | Telepathy | Reporter: | Xavier Claessens <xclaesse> |
| Component: | general | Assignee: | Telepathy bugs list <telepathy-bugs> |
| Status: | RESOLVED FIXED | QA Contact: | Telepathy bugs list <telepathy-bugs> |
| Severity: | normal | ||
| Priority: | medium | Keywords: | patch |
| Version: | git master | ||
| Hardware: | Other | ||
| OS: | All | ||
| Whiteboard: | review- | ||
| i915 platform: | i915 features: | ||
| Bug Depends on: | 71093 | ||
| Bug Blocks: | |||
|
Description
Xavier Claessens
2014-01-24 20:04:20 UTC
tp-glib: http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=account-path-suffix mc: http://cgit.collabora.com/git/user/xclaesse/telepathy-mission-control.git/log/?h=account-path-suffix gabble: http://cgit.collabora.com/git/user/xclaesse/telepathy-gabble.git/log/?h=account-path-suffix The parameter should be in the spec.
Regarding MC, I don't think I really want this special parameter to be in Get("...Account", "Parameters") - it isn't really part of user configuration at all. Could you maybe splice it in in _mcd_account_connection_begin() instead, so it's passed to RequestConnection() but isn't in Get("...Account", "Parameters")? That's what we used to do when MC had special code for injecting system-wide proxy configuration into CMs.
Regarding telepathy-glib, I'd like a tp_base_connection_get_account_path_suffix().
My patch from <https://bugs.freedesktop.org/show_bug.cgi?id=71093#c9>, and my comment there, should make the MC side a lot easier to get right. (In reply to comment #0) > For example offline roster cache (bug #62378) done from the CM should store > its DB in "~/.cache/telepathy/roster/gabble/jabber/acc0/user@example.com". OK. I think I'd prefer to shuffle the paths round so the CM comes first. That'd be really good for AppArmor, SELinux and similar - telepathy-foo could be confined so it can only write ~/.cache/telepathy/foo and ~/.local/share/telepathy/foo. We could perhaps even teach MC to delete XDG_CACHE_HOME/telepathy/$cm/$protocol/$rest and XDG_DATA_HOME/telepathy/$cm/$protocol/$rest (*after* calling Disconnect) when it deletes accounts, and we'd automatically have CM data cleaned up without needing an equivalent of CM.I.ExternalStorage? But we'd still need a CM.I.ExternalStorage equivalent for the "Haze should be able to have a persistent directory equivalent to ~/.purple" use-case, because libpurple shares one directory between all accounts. So we should decide up-front whose responsibility it is to clean up - MC or CMs - and if it's the CMs, provide API for MC to call into them. > Another example is telepathy-logger could also store its logs in similar > location. It should probably be somewhere where it won't be deleted with the account, so XDG_DATA_HOME/telepathy/logger/$cm/$protocol/$rest or something. Branch updated. I also made the property construct-only. I like the idea of MC clearing cached files when deleting accounts. I had the idea of writing a folks2 MC plugin to purge personas from the merge DB when an account is deleted as well. I did not add public _get_account_path_suffix() function yet, following the rule of "make api public when needed". For the offline-cache, an internal function is enough, just like that commit in the offline-cache branch: http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/commit/?h=offline-cache&id=08caeeab389ef81c0f7554ba7b6b058d76c56a90 (In reply to comment #4) > I did not add public _get_account_path_suffix() function yet, following the > rule of "make api public when needed". If it's a readable property, then it's already public API whether you like it or not, so it might as well have a getter. (I haven't re-reviewed yet.) Yes, please do add a getter. You've effectively already made it public API so you might as well. Spec wording is still missing. > + param_names = tp_protocol_dup_param_names (protocol); > + if (tp_strv_contains ((const gchar * const *) param_names, tp_protocol_has_param() is easier to use and more efficient. > + strrchr (account->priv->unique_name, '/') + 1); Oh, for the account /o/fd/Tp/gabble/jabber/blahblah you're just passing in "blahblah" and not "gabble/jabber/blahblah"? I'd assumed it would be the whole unique tail. The fact that it's taken me this long to realize what you meant means the documentation could probably be better. Explicitly mentioning an example would help a lot, I think: such as "chris_40example_2ecom0" for the account whose object path is %TP_ACCOUNT_OBJECT_PATH_BASE + "gabble/jabber/chris_40example_2ecom0" or such as "gabble/jabber/chris_40example_2ecom0" for the account whose object path is %TP_ACCOUNT_OBJECT_PATH_BASE + "gabble/jabber/chris_40example_2ecom0" I realize it's redundant, but I'd prefer "gabble/jabber/blahblah" so we only have two ways to talk about an account (object-path or suffix), rather than three (object-path, semi-machine-readable suffix including CM and protocol, and opaque suffix of the suffix). Also, in practice I expect CMs will often be writing to $XDG_CACHE_HOME/telepathy/gabble/jabber/blahblah or something, so it might as well be "precomposed". > + tp_asv_set_string (ctx->params, g_strdup ("account-path-suffix"), When doing tricky things with hash tables and their memory-allocation models, please write what you mean "the long way", with g_hash_table_insert(). You can use tp_g_value_slice_new_string() to make it a bit shorter, but tp_asv_set_string() is documented in terms of hash tables created with tp_asv_new(), so please don't use it with a g_strdup'd key in a hash table whose memory-allocation model differs (even though in this case it happens to work). Also, if you're going to rely on ctx->params being { g_strdup'd => slice-allocated }, please make sure to say so in the doc-comment of mcd_account_coerce_parameters(), so future maintainers know to be careful when changing it. Bonus points if you say which function relies on it. Ok, here it is: spec: http://cgit.collabora.com/git/user/xclaesse/telepathy-spec.git/log/?h=account-path-suffix tp-glib: http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=account-path-suffix gabble: http://cgit.collabora.com/git/user/xclaesse/telepathy-gabble.git/log/?h=account-path-suffix Looks great so far, please update your MC branch as described in Comment #6. Already did, forgot to link: http://cgit.collabora.com/git/user/xclaesse/telepathy-mission-control.git/log/?h=account-path-suffix + * Returns: A GHashTable with g_strdup'ed keys and tp_g_value_slice_dup'ed + * values. "... values. Be careful: callers rely on that memory allocation model." Otherwise fine, feel free to push these. Patches for the other major CMs (Salut, Haze, Rakia, Idle) also welcome. (In reply to comment #10) > + * Returns: A GHashTable with g_strdup'ed keys and tp_g_value_slice_dup'ed > + * values. > > "... values. Be careful: callers rely on that memory allocation model." Did that and pushed all my branches. > Otherwise fine, feel free to push these. Patches for the other major CMs > (Salut, Haze, Rakia, Idle) also welcome. I was going to add it "when needed". So far my only usage is offline-cache and it's implemented only in gabble. Haze will probably need it has well. Salut/Rakia/Idle could be out-of-scope. tp-glib bits released in 0.23.2 so you can consider merging the other branches. Oops, I already pushed gabble, forgot to wait for the release. Bumped the required tp-glib version in gabble master. Let's close this bug, other CM can add the support for this parameter when needed. |
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.