On Wed, Aug 10, 2022 at 11:02:45AM +0200, Stein Gunnar Bakkeby wrote: > 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 > <[1]hil...@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 > > @@ -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 > > References > > 1. mailto:hil...@codemadness.org
Hi, OK thanks for the explanation. I have pushed the patch. If people find issues in their (multi-monitor) setups please let me know. -- Kind regards, Hiltjo