Changeset View
Standalone View
dataengines/statusnotifieritem/statusnotifieritemsource.cpp
| Show First 20 Lines • Show All 463 Lines • ▼ Show 20 Line(s) | 422 | { | |||
|---|---|---|---|---|---|
| 464 | // additionally added pixmaps". | 464 | // additionally added pixmaps". | ||
| 465 | *icon = tmp; | 465 | *icon = tmp; | ||
| 466 | //hopefully huge and enormous not necessary right now, since it's quite costly | 466 | //hopefully huge and enormous not necessary right now, since it's quite costly | ||
| 467 | } | 467 | } | ||
| 468 | 468 | | |||
| 469 | void StatusNotifierItemSource::activate(int x, int y) | 469 | void StatusNotifierItemSource::activate(int x, int y) | ||
| 470 | { | 470 | { | ||
| 471 | if (m_statusNotifierItemInterface && m_statusNotifierItemInterface->isValid()) { | 471 | if (m_statusNotifierItemInterface && m_statusNotifierItemInterface->isValid()) { | ||
| 472 | m_statusNotifierItemInterface->call(QDBus::NoBlock, QStringLiteral("Activate"), x, y); | 472 | org::kde::StatusNotifierItem *snii = m_statusNotifierItemInterface; | ||
| 473 | m_statusNotifierItemInterface->call(QDBus::BlockWithGui, QStringLiteral("Activate"), x, y); | ||||
broulik: I think that method should return true for success, not failure. | |||||
| 474 | | ||||
| 475 | if (m_statusNotifierItemInterface == snii && snii->isValid()) { | ||||
| 476 | emit activateCallback(!m_statusNotifierItemInterface->lastError().isValid()); | ||||
| 477 | } | ||||
broulik: 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. romangg: > This spins its own event loop which is dangerous when dealing with QML
Since the event loop… | |||||
If only :) We spawn a new event loop here, from inside this event loop we continue processing animations, mouse, DBus whatever. 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: 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. davidedmundson: >Since the event loop is part of the service, isn't the QML shielded from it anyway? The QML… | |||||
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 davidedmundson: we could just move the the calling ContextMenu on failure logic into the C++
It saves this… | |||||
| 473 | } | 478 | } | ||
| 474 | } | 479 | } | ||
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). broulik: Can this lead to false positives? Maybe we need a tri-state here, ie. activate succeeded… | |||||
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. romangg: Imo if "nothing activates", there should be still always the attempt to open the context menu. | |||||
| 475 | 480 | | |||
| 476 | void StatusNotifierItemSource::secondaryActivate(int x, int y) | 481 | void StatusNotifierItemSource::secondaryActivate(int x, int y) | ||
| 477 | { | 482 | { | ||
| 478 | if (m_statusNotifierItemInterface && m_statusNotifierItemInterface->isValid()) { | 483 | if (m_statusNotifierItemInterface && m_statusNotifierItemInterface->isValid()) { | ||
| 479 | m_statusNotifierItemInterface->call(QDBus::NoBlock, QStringLiteral("SecondaryActivate"), x, y); | 484 | m_statusNotifierItemInterface->call(QDBus::NoBlock, QStringLiteral("SecondaryActivate"), x, y); | ||
| 480 | } | 485 | } | ||
| 481 | } | 486 | } | ||
| 482 | 487 | | |||
| Show All 20 Lines | |||||
I think that method should return true for success, not failure.