- Remove double lookup
- Use reference rather than copies
- Remove scene repaints which cause visual glitch (open window moves it to top right corner to became maximize, grab it for title and move it to restore it size)
- Decrease max time, effect looks better (say-so)
Details
Details
Diff Detail
Diff Detail
- Repository
- R108 KWin
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Comment Actions
This revision has a lot of unrelated changes. Please revert them.
Let's keep this revision focused on fixing QHash abuse and adding const refs.
Using new connect syntax, fixing coding style(KWin follows Frameworks/Kdelibs coding style), etc can be addressed in different revisions.
| effects/wobblywindows/wobblywindows.cpp | ||
|---|---|---|
| 299 | windows.erase(it); | |
Comment Actions
No, I didn't. Currently, I don't have much time to test the patch.
| effects/wobblywindows/wobblywindows.cpp | ||
|---|---|---|
| 60–136 | Please revert this change. | |
| 269 | We don't need this check. Please delete it. (if windows.isEmpty() is equal to true, then prePaintScreen, prePaintWindow, etc won't be called) | |
| 283 | Please revert this change. | |
| 309 | Please use const_iterator. | |
| 358–373 | Please revert this change. | |
| 490 | FWIW, we do not connect to windowShown, so this can never happen, at least right now. | |
| effects/wobblywindows/wobblywindows.h | ||
| 139–143 | Please revert this change. | |
| effects/wobblywindows/wobblywindows.cpp | ||
|---|---|---|
| 358–373 | Because that's unrelated change. | |
Comment Actions
Mainly optimization + 1 change, maxTime is changed from 10 to 5 ms, effect looks more *smootly*, I hope someone of you to test it as well, I run it about a week on x11.
Comment Actions
Optimization can mean many things. That's why I asked (so is it now a only code optimization without behavioral change?) The "+1" should then go in a separate patch.
Comment Actions
Please add the context. Either by using arc or by supplying git diff the -U99999parameter
| effects/wobblywindows/wobblywindows.cpp | ||
|---|---|---|
| 60 | I don't think it particularly helps the code readability to merge all that. What would help more is adding comments what this stuff does. | |
| effects/wobblywindows/wobblywindows.h | ||
| 201 | Functions should be listed before member variables with same scope. The old code did this wrong already though. | |
| effects/wobblywindows/wobblywindows.cpp | ||
|---|---|---|
| 60 | So we have copies of all 4 arrays, that's optimization. Even we have 4 unused arrays, then one of copies and it's used | |
Comment Actions
You also missed const_iterator. ;-)
| effects/wobblywindows/wobblywindows.cpp | ||
|---|---|---|
| 358–373 | It's not optimization, it should go in another patch. | |
| effects/wobblywindows/wobblywindows.cpp | ||
|---|---|---|
| 358–373 | This is outlast pedantic, since it's duplicate code. | |
| effects/wobblywindows/wobblywindows.cpp | ||
|---|---|---|
| 358–373 | Then name it like this: removal of duplicated code. If your patch is not only about removing duplicated code, mention the other "optimizations"/changes you did as well in the commit message. Or this will give you an indication that it might be better to directly split the patch up into multiple diffs, each improving one distinct aspect. The problem is that "optimization" can mean anything and everybody here seems to understand something differently by it. | |
Comment Actions
@anthonyfieroni Please delete unrelated changes (they can go in other patches). Let's focus on double hashing in this patch (also maybe update title and summary accordingly). If it's possible, don't alter logic, just fix double hashing.
Adding const refs in this patch looks okay to me.
| effects/wobblywindows/wobblywindows.cpp | ||
|---|---|---|
| 60–136 | This should go in another patch. | |
| 306 | Please use constFind/constEnd. | |
| 712–714 | Can you please clarify why you deleted this one? Shouldn't we pass closeRect as rect in paintWindow, e.g.: const QRectF rect = wwi.status == Closing ? wwi.closeRect : w->geometry(); ? | |
| effects/wobblywindows/wobblywindows.h | ||
| 201–211 | Please leave them here. (Move them up in a patch that does code cleanup) | |
| effects/wobblywindows/wobblywindows.cpp | ||
|---|---|---|
| 60–136 | I don't see why, it's pretty safe and does not need go in another one, after all we waste a time since doing some work. | |
| 712–714 | It does not make sense to me, why we want to use closeRect? | |
| effects/wobblywindows/wobblywindows.h | ||
| 201–211 | See above comment from @romangg | |
Comment Actions
Remove scene repaints which cause visual glitch (open window moves it to top right corner to became maximize, grab it for title and move it to restore it size)
The Maximize effect causes that, not scene repaints. Please add the repaints back until we find proper solution.
I don't see why, it's pretty safe and does not need go in another one,
Because that's unrelated change. If it should go in, then what does your patch try to address?
after all we waste a time since doing some work.
This patch has unrelated changes and you're objecting to remove them. Instead of focusing on one particular change, it shuffles code around, it alters behavior(e.g. removal of repaints), and it does code cleanup.
| effects/wobblywindows/wobblywindows.cpp | ||
|---|---|---|
| 503 | That's not possible to have ref'ed window at this point. | |
Comment Actions
So for it's pretty related, optimization is pretty known i.e. faster execution and reduced memory usage. Whole patch makes that.
After all i'm not interested to contribute to KWin anymore.
Comment Actions
Before i open RR i made following tests:
'+' Maximize effect
'-' Wobbly (no patch)
No visual glitches (they happen exactly after windows is opened and moved to maximize)
'+' Maximize effext
'+' Wobbly (no patch)
Visual glitches even if you add more repaints (i.e. after every container remove) they became bigger height, i.e. 2 repaints 2x height glitches, 3 repaints => 3x height
So i bet to problem is in Wobbly not in Maximize, remove all repaints, now after one week i don't see any downgrades even effect is way better.
'+' Maximize
+ Wobbly (patched)
No glitches at all, no performance penalty for now.