It was causing many problems to have a method which would return some values if it happend to have them laying around. Now we wait until the messages we need are available before sending a reply.
Details
Details
- Reviewers
- None
- Group Reviewers
KDE Connect
Nothing should visibly change from before:
- Run messaging app
- Verify that conversations are able to be displayed
Diff Detail
Diff Detail
- Repository
- R224 KDE Connect
- Branch
- synchonous-message-fetch
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 3703 Build 3721: arc lint + arc unit
Comment Actions
See the inline comments. Would you like these written in to the source code?
The idea is per https://wiki.qt.io/Threads_Events_QObjects#Forcing_event_dispatching . It works because the event queue handles signals serially, and the Device object (which receives the packet from the network), the SmsPlugin object (which handles the packet and the app's message request) and the ConversationDbusInterface (which handles the dbus request) are all resident on the same thread, so all events are handled by the same thread. Reentering the thread's event queue allows the data coming from Android to be handled. Exiting the event queue after the message is handled means the data we need is ready.
I tried to achieve the same thing using locks and multiple threads, but I could not make it work. The core issue is that I could not get the ConversationDbusInterface and the Device to be on separate threads AND have the dbus interface properly registered to the dbus. This means, regardless what happens, the dbus interface waiting would block the packet handling. QWaitCondition::wait() does not allow other events on the same thread to be handled, apparently 😢
For proof, put a breakpoint (or debug print) on smsplugin.ccp lines 56, 91, and 103, then launch the SMS app and open a conversation. You should see that the daemon hits line 91 when the request comes in, then 56 when the phone replies with the messages, then 103 as the daemon returns the messages to the phone (via the ConversationDbusInterface, of course)
For a less difficult test, open the messaging app before this patch and open a conversation. Notice that only one message shows *the first time* because that is all the conversation interface knows about. After the first time a conversation is opened, it will show 10 messages when a conversation is opened because they are in cache. With this patch, you will see 10 every time because the dbus interface waits for its cache to be populated before returning.
| plugins/sms/smsplugin.cpp | ||
|---|---|---|
| 55 | Unblock - The awaited data is ready (hopefully...) | |
| 92 | Connect the signal that we have processed some new data to the new event loop's quit signal | |
| 93 | Reenter the thread's event loop. loop.exec() does not return until signaled to quit | |
Comment Actions
Yes, I think having ConversationDbusInterface on its own thread would be the best answer to this problem. However, I couldn't figure out a way to do this which also had the service properly registered on dbus. I tried everything I could think of, from subclassing QThread to using QThread properly and connecting to its started signal. I was able to get the SMS plugin on a new thread (using QObject::movetothread) and responding to requests, but whenever the ConversationDbusInterface was on another thread it was not visible on dbus. I think you have more QDbus experience than I do so you might be able to get it working :)