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

Fix CUI setcellwidths characters draw behavior to same GUI behavior. #14540

Conversation

mikoto2000
Copy link
Contributor

Fixed #14539 issue.

@zeertzjq
Copy link
Member

I think this will cause performance problems. For example, if there are a lot of double-width characters on the screen, this will cause a lot of cursor positioning escape sequences even if setcellwidths() is never used.

@mikoto2000
Copy link
Contributor Author

@zeertzjq
Thank you for your comment.

I have changed the processing order so that it checks if the character is specified by setcellwidths before the existing process.

Would this solve the problem?
(Since cw_value was a static function, I created a new function named get_cellwidth for external use.)

diff from master branch:

diff --git a/src/mbyte.c b/src/mbyte.c
index d6fb7ecc7..726843315 100644
--- a/src/mbyte.c
+++ b/src/mbyte.c
@@ -140,6 +140,7 @@ static int dbcs_head_off(char_u *base, char_u *p);
 #ifdef FEAT_EVAL
 static int cw_value(int c);
 #endif
+int get_cellwidth(int c);
 
 /*
  * Lookup table to quickly get the length in bytes of a UTF-8 character from
@@ -5542,6 +5543,20 @@ string_convert_ext(
     return retval;
 }
 
+/*
+ * Return 1 or 2 when "c" is in the cellwidth table.
+ * Return 0 if not.
+ */
+    int
+get_cellwidth(int c)
+{
+#ifdef FEAT_EVAL
+    return cw_value(c);
+#else
+    return 0;
+#endif
+}
+
 #if defined(FEAT_EVAL) || defined(PROTO)
 
 /*
diff --git a/src/proto/mbyte.pro b/src/proto/mbyte.pro
index 7883b3b4c..c57c94c8a 100644
--- a/src/proto/mbyte.pro
+++ b/src/proto/mbyte.pro
@@ -85,6 +85,7 @@ int convert_input(char_u *ptr, int len, int maxlen);
 int convert_input_safe(char_u *ptr, int len, int maxlen, char_u **restp, int *restlenp);
 char_u *string_convert(vimconv_T *vcp, char_u *ptr, int *lenp);
 char_u *string_convert_ext(vimconv_T *vcp, char_u *ptr, int *lenp, int *unconvlenp);
+int get_cellwidth(int c);
 void f_setcellwidths(typval_T *argvars, typval_T *rettv);
 void f_getcellwidths(typval_T *argvars, typval_T *rettv);
 void f_charclass(typval_T *argvars, typval_T *rettv);
diff --git a/src/screen.c b/src/screen.c
index 71ddbcaab..8f0c8f058 100644
--- a/src/screen.c
+++ b/src/screen.c
@@ -1981,6 +1981,29 @@ screen_char(unsigned off, int row, int col)
     {
 	char_u	    buf[MB_MAXBYTES + 1];
 
+	if (get_cellwidth(ScreenLinesUC[off]) > 1)
+	{
+	    // If the width is set to 2 with `setcellwidths`
+
+#ifdef FEAT_GUI
+	    if (!gui.in_use)
+	    {
+#endif
+		// Clear the two screen cells. If the character is actually
+		// single width it won't change the second cell.
+		out_str((char_u *)"  ");
+		term_windgoto(row, col);
+		screen_cur_col = 9999;
+#ifdef FEAT_GUI
+	    }
+#endif
+	}
+	else
+	{
+	    // If it is not specified in `setcellwidths`
+	    // or if a width of 1 is encountered in `setcellwidths`,
+	    // do the same as the original.
+
 	    if (utf_ambiguous_width(ScreenLinesUC[off]))
 	    {
 	    if (*p_ambw == 'd'
@@ -2000,6 +2023,7 @@ screen_char(unsigned off, int row, int col)
 	    }
 	    else if (utf_char2cells(ScreenLinesUC[off]) > 1)
 		++screen_cur_col;
+	}
 
 	// Convert the UTF-8 character to bytes and write it.
 	buf[utfc_char2bytes(off, buf)] = NUL;

@zeertzjq
Copy link
Member

Yes, I think that makes sense, but there is no need to say "same as original" in the comment, and the last else can be followed by if (utf_ambiguous_width(ScreenLinesUC[off])) directly instead of having an extra pair of braces.

@mikoto2000
Copy link
Contributor Author

Thank you. I will fix it!

@mikoto2000 mikoto2000 force-pushed the fix-cui-setcellwidths-character-draw-behavior branch from f385408 to cd945ef Compare April 13, 2024 22:23
@mikoto2000
Copy link
Contributor Author

I fixed and rebased commit.

@chrisbra
Copy link
Member

thanks, can you please add a test for that?

@zeertzjq
Copy link
Member

zeertzjq commented Apr 15, 2024

This can now be partly tested by enabling the skipped part of the test in a59e031 (removing the if 0 and endif lines).

@mikoto2000
Copy link
Contributor Author

@chrisbra @zeertzjq

Thank you.

OK. I will check commit a59e031 and add tests.

@mikoto2000 mikoto2000 force-pushed the fix-cui-setcellwidths-character-draw-behavior branch from cd945ef to 1187d04 Compare April 16, 2024 08:04
@mikoto2000
Copy link
Contributor Author

I added tests and delete if 0 in a59e031 commit.

@chrisbra
Copy link
Member

thanks!

@chrisbra
Copy link
Member

Unfortunately, there are still some CI errors

src/mbyte.c Outdated
@@ -140,6 +140,7 @@ static int dbcs_head_off(char_u *base, char_u *p);
#ifdef FEAT_EVAL
static int cw_value(int c);
#endif
int get_cellwidth(int c);
Copy link
Member

@zeertzjq zeertzjq Apr 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not needed because it's a non-static function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.
Fixed in commit c69bff0.

@mikoto2000
Copy link
Contributor Author

Sorry, I had not set CFLAGS when doing the local build.
I will fix the build error.

Can the following three errors, which also appear in some other CIs, be ignored for this Pull Request?

For example, similar issues have occurred in other CIs, such as: https://github.com/vim/vim/actions/runs/8711945284/

@zeertzjq
Copy link
Member

Added `UNUSED` to the parameter `c` of `get_cellwidth` because it is not used when the build flag `FEAT_EVAL` is not defined.
@mikoto2000
Copy link
Contributor Author

I Fixed build error in commit 87feae6.

@mikoto2000
Copy link
Contributor Author

@zeertzjq

https://github.com/vim/vim/actions/runs/8702081110/job/23897551049 only appears on this PR.

I apologize, I sent the wrong links. Here are the correct three:

@zeertzjq
Copy link
Member

Yes, those are unrelated to this PR.

@mikoto2000
Copy link
Contributor Author

@zeertzjq

Yes, those are unrelated to this PR.

Understood, thank you!

@chrisbra
Copy link
Member

thanks. Looks good now

@chrisbra chrisbra closed this in e20fa59 Apr 17, 2024
zeertzjq added a commit to zeertzjq/neovim that referenced this pull request Apr 17, 2024
Problem:  Cursor wrong after using setcellwidth() in terminal
          (mikoto2000)
Solution: output additional spaces, so the behaviour matches the GUI
          (mikoto2000)

fixes: vim/vim#14539
closes: vim/vim#14540

Fix CUI `setcellwidths` characters draw behavior to same GUI behavior.

vim/vim@e20fa59

This is already fixed and tested in Nvim in neovim#28322.

Co-authored-by: mikoto2000 <mikoto2000@gmail.com>
zeertzjq added a commit to neovim/neovim that referenced this pull request Apr 17, 2024
…al (#28391)

Problem:  Cursor wrong after using setcellwidth() in terminal
          (mikoto2000)
Solution: output additional spaces, so the behaviour matches the GUI
          (mikoto2000)

fixes: vim/vim#14539
closes: vim/vim#14540

Fix CUI `setcellwidths` characters draw behavior to same GUI behavior.

vim/vim@e20fa59

This is already fixed and tested in Nvim in #28322.

Co-authored-by: mikoto2000 <mikoto2000@gmail.com>
huangyingw pushed a commit to huangyingw/neovim that referenced this pull request May 31, 2024
…al (neovim#28391)

Problem:  Cursor wrong after using setcellwidth() in terminal
          (mikoto2000)
Solution: output additional spaces, so the behaviour matches the GUI
          (mikoto2000)

fixes: vim/vim#14539
closes: vim/vim#14540

Fix CUI `setcellwidths` characters draw behavior to same GUI behavior.

vim/vim@e20fa59

This is already fixed and tested in Nvim in neovim#28322.

Co-authored-by: mikoto2000 <mikoto2000@gmail.com>
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.

The behavior when specifying 2 for setcellwidths is different between the GUI version and the CUI version.
3 participants