D23102: Reduce duplicate code calculating popup position

gladhorn added a comment.


  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.

INLINE COMMENTS

> useractions.cpp:154
>      m_client->blockActivityUpdates(true);
> -
> -    if (y == pos.top()) {
> -        if (needsPopup) {
> -            m_menu->popup(QPoint(x, y));
> -        } else {
> -            m_menu->exec(QPoint(x, y));
> -        }
> -    } else {
> -        QRect area = ws->clientArea(ScreenArea, QPoint(x, y), VirtualDesktopManager::self()->current());
> +    if (y != pos.top()) {
> +        QRect area = Workspace::self()->clientArea(ScreenArea, QPoint(x, y),

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).

> useractions.cpp:155-158
> +        QRect area = Workspace::self()->clientArea(ScreenArea, QPoint(x, y),
> +                                                   VirtualDesktopManager::self()->current());
>          menuAboutToShow(); // needed for sizeHint() to be correct :-/
>          int popupHeight = m_menu->sizeHint().height();

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

> useractions.cpp:159
>          int popupHeight = m_menu->sizeHint().height();
> -        if (y + popupHeight < area.height()) {
> -            if (needsPopup) {
> -                m_menu->popup(QPoint(x, y));
> -            } else {
> -                m_menu->exec(QPoint(x, y));
> -            }
> -        } else {
> -            if (needsPopup) {
> -                m_menu->popup(QPoint(x, pos.top() - popupHeight));
> -            } else {
> -                m_menu->exec(QPoint(x, pos.top() - popupHeight));
> -            }
> +        if (y + popupHeight >= area.height()) {
> +            y = pos.top() - popupHeight;

>= 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.

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D23102

To: gladhorn, #kwin
Cc: romangg, kwin, LeGast00n, sbergeron, jraleigh, fbampaloukas, GB_2, mkulinski, ragreen, jackyalcine, Pitel, iodelay, crozbo, bwowk, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart