I am open to discussion for whether this is desireable. The use-case is for moving NetworkPacket receive handling to a thread other than the one handling the rest of the device. In order to do this, we need to use a QtConnectionType::QueuedConnection. In order for that to work (with NetworkPackets), NetworkPacket has to be registered in the Metatype system.
Details
Details
- Reviewers
albertvaka - Group Reviewers
KDE Connect - Commits
- R224:0f5c9cd96cf0: Make NetworkPacket Metatype-capable
Nothing should be functionally different from before
Diff Detail
Diff Detail
- Repository
- R224 KDE Connect
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
The use-case is for moving NetworkPacket receive handling to a thread other than the one handling the rest of the device
Do you have a particular reason for this in mind?
Comment Actions
Basically I have just been trying all kinds of things to get the ConversationDbusInterface working, including moving packet handling to a different thread. This particular change is granular enough that I could split it out. I didn't want to throw it away without bringing it up for discussion since it is useful for a few cases, but I don't know if any of those cases are actually cases we care about, now or in the future!
| core/networkpacket.h | ||
|---|---|---|
| 52–53 | Is the default parameter needed? It like the fact that you get a compile error if no type is specified, otherwise one might think it is not necessary. | |
| core/networkpacket.h | ||
|---|---|---|
| 52–53 | In order to work with Q_DECLARE_METATYPE, the class needs a default constructor. I agree with you, though. This is one of the things I least like about this change, since in a lot of cases when using with the metatype system you will use the default constructor, and such a NetworkPacket makes no sense! | |
Comment Actions
The patch looks good to me, provided a reason. :)
| core/networkpacket.h | ||
|---|---|---|
| 52–53 | Why do you want this? To be able to access them from QML? DBus? | |
| core/networkpacket.h | ||
|---|---|---|
| 52–53 | The case I hit was being able to use NetworkPacket as the argument to a signal/slot connection across threads. I don't know if this is a useful usecase. So I guess this is more of a discussion of whether or not we ever think we want to make the daemon multi-threaded. I think the answer to that question is "no". If we think merging this patch won't cause us trouble, we can merge it on the offchance that we do find it useful (for that or something else). On the other hand, I can just abandon this revision and we can dig it up in 100 years when we actually need it 🙂 | |