Eike removed this, but I'm not sure why. All other roles trigger
datachanged, so I don't see why pid shouldn't.
This patch fixes the tests, which I should not have broken in the first
place.
Details
Details
- Reviewers
hein davidedmundson - Group Reviewers
Plasma
Diff Detail
Diff Detail
- Repository
- R127 KWayland
- Branch
- master
- Lint
No Linters Available - Unit
No Unit Test Coverage
Comment Actions
Possibly, in this case, I disagree. The pid is initially unknown set to in the client, and can change afterwards. In reality, the pid of the window doesn't change, but it can still be set after being initially 0. In fact, it has to be.
In the end, I don't care much, but having it connected to dataChanged makes this whole thing way more consistent, and it makes sense semantically. IMO, not putting it behind dataChanged is semantic wanking making the interface harder to understand, not easier, and not better.
Comment Actions
it can still be set after being initially 0. In fact, it has to be.
Not at the time the client sees it.
see initialStateCallback which is important.
void PlasmaWindow::Private::initialStateCallback(void *data, org_kde_plasma_window *window)
{
Q_UNUSED(window)
Private *p = cast(data);
if (!p->unmapped) {
emit p->wm->windowCreated(p->q);
}
}We don't emit that we have a new window until we get this event. This can be long after construction.
As long as the server sets the pid before this there's no need for clients to have an unknown state, that is important and worth testing.
Whether there's a changed signal or not isn't something I particularly care about, but it isn't needed.
Ping me on IRC if I haven't explained that well.
Comment Actions
like David explained: we don't need the change signal and IMHO we shouldn't expose "wrong API". I consider having a changed signal for a PID wrong API as it indicates that the PID could change. But it won't ever change, so we shouldn't expose it. I know that all other attributes have a changed signal, but they can change.
Comment Actions
I'm fine with no change signal, but heads-up that libtaskmanager has code connecting to it, so if you remove it from kwayland please also adapt plasma-workspace or the build breaks.
Comment Actions
I highly suggest to remove that. That signal doesn't work as there is no change going to happen. It's just pointless.
Anyway I want a solution on this broken test ASAP. I don't have the time to code a fix for it, but I want KWayland to be green prior to me leaving to vacations. Just adding the changed signal to fix the test is not acceptable as that would mean we have an incorrect API added which we cannot remove till KF 6.
If it has to be I'm going to revert the changes - even if it makes other software break compilation.