LukeSmithxyz / st

Luke's fork of the suckless simple terminal (st) with vim bindings and Xresource compatibility.
MIT License
1.64k stars 1.01k forks source link

patched in st-glyph-wide-support #349

Open iStagnant opened 1 year ago

iStagnant commented 1 year ago

Fixes Glyph truncation. Patch from st site: https://st.suckless.org/patches/glyph_wide_support/

Iskustvo commented 1 year ago

Hi @iStagnant, does the patch actually work for you?

I have just tried it and it certainly improves the situation, but doesn't really solve the issue.

I'll demonstrate the behavior of wide glyphs from Neovim, with 3 terminals:


Moving forward:

move_left_to_right From this gif, we can observe:


Move backwards:

move_right_to_left From this gif, we can observe:


Losing focus while positioned on the character after wide glyph.

lose_focus_on_space_after_wide_glyph From this gif, we can observe: After wide glyph character was chopped and we lose the focus to change the cursor shape:


FYI, @Dreomite as the original author of the patch.

iStagnant commented 1 year ago

Hello @Iskustvo i couldn't replicate the behaviour you are getting, when i tried it on the icons in my dwm config.h (you can try it it's in dwmrice/.config/dwm) it showed normal behaviour sure the " at the end kind of entered in the icons but that wasn't the issue this was supposed to solve. And sorry for not including visual examples i was in a hurry to test this and i can't access my laptop for the next 4 days.

Iskustvo commented 1 year ago

No worries, thank you for answering quick enough! Unfortunately, the behavior is exactly the same for all 3 "versions" of terminals (like above) with your tag icons :disappointed:

Btw, are you using this repository or clean st? I just did a quick check with this repository (clean master, without this PR) and it seems that everything is already working here, the only thing missing seems to be that when the character is highlighted, the remainder stays without reverted color (but isn't chopped!). Funny enough, that's the one thing that original patch fixes when applied to clean st directory, as observed in first gif :rofl:

I also tried this branch and I don't see any difference compared to the clean master, am I missing something?

Last but not least, I would really appreciate if anyone can double-check and confirm what I'm writing here. At the very list, we should be aware if official patch is not working on clean st code. If uncomfortable with manually applying patches, feel free to use my build-wrapper repository.

cd /tmp # Or wherever
git clone --recursive https://github.com/Iskustvo/simple_terminal.git # Download my build wrapper around st
cd simple_terminal
rm -rf patches/* # Delete all patches that I use
make get_upstream_patch LINK=https://st.suckless.org/patches/glyph_wide_support/st-glyph-wide-support-20220411-ef05519.diff # Download official patch with this fix
make -j8 # Apply downloaded patch and build st
(./st/st &> /dev/null &)& # Open newly built st
# Test behavior from above on any wide glyph symbol
iStagnant commented 1 year ago

This patch is applied to this repository and not clean st. And maybe it does not fix this issue, it does make the glyphs appear fully and not chopped in half, check @Dreomite's first issue to see what i mean by chopped in half. And through the research that i did i found a couple of patches:

And all of these patches fix the issue of glyphs being chopped in half, the only difference between boxdraw compatible and the non compatible ones is that if you apply any of the non compatible patches to a build of st that has boxdraw patched in weird characters appear instead of lines.

Lastly i already confirmed that all these patches that derive from @Dreomite's first patch fix the issue of glyphs getting chopped in half, but i don't know about your issue, so i would like to ask you to git checkout e3b821d this repo and try and see if your issue in fixed in the original commit by @Dreomite if not then this patch was flawed from the beginning and only fixes the issue of glyphs getting chopped in half. ~github's ui on phone is trash i clicked close with comment like 2 times by mistake~

Iskustvo commented 1 year ago

You are correct, I have tested all patches you mentioned (including e3b821d) and all of them fix the original issue as explained by @Dreomite. Unfortunately, none of them fixed the issue I posted here. However, that issue is fixed somewhere on master branch in this repository, so fix for it does exist, either directly or indirectly. I'll try to git bisect it unless someone knowledgeable about it responds here.

Regardless, thanks a lot for all the clarifications!

Iskustvo commented 1 year ago

Ok, so here's the thing. This patch for ligatures solved the issue. More precisely, this is the only thing needed for proper redrawing of wide glyphs:

diff --git a/st.c b/st.c
index 62def59..cc6c78e 100644
--- a/st.c
+++ b/st.c
@@ -2640,7 +2640,8 @@ draw(void)

        drawregion(0, 0, term.col, term.row);
        xdrawcursor(cx, term.c.y, term.line[term.c.y][cx],
-                       term.ocx, term.ocy, term.line[term.ocy][term.ocx]);
+               term.ocx, term.ocy, term.line[term.ocy][term.ocx],
+               term.line[term.ocy], term.col);
        term.ocx = cx;
        term.ocy = term.c.y;
        xfinishdraw();
diff --git a/win.h b/win.h
index 6de960d..94679e4 100644
--- a/win.h
+++ b/win.h
@@ -25,7 +25,7 @@ enum win_mode {

 void xbell(void);
 void xclipcopy(void);
-void xdrawcursor(int, int, Glyph, int, int, Glyph);
+void xdrawcursor(int, int, Glyph, int, int, Glyph, Line, int);
 void xdrawline(Line, int, int, int);
 void xfinishdraw(void);
 void xloadcols(void);
diff --git a/x.c b/x.c
index 43d1ba4..85deee6 100644
--- a/x.c
+++ b/x.c
@@ -1510,14 +1510,17 @@ xdrawglyph(Glyph g, int x, int y)
 }

 void
-xdrawcursor(int cx, int cy, Glyph g, int ox, int oy, Glyph og)
+xdrawcursor(int cx, int cy, Glyph g, int ox, int oy, Glyph og, Line line, int len)
 {
        Color drawcol;

        /* remove the old cursor */
        if (selected(ox, oy))
                og.mode ^= ATTR_REVERSE;
-       xdrawglyph(og, ox, oy);
+
+       /* Redraw the line where cursor was previously.
+        * It will restore wide glyphs and ligatures broken by the cursor. */
+       xdrawline(line, 0, oy, len);

        if (IS_SET(MODE_HIDE))
                return;

I don't know if other patches also include this redrawing part of the code, so I opened PR for official wiki to add separate patch that includes the original solution + this change above. Though I still have to figure out where to review my opened PR and potential review comments :laughing:

iStagnant commented 1 year ago

Nice job @Iskustvo! Since your solution is better i will close my PR and once you're done with everything you could open a PR here.

Iskustvo commented 1 year ago

No, no, you misunderstood me, sorry I wasn't clear enough! I meant that the only thing missing from the original/standalone patch for wide glyphs is the extracted snippet which solves my redrawing issue. In this repository, you already have that part in the code base (added through patch for ligatures) and only need to provide what you already have in this PR.

So, regarding this PR, everything is good as-is, "my" addition is only needed for standalone version of this patch, for someone who applies it on a clean st, like I do.

Therefore, please reopen this PR, you should already be familiar with the process :smile:

iStagnant commented 1 year ago

Ah ok thank you for clarifying, i will reopen the PR.

arsanysamuel commented 7 months ago

Thank you for implementing this fix, it works fine on st 0.9 but it adds a weird border to the top and left side of the terminal window as in the screenshot

Other patches I've applied:

image