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

Write characters one by one to display full-width characters correctly #108

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

shugo
Copy link

@shugo shugo commented Mar 13, 2019

Otherwise, full-width characters overlap on a new Windows console.

win_fullwidth_ng

Otherwise, full-width characters overlap on a new Windows console.
@shugo shugo force-pushed the new_windows_console_workaround branch from da2ce19 to 27960e7 Compare March 13, 2019 09:03
@shugo shugo changed the title Writes characters one by one to display full-width characters correctly Write characters one by one to display full-width characters correctly Mar 13, 2019
@Bill-Gray
Copy link
Owner

Thank you; this looks good!
I notice you have this only enabled with -DNEW_WINCON_WORKAROUND. Is there any reason not to use your new code all the time, removing the #ifdefs and leaving Makefile.mng alone?
I think it's possible that if we did that, the code would be slower (because of displaying one character at a time, instead of displaying a whole string at once). I haven't tested the speed yet. If it is slower, I would probably revise the code to check the string for instances of DUMMY_CHAR_NEXT_TO_FULLWIDTH. If they occurred, I would use your new code (and display characters one at a time). If there were no fullwidth characters, then the current code could be used and the entire string displayed at once.
We could actually get a little more clever than this : if the input string was, for example, "TextFullwidth" (with len=13), we could recursively call PDC_transform_line with len=4 and len=9. But I'll first check to see if there is any speed issue to worry about.

@shugo shugo closed this Mar 14, 2019
@shugo shugo reopened this Mar 14, 2019
@shugo
Copy link
Author

shugo commented Mar 14, 2019

Thank you; this looks good!
I notice you have this only enabled with -DNEW_WINCON_WORKAROUND. Is there any reason not to use your new code all the time, removing the #ifdefs and leaving Makefile.mng alone?

The only reason is that I was not sure whether this fix is acceptable all the time.

I think it's possible that if we did that, the code would be slower (because of displaying one character at a time, instead of displaying a whole string at once). I haven't tested the speed yet. If it is slower, I would probably revise the code to check the string for instances of DUMMY_CHAR_NEXT_TO_FULLWIDTH. If they occurred, I would use your new code (and display characters one at a time). If there were no fullwidth characters, then the current code could be used and the entire string displayed at once.
We could actually get a little more clever than this : if the input string was, for example, "TextFullwidth" (with len=13), we could recursively call PDC_transform_line with len=4 and len=9. But I'll first check to see if there is any speed issue to worry about.

That sounds great. Thank you!

@GitMensch
Copy link
Collaborator

Thank you for the cleanup. I don't think there is a reason to have the define and the !PDC_NEW_WINCON_WORKAROUND code in, is there?

@Bill-Gray: You wanted to check the speed issue and tweak the performance by first checking if the single-display is necessary, correct?
A performance-check should output something like https://en.wikipedia.org/wiki/UTF-16 or bigger to be a reasonable check.

@shugo
Copy link
Author

shugo commented Apr 16, 2019

Thank you for the cleanup. I don't think there is a reason to have the define and the !PDC_NEW_WINCON_WORKAROUND code in, is there?

No. Feel free to modify my code.

@Bill-Gray
Copy link
Owner

Bill-Gray commented Apr 18, 2019

Just returned to this, and I think there's a simpler solution to this problem, requiring two new lines of code and the removal of one old line :
https://www.projectpluto.com/temp/pdcdisp.c
Essentially, when we see a DUMMY_CHAR_NEXT_TO_FULLWIDTH, we replace it with a space. This has the same effect as your fix, but enables us to still output everything at once (so, no possible performance issues.)
It works on my Win10 box. I only hesitate because I realized that maybe we do need your PDC_NEW_WINCON_WORKAROUND flag. I don't have a Win8, Win7, or non-updated Win10 box. Does this fix (and yours) cause each fullwidth character to be preceded by a space? In other words, does the fix turn 'fullwidth' into 'one-and-a-half-width'?
If it does, I don't think it will be hard to fix things to work on all forks of Windows. It will mean that the line that originally read

if( (srcp[src] & A_CHARTEXT) != DUMMY_CHAR_NEXT_TO_FULLWIDTH)

would be modified to read

if( ((srcp[src] & A_CHARTEXT) != DUMMY_CHAR_NEXT_TO_FULLWIDTH) || post_2016_windows_console

so that if it's an old console, we'll continue to discard 'dummies', and on new consoles, we'll translate them into spaces.
If somebody reports seeing '1.5-width' characters on older Windows, I will investigate how one determines which Windows console is running so that post_2016_windows_console can be set correctly.

netbsd-srcmastr referenced this pull request in NetBSD/pkgsrc May 6, 2019
### 1.3.1 / 2019-04-21

Bug fixes:

* Check whether sizeof(WINDOW) is available to avoid build failures on macOS.
  Issue #48 reported by chdiza.

### 1.3.0 / 2019-04-16

New features:

* Add Curses::Form and Curses::Field.

Bug fixes:

* Fix TravisCI issues by amatsuda and znz.
* Fix typo in sample/menu.rb by binford2k.
* Ctrl-/ should return ^_ on Windows.
* Workaround for new Windows console.
  https://github.com/Bill-Gray/PDCurses/pull/108
netbsd-srcmastr referenced this pull request in NetBSD/pkgsrc May 19, 2019
### 1.3.1 / 2019-04-21

Bug fixes:

* Check whether sizeof(WINDOW) is available to avoid build failures on macOS.
  Issue #48 reported by chdiza.

### 1.3.0 / 2019-04-16

New features:

* Add Curses::Form and Curses::Field.

Bug fixes:

* Fix TravisCI issues by amatsuda and znz.
* Fix typo in sample/menu.rb by binford2k.
* Ctrl-/ should return ^_ on Windows.
* Workaround for new Windows console.
  https://github.com/Bill-Gray/PDCurses/pull/108
@GitMensch
Copy link
Collaborator

Just returned to this, and I think there's a simpler solution to this problem, requiring two new lines of code and the removal of one old line

@Bill-Gray It's been a while, but if it is that easy can you include it in 4.2? @shugo I assume that change fixes the issue you see, doesn't it?

A note @Bill-Gray: if needed I can recheck for issues with a WinXP and ReactOS VM.

@Bill-Gray
Copy link
Owner

Apparently, this is not as easy as I thought.

I recompiled on my Win10 machine, which has had the unavoidable MS-pushed patches. Now, the fullwidth character really is fullwidth. Combine with the space next to it, and each such character consumes three columns instead of two.

To get this to work, it would appear we will need to puzzle out the conditions under which MS rends fullwidth characters in two columns, versus those where it is rendered in one and we really do need that space. We should do that, but I don't think it will be a 4.2 kind of thing.

@GitMensch
Copy link
Collaborator

Agreed. So no need for @shugo to investigate the "simple but not working solution".
It would be a good issue for a later version.

@GitMensch
Copy link
Collaborator

@Bill-Gray with 4.2 long gone I do wonder where this stands and if you find some time to re-inspect / fix...

@Bill-Gray
Copy link
Owner

Bill-Gray commented Jun 2, 2023

Hmmm... turns out to be easier to handle than I thought; I've implemented a solution in commit fac87bd.

The problems arise after displaying a fullwidth character : we may (or may not) then be misaligned by one column. The solution is to say : if we have a packet with a fullwidth character in it (which we'll know because it has a DUMMY_CHAR_AFTER_FULLWIDTH following it), break the packet up into one containing stuff before the dummy character, and a packet containing everything after the dummy character. (Which somewhat resembles @shugo's original suggestion.)

This has the advantage of not requiring us to know whether a given system is in the "fullwidth characters advance the cursor by two columns" or "fullwidth characters advance the cursor by one column" camp. (I think Wine, Win10, and Win11 are in the first camp, and all other systems in the latter camp... but again, with the proposed solution, we don't actually need to figure that out.)

An additional benefit : I think this makes handling of combining characters rather simple. I have a solution that works, at least on Wine. I'm having problems testing it on my Win10 box, due to shortcomings in the fonts provided for the Windows console : they lack fullwidth and combining characters. It's not a PDCursesMod issue; Microsoft, in its finite wisdom, just doesn't provide those glyphs. Cut/paste, say, Fullwidth into the console, and you get garbage. So I'll have to install a better font, which is apparently doable, but not at all straightforward.

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