Bug 28018 - Bind Account.ChangingPresence
Summary: Bind Account.ChangingPresence
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: tp-qt (show other bugs)
Version: unspecified
Hardware: Other All
: medium enhancement
Assignee: Andre Moreira Magalhaes
QA Contact: Telepathy bugs list
URL:
Whiteboard:
Keywords:
Depends on: 26752 28536
Blocks:
  Show dependency treegraph
 
Reported: 2010-05-07 07:26 UTC by Simon McVittie
Modified: 2010-06-16 07:23 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments

Description Simon McVittie 2010-05-07 07:26:24 UTC
+++ This bug was initially created as a clone of Bug #26752 +++
Comment 1 Simon McVittie 2010-05-07 07:29:31 UTC
Minor changes needed, see <https://bugs.freedesktop.org/show_bug.cgi?id=26752#c14>

A spec release is also needed.
Comment 2 Andre Moreira Magalhaes 2010-05-07 07:33:14 UTC
In reply to https://bugs.freedesktop.org/show_bug.cgi?id=26752#c14

> No, I was complaining about the *signal*'s documentation. Compare:
> 
>     // getter
>     bool Tp::Account::isChangingPresence(void) const;
> 
>     // signal
>     void Tp::Account::changingPresence(bool);
> 
> This is more subtle than it ought to be :-)
True I missed that. will fix.
 
> Would it perhaps be clearer if we had two signals, like this?
> 
>     // getter
>     bool Tp::Account::isChangingPresence(void) const;
> 
>     // signal (part 1)
>     void Tp::Account::presenceChangeStarted(void);
> 
>     // signal (part 2)
>     void Tp::Account::presenceChangeFinished(void);
Hmm I am not sure about this, it surely would be more clear, but having one signal I believe it's easier to implement. We can have on signal handler to start/stop blinking the UI for example. Not sure here
Comment 3 Simon McVittie 2010-05-07 07:37:49 UTC
(In reply to comment #2)
> Hmm I am not sure about this, it surely would be more clear, but having one
> signal I believe it's easier to implement. We can have on signal handler to
> start/stop blinking the UI for example. Not sure here

Yeah, I'm not sure about this either - normally I'd vote for a smaller and more consistent API (i.e. one signal with a boolean argument, like you already implemented), but it does lead to confusingly similar method and signal names here, so the question is whether an larger and less consistent API is a worthwhile price for better clarity.

Keep it in the back of your mind for now, and we can make a final decision after the spec gets released?
Comment 4 Andre Moreira Magalhaes 2010-06-15 08:23:01 UTC
Updated branch to fix the issue with the signal documentation and depend on the new spec.

Chose to keep using changingPresence() for the signal name, I think it's the best way here, but I am opened for suggestions.
Comment 5 Will Thompson 2010-06-15 11:04:37 UTC
This looks good to me.
Comment 6 Andre Moreira Magalhaes 2010-06-16 07:23:12 UTC
Merged upstream. It will be in next release 0.3.5


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.