Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

patched in st-glyph-wide-support #349

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

iStagnant
Copy link

@iStagnant iStagnant commented Jan 5, 2023

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

@Iskustvo
Copy link

Iskustvo commented Jan 6, 2023

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:

  • Most left terminal is st without this patch.
  • Terminal in the middle is st with this patch applied
  • Most right terminal is alacritty which apparently has this mostly solved.

Moving forward:

move_left_to_right
From this gif, we can observe:

  • All 3 terminals are able to display character correctly upon loading file.
  • When wide glyph character is selected by the block cursor:
    • st version without this patch redraws (and highlights) just the original space of this character, while remainder remains without highlight.
    • st version with this patch redraws (and highlights) wide character as a whole, so that part is certainly fixed by this.
    • alacritty also fails to redraw whole character, just like st without this patch.
  • When character after wide glyph (in this case, that is space) is selected by the block cursor:
    • st version without this patch redraws that character and chops the remainder of the wide glyph.
    • st version with this patch does the same thing, unfortunately.
    • alacritty also draws over remainder of wide glyph character, but obviously remembers that it needs to restore it after next movement.

Move backwards:

move_right_to_left
From this gif, we can observe:

  • Once the wide glyph character is chopped after initial drawing:
    • st without this patch can't properly redraw it, even when moving over it (in both directions).
    • st with this patch is actually able to properly redraw it once it's selected again (from both directions), so that is also fixed.
    • alacritty had it already working so it just works normally without needing to do anything special.

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:

  • st without this patch doesn't have it restored.
  • st with this patch also doesn't have it restored.
  • alacritty has it restored as soon as it is done with highlighting character after it.

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

@iStagnant
Copy link
Author

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
Copy link

Iskustvo commented Jan 6, 2023

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 😞

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 🤣

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 iStagnant closed this Jan 6, 2023
@iStagnant iStagnant reopened this Jan 6, 2023
@iStagnant iStagnant closed this Jan 6, 2023
@iStagnant iStagnant reopened this Jan 6, 2023
@iStagnant
Copy link
Author

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:

  • A boxdraw compatible patch which i applied here because this repo has boxdraw patched in.
  • A couple non boxdraw compatible patches:
  1. The first is this one from the patches section on suckless's site which works on the latest version of st.
  2. This one created by @mohkale which also work on the latest version of st.
  3. And obviously the original patch done by @Dreomite which only works on st versions below 0.8.4

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
Copy link

Iskustvo commented Jan 6, 2023

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
Copy link

Iskustvo commented Jan 7, 2023

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 😆

Iskustvo added a commit to Iskustvo/simple_terminal that referenced this pull request Jan 7, 2023
@iStagnant
Copy link
Author

iStagnant commented Jan 7, 2023

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.

@iStagnant iStagnant closed this Jan 7, 2023
@Iskustvo
Copy link

Iskustvo commented Jan 7, 2023

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 😄

@iStagnant
Copy link
Author

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

@iStagnant iStagnant reopened this Jan 7, 2023
thatguynoe added a commit to thatguynoe/st that referenced this pull request Jan 9, 2023
Wide glyph truncation still occurs with the glyph-wide-support patch; see LukeSmithxyz/st#349 (comment). The code provided by LukeSmithxyz/st#349 (comment) completely fixes truncation.
nathanielevan added a commit to nathanielevan/st that referenced this pull request Mar 2, 2023
Thanks to Iskustvo; see [this comment](LukeSmithxyz/st#349 (comment)).
@arsanysamuel
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants