bakkeby / st-flexipatch

An st build with preprocessor directives to decide which patches to include during build time
MIT License
361 stars 108 forks source link

Fix border issues in the anysize patch #75

Closed veltza closed 2 years ago

veltza commented 2 years ago

I found two bugs in the anysize patch which can leave stripes on the borders when the patch is trying to clear them. So, for example, if you are using theme.sh to change the theme, these issues may occur and for that reason the anysize-nobar is designed to fix them. Unfortunately, that patch isn't attempting to fix the original problem but trying to circumvent it by doing some questionable things. But after this fix, the anysize-nobar patch shouldn't be needed anymore and could be removed unless it's doing something else I'm not aware of.

Here is some detailed info about the bugs:

The definition of xclear function

xclear(int x1, int y1, int x2, int y2)

The first bug

   if (x == 0) {
       xclear(0, (y == 0)? 0 : winy, win.vborderpx,
           winy + win.ch +
           ((winy + win.ch >= win.vborderpx + win.th)? win.h : 0));
   }

Because the third parameter of xclear is x2, win.vborderpx should be win.hborderpx (horizontal border) and the line should be:

   if (x == 0) {
       xclear(0, (y == 0)? 0 : winy, win.hborderpx,
           winy + win.ch +
           ((winy + win.ch >= win.vborderpx + win.th)? win.h : 0));
   }

The second bug

    if (y == 0)
        xclear(winx, 0, winx + width, win.hborderpx);

Because the fourth parameter of xclear is y2, win.hborderpx should be win.vborderpx (vertical border) and the line should be:

    if (y == 0)
        xclear(winx, 0, winx + width, win.vborderpx);

These fixes should be pushed to the upstream too, but unfortunately I haven't joined the suckless mailing list yet, so I'm unable to do so.

bakkeby commented 2 years ago

The anybar patch tries to center the columns and lines within the dimensions of the st window. This can give this swimming/floating look when the window is resized so I have never liked it. I prefer the simple version of anysize which just sets the size hints to 1. I think the clearing is fine in that respect as well.

The explanation for this fix I find a bit confusing. You explain it very well but logically a horizontal line (border) will be on the y-axis while a vertical line (border) will be on the x-axis. Here you say that since the fourth argument is y2 therefore we should pass in a position on the x-axis.

In that context the change does not make much sense I find, but I don't doubt that it had an effect in your case. It makes me wonder if the author of the anysize patch may have confused the two when it comes to positioning of the columns and rows. Or maybe something else is going on.

veltza commented 2 years ago
#if ANYSIZE_PATCH
win.hborderpx = (win.w - col * win.cw) / 2;
win.vborderpx = (win.h - row * win.ch) / 2;
#endif // ANYSIZE_PATCH

win.hborderpx = the width of the left and right borders. win.vborderpx = the height of the top and bottom borders.

So this buggy line doesn't fully clear the left border if the top and bottom borders are smaller than the left border (win.vborderpx < win.hborderpx):

   if (x == 0) {
       xclear(0, (y == 0)? 0 : winy, win.vborderpx,
           winy + win.ch +
           ((winy + win.ch >= win.vborderpx + win.th)? win.h : 0));
   }

And this buggy line doesn't fully clear the top border if the left and right borders are smaller than the top border (win.hborderpx < win.vborderpx):

    if (y == 0)
        xclear(winx, 0, winx + width, win.hborderpx);

I guess the terms "horizontal" and "vertical" can be confusing, and I don't know who made the mistakes here. Maybe the author of the anysize patch never noticed these mistakes, because if win.hborderpx and win.vborderpx are equal, the problems don't exist. Or if you don't change the background color, you won't notice the issues either.

veltza commented 2 years ago

Here are the steps to reproduce the bugs:

  1. Enable the anysize patch only
  2. Download theme.sh
  3. Change the theme to pop: theme.sh pop
  4. Resize the window until the top border is bigger than the left border
  5. Change the theme to gruvbox: theme.sh gruvbox
  6. You should see the black stripe on the top border (bug number 2)
  7. Resize the window until the left border is bigger than the top border (this can be trickier)
  8. Change the theme back to pop: theme.sh pop
  9. You should see the white stripe on the left border (bug number 1)

Now apply my fix and try to reproduce the bugs.

bakkeby commented 2 years ago

I'll have a play around when I have time, probably not before tomorrow though.

I had a look at the original patch and it is done consistently like this across the board. So we are taking about the vertical size of the horizontal border (vborderpx) and the horizontal size of the vertical border (hborderpx).

Things like this are typically obvious when programming but can cause confusion later. It would probably be more straightforward if the variables were called xborderpx and yborderpx.

I think your fix should be fine.

You also don't need to be subscribed to the mailing list to push patches to the suckless site git repository.

bakkeby commented 2 years ago

Another thing to note regarding replicating this issue is the issue is not reproducible if defaultbg is 0. Managed to reproduce the issue and the fix and reasoning behind it is sound. Will merge.

bakkeby commented 2 years ago

@veltza are you planning on uploading an updated version of the anysize patch to the suckless site?

veltza commented 2 years ago

@bakkeby I'm really busy right now because I'm going on vacation. So, feel free to upload the updated version to the suckless site.