Attachment 119368 Details for Bug 92153 – attachment-17384-0.html

attachment-17384-0.html

attachment-17384-0.html (text/html), 3.98 KB, created by James Smith on 2015-11-03 06:21:57 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: James Smith

Created: 2015-11-03 06:21:57 UTC

Size: 3.98 KB

patch

obsolete

><html><head><meta http-equiv="Content-Type" content="text/html; charset=UTF-8"></head><body ><div>Does the notify fetch the updated property in advance? Because reversing the order does fix a stale property issue when immediately fetching the property by the signal, e.g. when fetching the requested presence of a number of accounts in kci global presence, a stale presence can be brought in on a fresh signal and taint the global presence for that global presence change. The reason it doesn't happen every presence change is that the signal property isn't used anyway and the account property isn't yet changed to the new value and the current presence has a higher priority than the expected presence, causing the old presence to be kept.&nbsp;If the stale presence is higher priority when the last account signals this is how we get a stuck presence. Without this patch it is assumed that the signal is always right because it has a property attached and the notify line would seem redundant because it doesn't send any updated property. I like this approach reversed, because -any- signal comes after -any- consideration of the corresponding property, especially if the signal is used to query the property immediately. Either way, I've been using this patch to fix the global presence issue for a while now, it looks correct and doesn't seem to have any regressions. I briefly second-guessed also changing the serviceNameChanged and profileChanged signals, because the above behaviour may have been relied on so that the account was still adressable by these properties until the new property was useable and there doesn't seem to be any reason to rely on these properties to access the account while they are being changed anyway.</div><div></div><br><br>-------- Original message --------<br>From: bugzilla-daemon@freedesktop.org <br>Date: 11-02-2015  11:42 AM  (GMT-07:00) <br>To: smithjd15@gmail.com <br>Subject: [Bug 92153] Account properties notify signals are dispatched before
  the account property is updated <br><br>
>      <p>
>        </p><div>
>            <b><a class="bz_bug_link 
>          bz_status_NEW " title="NEW - Account properties notify signals are dispatched before the account property is updated" href="https://bugs.freedesktop.org/show_bug.cgi?id=92153#c1">Comment # 1</a>
>              on <a class="bz_bug_link 
>          bz_status_NEW " title="NEW - Account properties notify signals are dispatched before the account property is updated" href="https://bugs.freedesktop.org/show_bug.cgi?id=92153">bug 92153</a>
>              from <span class="vcard"><a class="email" href="mailto:kde@davidedmundson.co.uk" title="David Edmundson &lt;kde@davidedmundson.co.uk&gt;"> <span class="fn">David Edmundson</span></a>
></span></b>
>        <pre>I'm not sure I understand
>
>parent-&gt;notify just emits propertyChanged("propertyName")
>
>
> * Notify that a property named \a propertyName changed.
> *
> * This method will emit propertyChanged() for \a propertyName.
> *
> * \todo Use for more classes beyond Account. Most importantly Contact.
> */
>void Object::notify(const char *propertyName)
>{
>    emit propertyChanged(QLatin1String(propertyName));
>}
>
>
>As far as I can tell, the only user of this is AccountSet and some unit tests.
>
>git grep propertyChanged
>TelepathyQt/account-set.cpp:            SIGNAL(propertyChanged(QString)),
>TelepathyQt/object.cpp: * This method will emit propertyChanged() for \a
>propertyName.
>TelepathyQt/object.cpp:    emit propertyChanged(QLatin1String(propertyName));
>TelepathyQt/object.h:    void propertyChanged(const QString &amp;propertyName);
>tests/dbus/account-connection-factory.cpp:               
>SIGNAL(propertyChanged(QString)),
>tests/dbus/account-connection-factory.cpp:               
>SIGNAL(propertyChanged(QString)),
>
>
>As for requested presence; it's set on line 4466 and the emit is on line 4470,
>so that's always set before the emit.</pre>
>        </div>
>      <p></p>
>      <hr>
>      <span>You are receiving this mail because:</span>
>      
>      <ul>
>          <li>You reported the bug.</li>
>      </ul>
>    
>
></body>

