Bug 26752

Sjoerd, Simon and I discussed this issue today. We agreed that this issue would be solved by adding a method to Account like Simon described in that chat log:

method RequestPresence ( (uss): Simple_Presence )
                     → ( (uss): Simple_Presence )

That is: rather than calling Set('...Account', 'RequestedPresence', status), the UI would call RequestPresence(status), which would return successfully with the status that was actually set. So in the case described in the bug where the status can't be changed as requested, it would return the previous, unchanged status. If the account's not online, it would not return until the account has been brought online or an error has occurred in doing so.

This needs writing up as a proper spec. addition, and then implementing in MC and TpQt4.

(In reply to comment #1)
> Sjoerd, Simon and I discussed this issue today. We agreed that this issue would
> be solved by adding a method to Account like Simon described in that chat log:
> 
> method RequestPresence ( (uss): Simple_Presence )
>                      → ( (uss): Simple_Presence )
> 
> That is: rather than calling Set('...Account', 'RequestedPresence', status),
> the UI would call RequestPresence(status), which would return successfully with
> the status that was actually set. So in the case described in the bug where the
> status can't be changed as requested, it would return the previous, unchanged
> status. If the account's not online, it would not return until the account has
> been brought online or an error has occurred in doing so.
> 
> This needs writing up as a proper spec. addition, and then implementing in MC
> and TpQt4.

I was thinking about this today and I see some problems with this solution:
1 - There is no way to know why the change actually failed (NetworkError, UnsupportedStatus, ...).
2 - There is no point in returning the same previous presence as the user already has this information, either cached (tp-qt4 for example caches the current presence) or using Get('...Account', 'CurrentPresence')
3 - What if it takes too long to return? Some bindings may not be able to set the timeout to ~infinite. QtDBus didn't have this until 4.5 and I haven't tested this support so far.
4 - Let's imagine you have 2 applets (one sitting in your desktop and another one in the system tray) showing the current presence. User tries to change the presence, both applets should be in sync (blink for example) until the presence is changed. This can't be achieved with the proposed solution.

I can think of 2 solutions for this based on the issues above

1 - Ignoring issue #3 and #4
    Add a method RequestPresence(Status) that would return nothing and raise an error on failure.

    RequestPresence(Status) -> nothing
    Errors: Define possible errors here

    It would return successfully when the requested status is actually set, or raise an error if something failed.

2 - Add 2 new signals:
    
    1 - CurrentPresenceChangeRequested(status)
    Emitted when Set("...Account", "RequestedPresence", status) is called. It will indicate that someone is trying to change the presence to "status".

    2 - CurrentPresenceChangeFailed(errorName, errorMessage)
    Emitted if Set("...Account", "RequestedPresence", status) fails.

    If the user tries to set the requested presence with different statuses N consecutive times and the change didn't happened or failed yet, emit CurrentPresenceChangeRequested N times and emit AccountPropertyChanged({CurrentPresence, status}) or CurrentPresenceChangeFailed only once when the change occurs or all attempts fail.

    This way separate UIs will be able to be in sync with what is actually happening, trying to set a requested presence is in progress, failed or succeeded.

    Use cases:
    1 - App A calls Set("...Account", "RequestedPresence", status)
    2 - CurrentPresenceChangeRequested(status) is emitted (all UIs start blinking the icon or whatever)
    3 - Change succeeded and AccountPropertyChanged({"CurrentPresence", status}) is emitted (all UIs update stop blinking the icon and update the status)

    1 - App A calls Set("...Account", "RequestedPresence", status)
    2 - CurrentPresenceChangeRequested(status) is emitted (all UIs start blinking the icon or whatever)
    3 - Change failed and CurrentPresenceChangeFailed(UnsupportedStatus, "Status not supported by this Account") is emitted (all UIs stop blinking the icon and update the status to CurrentPresence)
    
Please let me know what do you think.

When writing spec for this, please consider the interaction with the global/default presence mechanism in Bug #24104 - it might be worth specifying and implementing them at the same time.

Perhaps what we actually want is a simpler version of your proposal 2:

property Account.ChangingPresence: b
    If true, a change to the presence of this account is in progress.

    Whenever _RequestedPresence_ is set, ChangingPresence MUST change
    to True, and the two property changes MUST be emitted in the same
    _AccountPropertyChanged_ signal, before the Set method returns.

    When the account manager succeeds or fails in changing the presence,
    or the connection disconnects due to an error, ChangingPresence MUST
    change to False as part of the same _AccountPropertyChanged_ signal.

In cases where the connection disconnects or fails to connect, we already signal why, as ConnectionStatusChangeReason, and after the API proposed in Bug #21200 is implemented, we'll also put a more detailed version in ConnectionError. In MC git master (will be 5.5.0 once released), AccountPropertyChanged always aggregates changes that happened together, and always separates changes that didn't, which will make this signal easier to interpret.

For all other failed presence changes, the only reason I can think of why it could possibly fail is "the new value isn't supported, so I used the nearest I could get", so I don't think we need to signal why.

Language bindings for AccountPropertyChanged should ideally apply all the changes to the Tp[::]Account's internal cache first, while remembering what changes they made, and only emit GLib/Qt change-notification signals for *any* of the changes after they've updated their internal state for *all* the changes.

>     Whenever _RequestedPresence_ is set, ChangingPresence MUST change
>     to True

This wording isn't right; we don't want disabled or invalid accounts to be ChangingPresence, since the change would never complete. A better phrasing would be something like:

    Whenever _RequestedPresence_ is set on an account that could
    go online, or whenever an account with a non-offline
    _RequestedPresence_ becomes able to go online (for instance
    because _Enabled_ or _Valid_ changes to True), ...

(By _Foo_ I mean "hyperlink to Foo".)

Implemented a slightly modified version of Account.ChangingPresence. The implementation changes the property to true whenever Connection.SimplePresence.SetPresence is called and back to false when it finishes either with an error or successfully (Connection.SimplePresence.PresenceChanged).

MC branch is:
http://git.collabora.co.uk/?p=user/andrunko/telepathy-mission-control.git;a=shortlog;h=refs/heads/account-changing-presence

Spec:
http://git.collabora.co.uk/?p=user/andrunko/telepathy-spec.git;a=shortlog;h=refs/heads/account-changing-presence

TelepathyQt4 impl:
http://git.collabora.co.uk/?p=user/andrunko/telepathy-qt4.git;a=shortlog;h=refs/heads/account-changing-presence

(In reply to comment #5)
> Implemented a slightly modified version of Account.ChangingPresence. The
> implementation changes the property to true whenever
> Connection.SimplePresence.SetPresence is called

What's the difference between my proposal, and what you implemented? Is it this, or something else?

* my proposal: ChangingPresence is set as soon as we "want to" change presence
* your implementation: ChangingPresence is set only when we actually call SetPresence

If it's that, then I prefer my version. Here's a sequence of events which should hopefully illustrate why:

* initially:
    Connection = "/"
    ConnectionStatus = DISCONNECTED
    RequestedPresence = CurrentPresence = (OFFLINE, "offline", "")
    ChangingPresence = TRUE
* UI calls Set(RequestedPresence, (HIDDEN, "hidden", "I'm not here"))
* emit AccountPropertiesChanged ({
    Connection: "/",
    ConnectionStatus: CONNECTING,
    RequestedPresence: (HIDDEN, "hidden", "I'm not here"),
    ChangingPresence: TRUE,
  })
* call RequestConnection on CM
* return from Set with success
* time passes
* CM.RequestConnection returns /my/conn
* emit AccountPropertiesChanged ({
    Connection: "/my/conn",
    ConnectionStatus: CONNECTING,
  })
* call SetPresence on connection
* call Connect on connection
* time passes
* Connect returns success
* time passes
* StatusChanged(CONNECTED)
* emit AccountPropertiesChanged ({
    Connection: "/my/conn",
    ConnectionStatus: CONNECTED,
  })
* call GetPresence to fetch the real presence
* oops! CM didn't support HIDDEN and fell back to BUSY
* GetPresence returns (BUSY, "dnd", "I'm not here")
* emit AccountPropertiesChanged ({
    Connection: "/my/conn",
    ConnectionStatus: CONNECTED,
    ChangingPresence: FALSE,
    CurrentPresence: (BUSY, "dnd", "I'm not here"),
  })

(In reply to comment #6)
> (In reply to comment #5)
> > Implemented a slightly modified version of Account.ChangingPresence. The
> > implementation changes the property to true whenever
> > Connection.SimplePresence.SetPresence is called
> 
> What's the difference between my proposal, and what you implemented? Is it
> this, or something else?
> 
> * my proposal: ChangingPresence is set as soon as we "want to" change presence
> * your implementation: ChangingPresence is set only when we actually call
> SetPresence
> 
> If it's that, then I prefer my version. Here's a sequence of events which
> should hopefully illustrate why:
That is it. I chose to do it this way as we signal ChangingPresence really when the presence is changing, not when we asked for it. So if we are offline and someones asks to change the presence to Foo, we will first attempt to connect, then change change the presence. Only when SetPresence is called the ChangingPresence changes. I can change it and move the Set(ChangingPresence, TRUE) code to set_requested_presence in mcd-account.c, but we would need to cover other cases as when the connection cannot go online, as in connection_error and connection_abort to change the ChangingPresence property back to false on error.
 
This is what the impl does:
 * initially:
 *     Connection = "/"
 *     ConnectionStatus = DISCONNECTED
 *     RequestedPresence = CurrentPresence = (OFFLINE, "offline", "")
 *     ChangingPresence = FALSE
 * UI calls Set(RequestedPresence, (HIDDEN, "hidden", "I'm not here"))
 * emit AccountPropertiesChanged ({
     Connection: "/",
     ConnectionStatus: CONNECTING,
     RequestedPresence: (HIDDEN, "hidden", "I'm not here"),
   })
 * call RequestConnection on CM
 * return from Set with success
 * time passes
 * CM.RequestConnection returns /my/conn
 * emit AccountPropertiesChanged ({
     Connection: "/my/conn",
     ConnectionStatus: CONNECTING,
   })
 * ChangingPresence = TRUE
 * emit AccountPropertiesChanged ({
     ChangingPresence: TRUE,
   })
 * call SetPresence on connection
 * call Connect on connection
 * time passes
 * Connect returns success
 * time passes
 * StatusChanged(CONNECTED)
 * emit AccountPropertiesChanged ({
     Connection: "/my/conn",
     ConnectionStatus: CONNECTED,
   })
 * call GetPresence to fetch the real presence
 * oops! CM didn't support HIDDEN and fell back to BUSY
 * GetPresence returns (BUSY, "dnd", "I'm not here")
 * emit AccountPropertiesChanged ({
     Connection: "/my/conn",
     ConnectionStatus: CONNECTED,
     ChangingPresence: FALSE,
     CurrentPresence: (BUSY, "dnd", "I'm not here"),
   })

(In reply to comment #7)
> That is it. I chose to do it this way as we signal ChangingPresence really when
> the presence is changing, not when we asked for it. So if we are offline and
> someones asks to change the presence to Foo, we will first attempt to connect,
> then change change the presence. 

That doesn't sound right. Conceptually, we're already "trying to change the presence" as soon as Set() is called.

> I can change it and move the Set(ChangingPresence,
> TRUE) code to set_requested_presence in mcd-account.c, but we would need to
> cover other cases as when the connection cannot go online

Please do.

Also, I'm not sure that ChangingPresence should always be cleared by a PresencesChanged signal for the self-handle. Consider this:

* we call SetPresence for HIDDEN (but the CM doesn't support "pre-loading" presences, so it will fail)
* we call Connect
* time passes
* SetPresence returns failure
* Connect returns success
* time passes
* StatusChanged(CONNECTED)
* PresencesChanged({self: available}) [1]
* we respond to StatusChanged by fetching Statuses
* time passes
* Statuses comes back
* we can now call SetPresence again; knowing that HIDDEN is impossible, we'll settle for BUSY
* PresencesChanged({self: busy}) [2]
* SetPresence returns success [3]

You'd clear ChangingPresence at [1]. I think it shouldn't be cleared until [2] or [3].

(Or, to put it another way: ChangingPresence is TRUE if we have a SetPresence call either in-flight or queued up.)

I think it might be easiest to decompose the state into several booleans (SetPresence in flight / we're connecting / ...), and have the actual value of ChangingPresence computed from those. It's OK if you sometimes emit AccountPropertyChanged for ChangingPresence even if it didn't actually change (although you could avoid that by caching a boolean, last_emitted_changing_presence).

(In reply to comment #8)
> You'd clear ChangingPresence at [1]. I think it shouldn't be cleared until [2]
> or [3].

Note that it makes intuitive sense to have ChangingPresence: FALSE appear in the same signal as a new value for CurrentPresence - that way, the widget goes to the correct end state immediately.

Updated my branch with the suggested modifications. tp-qt4 and spec patches unchanged.

Some notes:
- ChangingPresence=TRUE happens whenever mcd_account_request_presence_int or a temporary presence is requested is called, which happens inside Set(RequestedPresence) and also when going to/from idle, enable/disable, auto presence, so the user can rely on this property to check whether a change in the presence is in place.
- ChangingPresence=FALSE only happens when the we are sure the presence "set" is supported (set to hidden on gabble for eg will emit CurrentPresence=hidden then CurrentPresence=busy and only when busy it will set ChangingPresence to FALSE).

Logs from a test app I wrote (testing on gabble):

Setting to hidden:

andrunko@andrunko-laptop:~/work/telepathy/telepathy-qt4/examples/accounts$ ./accounts 
requested presence changed to "hidden" 
changing presence changed to "true" 
connection status changed to 1 
connection status changed to 0 
current presence changed to "hidden" 
current presence changed to "dnd" 
changing presence changed to "false" 

Setting to available:

andrunko@andrunko-laptop:~/work/telepathy/telepathy-qt4/examples/accounts$ ./accounts 
requested presence changed to "available" 
changing presence changed to "true" 
connection status changed to 1 
connection status changed to 0 
current presence changed to "available" 
changing presence changed to "false" 

Setting to available and then hidden:

andrunko@andrunko-laptop:~/work/telepathy/telepathy-qt4/examples/accounts$ ./accounts 
changing presence changed to "true" 
requested presence changed to "available" 
connection status changed to 1 
connection status changed to 0 
current presence changed to "available" 
changing presence changed to "false" 
requested presence changed to "hidden" 
changing presence changed to "true" 
current presence changed to "dnd" 
changing presence changed to "false"

(In reply to comment #10)
> andrunko@andrunko-laptop:~/work/telepathy/telepathy-qt4/examples/accounts$
> ./accounts 
> requested presence changed to "hidden" 
> changing presence changed to "true" 
> connection status changed to 1 
> connection status changed to 0 
> current presence changed to "hidden" 
> current presence changed to "dnd" 
> changing presence changed to "false" 

How do these map into AccountPropertiesChanged signals? (I'd recommend using the lower-level tp-qt4 or tp-glib API to bind to that signal directly, so you can print "--------" or something after each signal.)

This output looks sensible, though.

Specification
=============

> +        <p>This property indicates whether the account's
> +          <tp:member-ref>Connection</tp:member-ref> is changing presence</p>

I preferred my version: "If true, a change to the presence of this account is in progress."

> +
> +        <tp:rationale>
> +          <p>Use cases:</p>
> +
> +          <ul>
> +            <li>user has two applets (one in the desktop area and another
> +             one in the system tray area) showing the current presence.
> +             User tries to change the presence and ChangingPresence changes > to
> +             true. Now both applets start to blink. After the presence is set,
> +             ChangingPresence changes to false and both applets now show the
> +             <tp:member-ref>CurrentPresence</tp:member-ref>.</li>
> +          </ul>
> +        </tp:rationale>

That's only one use case, so it doesn't really need to be in a list :-)

I'd be inclined to move this to the end of the docstring so the whole description of how the property works comes first, and shorten it to something like:

    <tp:rationale>
      <p>This allows UIs to indicate that a presence change is in progress
        or has finished, even if the change was initiated by a different
        UI.</p>

      <p>For instance, Maemo 5 and Empathy indicate a presence change by
        having the presence indicator alternate between the RequestedPresence
        and the CurrentPresence; they should start blinking when
        ChangingPresence becomes true, and stop when it becomes false.</p>
    </tp:rationale>

> +        <p>The AccountManager MUST set this property to true whenever a change
> +        in the account's <tp:member-ref>Connection</tp:member-ref> presence is
> +        requested. When the change is finished, either successfully or with an
> +        error, this property MUST be set to false.</p>
> +      </tp:docstring>

I preferred my version, which makes it explicit how things should combine into AccountPropertyChanged signals:

    Whenever _RequestedPresence_ is set on an account that could
    go online, or whenever an account with a non-offline
    _RequestedPresence_ becomes able to go online (for instance
    because _Enabled_ or _Valid_ changes to True), ChangingPresence MUST
    change to True, and the two property changes MUST be emitted in the same
    _AccountPropertyChanged_ signal, before the Set method returns.

    When the account manager succeeds or fails in changing the presence,
    or the connection disconnects due to an error, ChangingPresence MUST
    change to False as part of the same _AccountPropertyChanged_ signal.

Mission Control
===============

The MC branch looks good now, but please don't merge it just yet - I'd much rather do a spec release with this change included, get that into telepathy-glib, and get MC depending on that telepathy-glib.

telepathy-qt4
=============

> + * \fn void Account::isChangingPresence(bool value);

I think you mean changingPresence(bool).

Other than that, it looks OK, conditional on updating telepathy-qt4 to a spec release that includes this property.

(In reply to comment #12)
> Specification
> =============
> 
> > +        <p>This property indicates whether the account's
> > +          <tp:member-ref>Connection</tp:member-ref> is changing presence</p>
> 
> I preferred my version: "If true, a change to the presence of this account is
> in progress."
Done

> > +
> > +        <tp:rationale>
> > +          <p>Use cases:</p>
> > +
> > +          <ul>
> > +            <li>user has two applets (one in the desktop area and another
> > +             one in the system tray area) showing the current presence.
> > +             User tries to change the presence and ChangingPresence changes > to
> > +             true. Now both applets start to blink. After the presence is set,
> > +             ChangingPresence changes to false and both applets now show the
> > +             <tp:member-ref>CurrentPresence</tp:member-ref>.</li>
> > +          </ul>
> > +        </tp:rationale>
> 
> That's only one use case, so it doesn't really need to be in a list :-)
> 
> I'd be inclined to move this to the end of the docstring so the whole
> description of how the property works comes first, and shorten it to something
> like:
>     <tp:rationale>
>       <p>This allows UIs to indicate that a presence change is in progress
>         or has finished, even if the change was initiated by a different
>         UI.</p>
> 
>       <p>For instance, Maemo 5 and Empathy indicate a presence change by
>         having the presence indicator alternate between the RequestedPresence
>         and the CurrentPresence; they should start blinking when
>         ChangingPresence becomes true, and stop when it becomes false.</p>
>     </tp:rationale>
Done
 
> > +        <p>The AccountManager MUST set this property to true whenever a change
> > +        in the account's <tp:member-ref>Connection</tp:member-ref> presence is
> > +        requested. When the change is finished, either successfully or with an
> > +        error, this property MUST be set to false.</p>
> > +      </tp:docstring>
> 
> I preferred my version, which makes it explicit how things should combine into
> AccountPropertyChanged signals:
> 
>     Whenever _RequestedPresence_ is set on an account that could
>     go online, or whenever an account with a non-offline
>     _RequestedPresence_ becomes able to go online (for instance
>     because _Enabled_ or _Valid_ changes to True), ChangingPresence MUST
>     change to True, and the two property changes MUST be emitted in the same
>     _AccountPropertyChanged_ signal, before the Set method returns.
> 
>     When the account manager succeeds or fails in changing the presence,
>     or the connection disconnects due to an error, ChangingPresence MUST
>     change to False as part of the same _AccountPropertyChanged_ signal.
> 
Done

> Mission Control
> ===============
> 
> The MC branch looks good now, but please don't merge it just yet - I'd much
> rather do a spec release with this change included, get that into
> telepathy-glib, and get MC depending on that telepathy-glib.
Great, tnx
 
> telepathy-qt4
> =============
> 
> > + * \fn void Account::isChangingPresence(bool value);
> 
> I think you mean changingPresence(bool).
I Used isChangingPresence cause we use isSomething for other boolean accessors, as in isValid/isEnabled/... but I can change it if you prefer.

> Other than that, it looks OK, conditional on updating telepathy-qt4 to a spec
> release that includes this property.
Yep, tp-qt4 will only include this when we update the spec, it's just that I already implemented it so I could test it easily.

(In reply to comment #13)
> > telepathy-qt4
> > =============
> > 
> > > + * \fn void Account::isChangingPresence(bool value);
> > 
> > I think you mean changingPresence(bool).
> I Used isChangingPresence cause we use isSomething for other boolean accessors,
> as in isValid/isEnabled/... but I can change it if you prefer.

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 :-)

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);

I've filed Bug #28018 for telepathy-qt4 and Bug #28019 for MC.

review+ from me; requesting spec cabal approval, since this change will go straight in as stable D-Bus API.

wjt, your thoughts on this would be appreciated :-)

Added in 0.19.6.