bakkeby / st-flexipatch

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

LIGATURES_PATCH breaks BOXDRAW_PATCH when applied together #29

Closed AlexKurisu closed 3 years ago

AlexKurisu commented 3 years ago

LIGATURES_PATCH for some reason completely breaks rendering of BOXDRAW_PATCH. LukeSmithxyz/st works fine. Here's screenshot: 2021-05-24-113419_50x679_scrot

avih commented 3 years ago

That's probably because ligature originally has a very slightly different patch if it's used together with boxdraw: Basic: https://st.suckless.org/patches/ligatures/0.8.3/st-ligatures-20200430-0.8.3.diff With boxdraw: https://st.suckless.org/patches/ligatures/0.8.3/st-ligatures-boxdraw-20200430-0.8.3.diff

The only difference is that without boxdraw hbtransform at hb.c has this code towards the end:

    /* Apply the transformation to glyph specs. */
    for (int i = 0, specidx = 0; i < len; i++) {
        if (glyphs[i].mode & ATTR_WDUMMY)
            continue;

        if (codepoints[i] != specs[specidx].glyph)
            ((Glyph *)glyphs)[i].mode |= ATTR_LIGA;

        specs[specidx++].glyph = codepoints[i];
    }

While with boxdraw it has one more check:

    /* Apply the transformation to glyph specs. */
    for (int i = 0, specidx = 0; i < len; i++) {
        if (glyphs[i].mode & ATTR_WDUMMY)
            continue;
        if (glyphs[i].mode & ATTR_BOXDRAW) {
            specidx++;
            continue;
        }

        if (codepoints[i] != specs[specidx].glyph)
            ((Glyph *)glyphs)[i].mode |= ATTR_LIGA;

        specs[specidx++].glyph = codepoints[i];
    }

So the fix is probably to change it to be like so:

    /* Apply the transformation to glyph specs. */
    for (int i = 0, specidx = 0; i < len; i++) {
        if (glyphs[i].mode & ATTR_WDUMMY)
            continue;
#if BOXDRAW_PATCH 
        if (glyphs[i].mode & ATTR_BOXDRAW) {
            specidx++;
            continue;
        }
#endif

        if (codepoints[i] != specs[specidx].glyph)
            ((Glyph *)glyphs)[i].mode |= ATTR_LIGA;

        specs[specidx++].glyph = codepoints[i];
    }
bakkeby commented 3 years ago

Thanks @avih.

@AlexKurisu can you try with the above change? Also for reference how do you reproduce this issue?

AlexKurisu commented 3 years ago

Yep, above change fixed the issue. To reproduce it you could:

  1. enable BOXDRAW_PATCH and LIGATURES_PATCH
  2. set const int boxdraw = 1 in config.h
  3. launch any program that uses box drawing characters ({n,}vim, mc, elinks, etc…)
bakkeby commented 3 years ago

Thanks @AlexKurisu, yes mc looks interesting without the change. mc

Thanks for reporting, much appreciated.

avih commented 3 years ago

@bakkeby the ligature page has also different patches when combined with scrollback and/or alpha - https://st.suckless.org/patches/ligatures/

I didn't check how they differ, but you probably should.

bakkeby commented 3 years ago

Just checked, the alpha variant doesn't have any actual code changes (just different line number references), and the scrollback patch just adds an if statement. These should be fine.

avih commented 3 years ago

Nice.