Reduce duplicate code calculating popup position
ClosedPublic

Authored by gladhorn on Sun, Aug 11, 7:55 PM.

Details

Summary

The popup/exec if was duplicated three times, only to calculate the y position.

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.
gladhorn created this revision.Sun, Aug 11, 7:55 PM
Restricted Application added a project: KWin. · View Herald TranscriptSun, Aug 11, 7:55 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
gladhorn requested review of this revision.Sun, Aug 11, 7:55 PM

Did you test the menu position in particular in edge cases?

useractions.cpp
154

Remove local var and just call

Workspace::self()->clientArea(ScreenArea, QPoint(x, y),
                              VirtualDesktopManager::self()->current());
163

Thanks to your big duplicate reduction you can now remove the local var.

gladhorn updated this revision to Diff 63574.Sun, Aug 11, 8:18 PM

Remove locals.

Hi @romangg, I did not test this. I added inline comments explaining why the code is completely unchanged. It does 100% the same as before, just without the duplication. All you need is to invert the two conditions inside the ifs and switch the else branch with the first branch of the if and you'll arrive at this.

useractions.cpp
154–156

If (y == pos.top()) we use the x, y position unchanged (I'm unsure why this special case is necessary, it skips the caclulations below, so it's a micro-optimization).

155–158

This bit was executed after the first if was !=. Unchanged.

159–160

>= is the inversion of <, so this corresponds to the second, inner else. It is the only case where x, y are modified - y is calculated.

romangg accepted this revision.Mon, Aug 12, 9:54 AM

Your comments explain the new control flow just fine and why it is equivalent to the old one. You can add the const on push, no need to update the diff on Phab before.

useractions.cpp
155

const

This revision is now accepted and ready to land.Mon, Aug 12, 9:54 AM

Maybe add a comment why the y value needs to be adjusted in this one special case.

zzag added a subscriber: zzag.Mon, Aug 12, 9:38 PM
zzag added inline comments.
useractions.cpp
154–167

Remove this code as well.

zzag added inline comments.Mon, Aug 12, 9:40 PM
useractions.cpp
154–167

meh, please do it in another revision

zzag accepted this revision.Mon, Aug 12, 9:41 PM
This revision was automatically updated to reflect the committed changes.
gladhorn added inline comments.Tue, Aug 13, 6:41 AM
useractions.cpp
154–167

I'm a bit unsure what you mean - the entire popup position trickery? I was wondering how much it's needed, but didn't want to break existing things. For now I assume the code is there for a reason - it seems to me that it want to move the popup high enough to not be positioned at the bottom in a way that it would be cut off. I'm not sure what Qt actually does... in theory I think it should move it up.

zzag added inline comments.Tue, Aug 13, 8:16 AM
useractions.cpp
154–167

I don't see why we have to ensure that QMenu doesn't fall off screen. QMenu already does that for us. Besides that our code is incomplete(only one out of three cases is handled) and incorrect(y + popupHeight >= area.height() is wrong).

Remove if (y != pos.top()) and everything that goes inside.

zzag added inline comments.Tue, Aug 13, 8:17 AM
useractions.cpp
154–167

Why do we have this check? Well, that's a good question! I'd like to know why ;-)

zzag added inline comments.Tue, Aug 13, 8:18 AM
useractions.cpp
154–167

*out of four