Re: [hackers] [dwm][PATCH] Simplify client y-offset correction

From: Stein Gunnar Bakkeby <bakkeby_AT_gmail.com>
Date: Wed, 10 Aug 2022 11:02:45 +0200

Hi Hijito,

I wasn't sure how big of a wall of text to put in the commit comment. Let
me clarify this first.

We have a block of code adjusting the client's starting position for
floating windows where:
    - if the window position and size exceeds the right hand side of the
monitor, then we move it so that it touches the monitor's right edge
    - if the window position and size exceeds the bottom of the monitor,
then we move it so that it touches the monitor's bottom edge
    - if the window position exceeds the left hand side of the monitor,
then we move it so that it touches the monitor's left edge
    - then we have the line in question

    /* only fix client y-offset, if the client center might cover the bar */
    c->y = MAX(c->y, ((c->mon->by == c->mon->my) && (c->x + (c->w / 2) >=
c->mon->wx)
        && (c->x + (c->w / 2) < c->mon->wx + c->mon->ww)) ? bh :
c->mon->my);

This line sucks because one can't read it and go "oh, I see, that makes
sense". The git history doesn't reveal why the line was added or the
reasoning behind it.

If we break it down we have:

if
    (c->mon->by == c->mon->my)
    (the bar is at the top of the monitor and it is not hidden)
and
    (c->x + (c->w / 2) >= c->mon->wx) &&
    (c->x + (c->w / 2) < c->mon->wx + c->mon->ww)
    (the window center on the x-axis is within the window area of the
monitor)
then
    use bh as the position
else
    use my as the position

Finally we end up with either:
c->y = MAX(c->y, bh);
or
c->y = MAX(c->y, c->mon->my);

The line comment I presume was added later in an attempt to explain what
the line of code does to justify the complexity, but it doesn't actually
represent what is happening. The client's position on the x-axis would not
have anything to do with covering the bar - this is just misleading.

The check to verify that the client center is within the window area is
redundant given the two prior adjustments on the x-axis. The only exception
to this would be if the window is so large that it spans three monitors, in
which case the center of the window may be outside the window area of the
client's monitor, and it is unlikely that this is the edge case that the
line of code was trying to capture.

In the event that the bar is at the top then we end up using bh as the
constraint on the y-axis. This is wrong as the variable represents the
height of the bar, it is not a position and it assumes that the monitor's
my is 0.

As-is this should have been c->mon->by + bh (or c->mon->my + bh). In
practice this rarely causes any issues which is why this line of code has
survived for this long.

If need be this can be replicated in dual monitor setups where one monitor
is placed further down than the other (e.g. one vertical monitor and one
horizontal). Add a rule to make st windows floating and start st - the
window will be placed partially or fully out of view when started on the
lower monitor which will have a larger my value.

Taking everything into account we can deduce that the intended behaviour is
simply that if the window's y position is above the top edge of the
monitor, then we move the window into view so that it touches the monitor's
top edge - or below the bar if the bar is at the top (and is shown).

To keep the same behaviour as present the statement can be simplified to:
c->y = MAX(c->y, c->mon->wy);

where the window area's wy position covers all the cases (bar placement and
whether the bar is hidden).


If there is agreement regarding that then I would also recommend that we
use the window area variables (wx, ww, wy, wh) rather than the monitor area
(mx, mw, my, mh) for the other three constraints for consistency:

    if (c->x + WIDTH(c) > c->mon->wx + c->mon->ww)
        c->x = c->mon->wx + c->mon->ww - WIDTH(c);
    if (c->y + HEIGHT(c) > c->mon->wy + c->mon->wh)
        c->y = c->mon->wy + c->mon->wh - HEIGHT(c);
    c->x = MAX(c->x, c->mon->wx);

My reasoning for this is that I think the whole block should signal the
intent that we want to have the window placed within the window
area, rather than within the monitor.

As-is if using a bottom bar then a window exceeding the bottom edge of the
monitor is allowed to cover the bar.

E.g. dwm with bottom bar and a rule to make st windows floating, try to run:
   st -g 50x20+0+9000

Using window area variables would address that.


Thanks,

-Stein


On Tue, Aug 9, 2022 at 11:38 PM Hiltjo Posthuma <hiltjo_AT_codemadness.org>
wrote:

> On Tue, Aug 09, 2022 at 10:38:08AM +0200, Stein wrote:
> > The reasoning behind the original line may be lost to time as
> > it does not make much sense checking the position on the x-axis
> > to determine how to position the client on the y-axis.
> >
> > In the context of multi-monitor setups the monitor y position
> > (m->my) may be greater than 0 (say 500), in which case the window
> > could be placed out of view if:
> > - the window attributes have a 0 value for the y position and
> > - we end up using the y position of bh (e.g. 22)
> >
> > If the aim is to avoid a new floating client covering the bar then
> > restricting y position to be at least that of the window area
> > (m->wy) should cover the two cases of using a top bar and using a
> > bottom bar.
> > ---
> > dwm.c | 4 +---
> > 1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/dwm.c b/dwm.c
> > index 967c9e8..87d0ada 100644
> > --- a/dwm.c
> > +++ b/dwm.c
> > _AT__AT_ -1049,9 +1049,7 @@ manage(Window w, XWindowAttributes *wa)
> > if (c->y + HEIGHT(c) > c->mon->my + c->mon->mh)
> > c->y = c->mon->my + c->mon->mh - HEIGHT(c);
> > c->x = MAX(c->x, c->mon->mx);
> > - /* only fix client y-offset, if the client center might cover the
> bar */
> > - c->y = MAX(c->y, ((c->mon->by == c->mon->my) && (c->x + (c->w / 2)
> >= c->mon->wx)
> > - && (c->x + (c->w / 2) < c->mon->wx + c->mon->ww)) ? bh :
> c->mon->my);
> > + c->y = MAX(c->y, c->mon->wy);
> > c->bw = borderpx;
> >
> > wc.border_width = c->bw;
> > --
> > 2.37.1
> >
> >
>
> Hi,
>
> Sorry but this commit is much too vague. Doesn't this change the behaviour
> as
> described in the original comment ("client center")?
>
> Please provide a more clear description of the setup you have and the
> steps to
> reproduce them as it would help to understand the problem and the intended
> behaviour better.
>
> --
> Kind regards,
> Hiltjo
>
>
Received on Wed Aug 10 2022 - 11:02:45 CEST