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

Fixes #2616. Support combining sequences that don't normalize. #3877

Draft
wants to merge 24 commits into
base: v2_develop
Choose a base branch
from

Conversation

BDisp
Copy link
Collaborator

@BDisp BDisp commented Dec 3, 2024

Fixes

Proposed Changes/Todos

  • Using WindowsDriver only works on Windows Terminal Preview

Pull Request checklist:

  • I've named my PR in the form of "Fixes #issue. Terse description."
  • My code follows the style guidelines of Terminal.Gui - if you use Visual Studio, hit CTRL-K-D to automatically reformat your files before committing.
  • My code follows the Terminal.Gui library design guidelines
  • I ran dotnet test before commit
  • I have made corresponding changes to the API documentation (using /// style comments)
  • My changes generate no new warnings
  • I have checked my code and corrected any poor grammar or misspellings
  • I conducted basic QA to assure all features are working

@BDisp BDisp requested a review from tig as a code owner December 3, 2024 04:22
@BDisp
Copy link
Collaborator Author

BDisp commented Dec 5, 2024

image

Terminal.Gui/ConsoleDrivers/ConsoleDriver.cs Outdated Show resolved Hide resolved
@tig
Copy link
Collaborator

tig commented Dec 5, 2024

This is awesome.

Can you help figure out why charmap is doing this?

Tq8hSSN 1

@BDisp
Copy link
Collaborator Author

BDisp commented Dec 5, 2024

The WindowsDriver it's more harder to deal because it's using a buffer instead of rows and columns like the other drivers. it's needed to add space characters to fill the gaps generated in the columns. It's giving much more work and I couldn't yet figure out the way to fix it.

@BDisp
Copy link
Collaborator Author

BDisp commented Dec 5, 2024

I think the better solution for the charmap is to use views with a width of 2 and a height of 1 for all glyphs. Thus it isn't needed to manipulate the column width of the runes. Also all the views are aligned with each other. What do you think?

@tig
Copy link
Collaborator

tig commented Dec 5, 2024

I think the better solution for the charmap is to use views with a width of 2 and a height of 1 for all glyphs. Thus it isn't needed to manipulate the column width of the runes. Also all the views are aligned with each other. What do you think?

No, that would just mask the issue.

I'd rather us figure out how to fix Windows driver.

@BDisp
Copy link
Collaborator Author

BDisp commented Dec 7, 2024

This approach and the existent one have a real issue when a combining mark is set in the View.Text and the previous rune don't belong to this view. For e.g. if the previous rune belong to label and it's an 'a' starting inserting an '\u0301' in the View.Text, the driver will write 'á' instead of "a´". So, we need to think a better solution to add combining marks. I'll try find a more appropriate solution.

@tig
Copy link
Collaborator

tig commented Dec 8, 2024

This approach and the existent one have a real issue when a combining mark is set in the View.Text and the previous rune don't belong to this view. For e.g. if the previous rune belong to label and it's an 'a' starting inserting an '\u0301' in the View.Text, the driver will write 'á' instead of "a´". So, we need to think a better solution to add combining marks. I'll try find a more appropriate solution.

This is precisely why I started down the Cell route way back.

We need to create an abstraction that lets us keep track of what the user/dev intended and then ensure the driver (where each platform behaves differently) gets the right chars in the right spot.

@tig
Copy link
Collaborator

tig commented Dec 8, 2024

This is relevant: #2933

Particularly the links near end of discussion.

@tznind
Copy link
Collaborator

tznind commented Dec 8, 2024

If possible can you please think about how we will integrate these changes into the v2 drivers refactoring branch too. I've copied much of this code you are changing to the WindowsOutput class.

@BDisp
Copy link
Collaborator Author

BDisp commented Dec 8, 2024

If possible can you please think about how we will integrate these changes into the v2 drivers refactoring branch too. I've copied much of this code you are changing to the WindowsOutput class.

That's good, this way you can check if it's working as expected in the new v2 drivers. I haven't yet resolves all the glyphs handling which I still try to found how to deal with some situations. But when I've this almost finished I'll to see how it can be integrated. WindowsDriver is giving some more work and I really seriously thinking using a for loop for rows and cols in the write to console instead of a foreach, unless I figure that I can deal well to find each last column.

@tig tig marked this pull request as draft December 10, 2024 16:06
@BDisp
Copy link
Collaborator Author

BDisp commented Dec 13, 2024

I already ran the ShowTopLine_True_TabsOnBottom_False_With_Unicode unit test in Linux and Mac and it pass on both. So, I've no idea about it's going on here.

Passed Terminal.Gui.ViewsTests.TabViewTests.RemoveTab_ChangesSelection [2 ms]
[xUnit.net 00:01:34.23]     Terminal.Gui.ViewsTests.TabViewTests.ShowTopLine_True_TabsOnBottom_False_With_Unicode [FAIL]
[xUnit.net 00:01:34.23]       Assert.Equal() Failure: Strings differ
[xUnit.net 00:01:34.23]                                          ↓ (pos 29)
[xUnit.net 00:01:34.23]       Expected: ···"──────╮    \n│Les Misérables│    \n◄       "···
 │hi2               │
 └──────────────────┘
  But Was:
 ╭──────────────╮    
 │Les Miserables│    
 ◄              ╰───╮
 │hi2               │
 └──────────────────┘



Test Run Failed.
Total tests: 10312
     Passed: 10289
     Failed: 1
    Skipped: 22
 Total time: 2.0031 Minutes
       _VSTestConsole:
         MSB4181: The "VSTestTask" task returned false but did not log an error.
     8>Done Building Project "/home/runner/work/Terminal.Gui/Terminal.Gui/UnitTests/UnitTests.csproj" (VSTest target(s)) -- FAILED.
     1>Done Building Project "/home/runner/work/Terminal.Gui/Terminal.Gui/Terminal.sln" (VSTest target(s)) -- FAILED.

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.

Support combining sequences that don't normalize
3 participants