[System Tray] SNI fallback to context menu on failing Activate
ClosedPublic

Authored by romangg on Jan 26 2017, 11:22 PM.

Details

Summary

This patch primarily is aimed at applications using libappindicator. For example:

  • Steam
  • Discord
  • Deluge

libappindicator doesn't provide functionality for raising an application, but only a context menu. Since it also doesn't even provide the corresponding DBus method, we use the resulting error to try to display the context menu instead, which matches the behaviour on Unity and Gnome.

https://www.gamingonlinux.com/forum/topic/2519
https://bugs.kde.org/show_bug.cgi?id=375351

Test Plan

Manually with libappindicator and Qt based tray icons.

Diff Detail

Repository
R120 Plasma Workspace
Lint
Lint Skipped
Unit
Unit Tests Skipped
romangg updated this revision to Diff 10605.Jan 26 2017, 11:22 PM
romangg retitled this revision from to [System Tray] SNI fallback to context menu on failing Activate.
romangg updated this object.
romangg edited the test plan for this revision. (Show Details)
romangg added reviewers: Plasma, davidedmundson.
romangg set the repository for this revision to R120 Plasma Workspace.
romangg added a project: Plasma.
Restricted Application added a subscriber: plasma-devel. · View Herald TranscriptJan 26 2017, 11:22 PM
broulik added inline comments.
dataengines/statusnotifieritem/statusnotifieritemsource.cpp
472–478

This spins its own event loop which is dangerous when dealing with QML

473

I think that method should return true for success, not failure.

480

Can this lead to false positives? Maybe we need a tri-state here, ie. activate succeeded, activate failed (fall back to menu), nothing to activate (do nothing).

davidedmundson added inline comments.Jan 27 2017, 8:29 AM
dataengines/statusnotifieritem/statusnotifieritemsource.cpp
472–478

we could just move the the calling ContextMenu on failure logic into the C++

It saves this blocking, fixes the bug for all SNI datasource users, and allows us to act differently depending on what the DBus error actually is

romangg marked an inline comment as done.Jan 27 2017, 9:14 AM
romangg added inline comments.
dataengines/statusnotifieritem/statusnotifieritemsource.cpp
472–478

This spins its own event loop which is dangerous when dealing with QML

Since the event loop is part of the service, isn't the QML shielded from it anyway? The QML code connects only to the job finished signal in the end, which is emitted again by the job's/service's main thread.

480

Imo if "nothing activates", there should be still always the attempt to open the context menu. The user expects that atleast "something" happens on left click.

But in reality if we're at this point, this won't work either, since there is a general problem with m_statusNotifierItemInterface, which means the context menu won't work either.

davidedmundson added inline comments.Jan 27 2017, 9:32 AM
dataengines/statusnotifieritem/statusnotifieritemsource.cpp
472–478

Since the event loop is part of the service, isn't the QML shielded from it anyway? The QML code connects only to the job finished signal in the end, which is emitted again by the job's/service's main thread.

If only :)

We spawn a new event loop here, from inside this event loop we continue processing animations, mouse, DBus whatever.
During this, we might end up deleting the SNI and implicitly this datasource.

We then finish this method, potentially 30 seconds after we started, and come out of this line.

"this" is now a danging pointer and the m_statusNotifierItemInterface->interface on the next line seg faults.

This is a nice simple explanation of a simpler version of the same problem:
https://blogs.kde.org/2009/03/26/how-crash-almost-every-qtkde-application-and-how-fix-it-0

only in QML it's even worse! Even if you did guard against it, QMLEngine then returns with the nicely wrapped JS value to a javascript context that doesn't exist - and there's no way to guard that.

romangg updated this revision to Diff 10649.Jan 28 2017, 3:13 AM

One more try with guarding against dangling pointers. Just trying out stuff here. So in case this is utter bullshit please tell me if and why it doesn't work.

My thinking was the following:

  • Guard code execution in StatusNotifierItemSource::activate against deleted m_statusNotifierItemInterface by comparing it to the old pointer value
  • In case not deleted emit a callback signal, which is received in StatusNotifierItemJob::activateCallback, if job object hasn't been deleted before
  • set the result, which is received by StatusNotifierItem.qml as long as it exists. I mean since it connects to the finished signal of the job, there shouldn't be a problem, if it got deleted earlier.
  • Guard code execution in StatusNotifierItemSource::activate against deleted m_statusNotifierItemInterface by comparing it to the old pointer value

You can't do that in this way, do this

QPointer<org::kde::StatusNotifierItem> snii = m_statusNotifierItemInterface;
if (snii && snii->isValid()) {
    emit activateCallback(!snii->lastError().isValid());
}
In D4301#80882, @subdiff wrote:

One more try with guarding against dangling pointers. Just trying out stuff here. So in case this is utter bullshit please tell me if and why it doesn't work.

My thinking was the following:

  • Guard code execution in StatusNotifierItemSource::activate against deleted m_statusNotifierItemInterface by comparing it to the old pointer value
  • In case not deleted emit a callback signal, which is received in StatusNotifierItemJob::activateCallback, if job object hasn't been deleted before
  • set the result, which is received by StatusNotifierItem.qml as long as it exists. I mean since it connects to the finished signal of the job, there shouldn't be a problem, if it got deleted earlier.

What Anthony said is right...But also there's no need to make things complex.

The operations are kjobs.
Just use signal/slots as normal.

And/or don't return anything and just call context menu if activate fails. No need to return anything to qml.

romangg updated this revision to Diff 10713.Jan 30 2017, 12:24 PM
romangg edited edge metadata.

Use QDBusPendingCallWatcher.

davidedmundson accepted this revision.Jan 31 2017, 1:41 PM
davidedmundson edited edge metadata.
This revision is now accepted and ready to land.Jan 31 2017, 1:41 PM
This revision was automatically updated to reflect the committed changes.