D8346: Remove blocking call in KStatusNotifierItem

2017-10-18 Thread David Edmundson 
davidedmundson abandoned this revision.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D8346

To: davidedmundson
Cc: xuetianweng, #frameworks

D8346: Remove blocking call in KStatusNotifierItem

2017-10-17 Thread David Edmundson 
davidedmundson added a comment.


  I've realised there's much more to do:
  
  The existing code has some super weird behaviour.
  
  Find there's an SNI watcher, but no host (i.e kded but no plasmashell) - 
that's fine, we don't check and create one anyway
  But if a host gets added/removed - then we check
  
  That makes no sense.
  
  IMHO, there's no point checking for hosts, if we have an SNI watcher, we're 
clearly on a system that supports SNIs, even if the view temporarily disappears.
  At which point I should just kill this code.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D8346

To: davidedmundson
Cc: xuetianweng, #frameworks

D8346: Remove blocking call in KStatusNotifierItem

2017-10-17 Thread Xuetian Weng 
xuetianweng added inline comments.

INLINE COMMENTS

> kstatusnotifieritem.cpp:882
> +
> +QDBusPendingReply pendingReply = 
> statusNotifierWatcher->connection().call(call);
> +auto watcher = new QDBusPendingCallWatcher(pendingReply);

The template is not necessary here. Maybe auto or QDBusPendingCall ?

> kstatusnotifieritem.cpp:883
> +QDBusPendingReply pendingReply = 
> statusNotifierWatcher->connection().call(call);
> +auto watcher = new QDBusPendingCallWatcher(pendingReply);
> +QObject::connect(watcher, &QDBusPendingCallWatcher::finished, q, 
> [this, watcher]() {

I prefer to assign a parent to the watcher. Maybe "q"?

> kstatusnotifieritem.cpp:884
> +auto watcher = new QDBusPendingCallWatcher(pendingReply);
> +QObject::connect(watcher, &QDBusPendingCallWatcher::finished, q, 
> [this, watcher]() {
> +QDBusPendingReply reply = *watcher;

watcher can be obtained via the argument of this signal, no need to capture it.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D8346

To: davidedmundson
Cc: xuetianweng, #frameworks

D8346: Remove blocking call in KStatusNotifierItem

2017-10-17 Thread David Edmundson 
davidedmundson planned changes to this revision.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D8346

To: davidedmundson
Cc: xuetianweng, #frameworks

D8346: Remove blocking call in KStatusNotifierItem

2017-10-17 Thread Xuetian Weng 
xuetianweng added inline comments.

INLINE COMMENTS

> kstatusnotifieritem.cpp:879
> +} else {
> +QDBusMessage call = 
> QDBusMessage::createMethodCall(statusNotifierWatcher->service(), 
> statusNotifierWatcher->path(), 
> QStringLiteral("org.freedesktop.DBus.Properties."), QStringLiteral("Get"));
> +call.setArguments({statusNotifierWatcher->interface(), 
> QStringLiteral("IsStatusNotifierHostRegistered")});

Do you need the extra dot at the end of "org.freedesktop.DBus.Properties." ?

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D8346

To: davidedmundson
Cc: xuetianweng, #frameworks

D8346: Remove blocking call in KStatusNotifierItem

2017-10-17 Thread David Edmundson 
davidedmundson created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
  BUG: 385867

TEST PLAN
  Ran test
  killed plasmashell
  SNI restored when plasmashell came back

REPOSITORY
  R289 KNotifications

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D8346

AFFECTED FILES
  src/kstatusnotifieritem.cpp

To: davidedmundson
Cc: #frameworks