Bug 27677 - Add Observer.Recover support
Summary: Add Observer.Recover support
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: tp-qt (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Andre Moreira Magalhaes
QA Contact: Telepathy bugs list
URL: http://git.collabora.co.uk/?p=user/an...
Whiteboard: review+
Keywords: patch
Depends on: 27670
Blocks:
  Show dependency treegraph
 
Reported: 2010-04-15 15:15 UTC by Andre Moreira Magalhaes
Modified: 2010-04-16 06:34 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments

Description Andre Moreira Magalhaes 2010-04-15 15:15:10 UTC
Add Observer.Recover support to AbstractClientObserver
Comment 1 Andre Moreira Magalhaes 2010-04-15 15:16:58 UTC
The patch adds Observer.Recover support to AbstractClientObserver.
The param is passed in the new constructor of AbstractClientObserver to avoid breaking API/ABI adding a new virtual method.
The old constructor sets the recover flag to false by default, to keep compatibility with older versions.
Comment 2 Simon McVittie 2010-04-16 05:05:07 UTC
This is more or less what I had in mind, but:

* I don't like the method name recover() - it sounds like it ought to mean "try to recover [something] [from something] and return true on success" rather than just being an accessor. Could it be hasRecover() or willRecover() or something?

* Could the old (filter) constructor be implemented as a call to the new (filter, bool) constructor? Do C++ constructors work like that?

AbstractClientObserver::AbstractClientObserver(... filter)
    : AbstractClientObserver(filter, false)
{}

AbstractClientObserver::AbstractClientObserver(... filter, bool recover)
    : mPriv(new Private)
{
    mPriv->channelFilter = channelFilter;
    mPriv->recover = recover;
}

(If that doesn't work in C++, then never mind.)

* It'd be nice to have a convention for noting "what we'll do when we break ABI" - perhaps a comment containing "ABI-break"? - and mark these two constructors as "// ABI-break: combine (filter, bool) and (filter) constructors into (filter, bool=false)", or something
Comment 3 Simon McVittie 2010-04-16 05:05:52 UTC
(The first of those points, the name of the recover() method, is the only one I feel strongly about.)
Comment 4 Andre Moreira Magalhaes 2010-04-16 06:27:31 UTC
(In reply to comment #2)
> This is more or less what I had in mind, but:
> 
> * I don't like the method name recover() - it sounds like it ought to mean "try
> to recover [something] [from something] and return true on success" rather than
> just being an accessor. Could it be hasRecover() or willRecover() or something?
Updated. Changed to shouldRecover as agreed on IRC.

> * Could the old (filter) constructor be implemented as a call to the new
> (filter, bool) constructor? Do C++ constructors work like that?
> 
> AbstractClientObserver::AbstractClientObserver(... filter)
>     : AbstractClientObserver(filter, false)
> {}
> 
> AbstractClientObserver::AbstractClientObserver(... filter, bool recover)
>     : mPriv(new Private)
> {
>     mPriv->channelFilter = channelFilter;
>     mPriv->recover = recover;
> }
> 
> (If that doesn't work in C++, then never mind.)
Unfortunately that is not possible in C++, but I improved this and moved the members initialization to a new Private class constructor.

> * It'd be nice to have a convention for noting "what we'll do when we break
> ABI" - perhaps a comment containing "ABI-break"? - and mark these two
> constructors as "// ABI-break: combine (filter, bool) and (filter) constructors
> into (filter, bool=false)", or something
I didn't change that but I agree that should have a convention here, let's discuss this more and then we can do it later.
Comment 5 Simon McVittie 2010-04-16 06:29:56 UTC
Better. Ship it!
Comment 6 Andre Moreira Magalhaes 2010-04-16 06:34:40 UTC
Merged upstream. It will be in next release 0.3.2


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.