<html><head><meta http-equiv="Content-Type" content="text/html; charset=UTF-8"></head><body ><div>Does the notify fetch the updated property in advance? Because reversing the order does fix a stale property issue when immediately fetching the property by the signal, e.g. when fetching the requested presence of a number of accounts in kci global presence, a stale presence can be brought in on a fresh signal and taint the global presence for that global presence change. The reason it doesn't happen every presence change is that the signal property isn't used anyway and the account property isn't yet changed to the new value and the current presence has a higher priority than the expected presence, causing the old presence to be kept.&nbsp;If the stale presence is higher priority when the last account signals this is how we get a stuck presence. Without this patch it is assumed that the signal is always right because it has a property attached and the notify line would seem redundant because it doesn't send any updated property. I like this approach reversed, because -any- signal comes after -any- consideration of the corresponding property, especially if the signal is used to query the property immediately. Either way, I've been using this patch to fix the global presence issue for a while now, it looks correct and doesn't seem to have any regressions. I briefly second-guessed also changing the serviceNameChanged and profileChanged signals, because the above behaviour may have been relied on so that the account was still adressable by these properties until the new property was useable and there doesn't seem to be any reason to rely on these properties to access the account while they are being changed anyway.</div><div></div><br><br>-------- Original message --------<br>From: bugzilla-daemon@freedesktop.org <br>Date: 11-02-2015  11:42 AM  (GMT-07:00) <br>To: smithjd15@gmail.com <br>Subject: [Bug 92153] Account properties notify signals are dispatched before
  the account property is updated <br><br>
      <p>
        </p><div>
            <b><a class="bz_bug_link 
          bz_status_NEW " title="NEW - Account properties notify signals are dispatched before the account property is updated" href="https://bugs.freedesktop.org/show_bug.cgi?id=92153#c1">Comment # 1</a>
              on <a class="bz_bug_link 
          bz_status_NEW " title="NEW - Account properties notify signals are dispatched before the account property is updated" href="https://bugs.freedesktop.org/show_bug.cgi?id=92153">bug 92153</a>
              from <span class="vcard"><a class="email" href="mailto:kde@davidedmundson.co.uk" title="David Edmundson &lt;kde@davidedmundson.co.uk&gt;"> <span class="fn">David Edmundson</span></a>
</span></b>
        <pre>I'm not sure I understand

parent-&gt;notify just emits propertyChanged("propertyName")


 * Notify that a property named \a propertyName changed.
 *
 * This method will emit propertyChanged() for \a propertyName.
 *
 * \todo Use for more classes beyond Account. Most importantly Contact.
 */
void Object::notify(const char *propertyName)
{
    emit propertyChanged(QLatin1String(propertyName));
}


As far as I can tell, the only user of this is AccountSet and some unit tests.

git grep propertyChanged
TelepathyQt/account-set.cpp:            SIGNAL(propertyChanged(QString)),
TelepathyQt/object.cpp: * This method will emit propertyChanged() for \a
propertyName.
TelepathyQt/object.cpp:    emit propertyChanged(QLatin1String(propertyName));
TelepathyQt/object.h:    void propertyChanged(const QString &amp;propertyName);
tests/dbus/account-connection-factory.cpp:               
SIGNAL(propertyChanged(QString)),
tests/dbus/account-connection-factory.cpp:               
SIGNAL(propertyChanged(QString)),


As for requested presence; it's set on line 4466 and the emit is on line 4470,
so that's always set before the emit.</pre>
        </div>
      <p></p>
      <hr>
      <span>You are receiving this mail because:</span>
      
      <ul>
          <li>You reported the bug.</li>
      </ul>
    

</body>

Actions: View

Attachments on bug 92153: 118471 | 119368 | 128461 | 128535 | 128601 | 128618

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.