This fixes a bug I had where Session Management restored a window on an
activity that didn't exist that, meaning I was unable to access it.
setOnActivity() already has this check
Details
Details
- Reviewers
graesslin - Group Reviewers
Plasma - Commits
- R108:0dd927c4a47d: Validate activities in setOnActivities
R108:6c8f0e6cc0ba: Include setOnActivity in the test
Using my broken session, restored and got my ghost process back
on all activities
Added a window to activity 2, checked it came back there and
only there.
Diff Detail
Diff Detail
- Repository
- R108 KWin
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
I'd love to see an autotest for this. If you have any questions on how to get this autotested I'm happy to help, but I guess you find the way there yourself - take any of the tests in autotest/wayland which interact with X11 clients as an example.
| client.cpp | ||
|---|---|---|
| 1244–1245 | coding style | |
| 1244–1247 | this looks dangerous: your removing from the container you are iterating over. I have seen this crash to often. I'd suggest to use the iterator API and use erase on the container. | |
| client.cpp | ||
|---|---|---|
| 1244–1247 | no it isn't. foreach() does a shallow copy and then any modifications (which there normally won't be) would do a full copy. | |
| client.cpp | ||
|---|---|---|
| 1244–1247 | It's just about the most inefficient approach ;-) | |
| client.cpp | ||
|---|---|---|
| 1244–1247 | Again no. No worse than ever passing a QString somewhere. | |
Comment Actions
in the noop case.
otherwise there's an unnecessary detach and on top of this there's a
useless secondary lookup for the removeOne() call.
it's not getting much worse unless you add some bitcoin mining in every
loop :-P
Comment Actions
Swing and a miss.
There's no extra detatch because we have a reference to the original full QStringList in whatever stack called setOnActivities.
So the copy taken by the foreach will always be the same list as the one on the stack in whoever called us.
In both cases, one detach
(the extra lookup argument is valid)
Comment Actions
sorry, missed context. i thought "newlist" to be a local var to build the
return value.
Comment Actions
Apart from the comment above, seems good to go.
| client.cpp | ||
|---|---|---|
| 1244–1247 | It is efficient in the common case, not efficient in the case where it actually deletes an activity from the list. Now, this should be rare enough for this not to matter. Personal reason for not liking this is that this is a special 'feature' of Q_FOREACH - when someone decides in the future to replace it with a range-for, it will lead to problems. At least, add a comment above regarding deletion in Q_FOREACH. | |
Comment Actions
@davidedmundson I just pushed an autotest for the condition. With your patch you we should be able to remove the three QEXPECT_FAIL and call it a day.
Comment Actions
- Fix activites_test check
- Include setOnActivity in the test
- Port foreach into iterating