Fixes #2657. Rune parameter in ConsoleDriver.IsRuneSupported is never invalid #2658

BDisp · 2023-05-22T11:38:03Z

Fixes #2657 - Using Rune.IsValid is the best method to check for a valid rune or not.

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

… never invalid

tig · 2023-05-22T22:02:00Z

Terminal.Gui/ConsoleDrivers/ConsoleDriver.cs

-				return false;
-			}
-			return true;
+			return Rune.IsValid (rune.Value);


We should just nuke the IsRuneSupported API and have callers use Rune.IsValid(rune.Value) instead.

@tig I agree nuke the IsRuneSupported but also there is no need to have this callers in AddRune methods because the parameter of type Rune is always valid if this method is executed. Where can happen throwing an exception is when an API call Driver.AddRune(invalid rune) but never inside of the AddRune method. Please say something after you read this. Thanks.

tig · 2023-05-26T10:18:06Z

I will fix this in #2621. We can close this.

tig · 2023-05-26T10:46:23Z

I will fix this in #2621. We can close this.

Note, to be clear, I put this API in for a slightly different purpose. Calling it from AddRune is not needed, however, there ARE cases where app code needs to know if the current console driver supports rendering a particular glpyh correctly. See the Snake code in #2621.

So I won't be removing this API, but clarifying its purpose.

BDisp · 2023-05-26T11:04:47Z

I will fix this in #2621. We can close this.

Note, to be clear, I put this API in for a slightly different purpose. Calling it from AddRune is not needed, however, there ARE cases where app code needs to know if the current console driver supports rendering a particular glpyh correctly. See the Snake code in #2621.

I think you have write the wrong #number. Ok it's #2612.

So I won't be removing this API, but clarifying its purpose.

Yes I understand. You just have to return if the driver supports non-BMP in the overridden methods and always true in the virtual method.

…ed code (#2612) * Added ClipRegion; cleaned up driver code * clip region unit tests * api docs * Moved color stuff from ConsoleDriver to Color.cs * Removes unused ConsoleDriver APIs * Code cleanup and Removes unused ConsoleDriver APIs * Code cleanup and Removes unused ConsoleDriver APIs * Work around #2610 * adjusted unit tests * initial commit * Made Rows, Cols, Top, Left virtual * Made Clipboard non-virtual * Made EnableConsoleScrolling non-virtual * Made Contents non-virtual * Pulled Row/Col up * Made MoveTo virtual; fixed stupid FakeDriver cursor issue * Made CurrentAttribute non-virtual * Made SetAttribute non-virtual * Moved clipboard code out * Code cleanup * Removes dependecy on NStack from ConsoleDrivers - WIP * Fixed unit tests * Fixed unit tests * Added list of unit tests needed * Did some perf testing; tweaked code and charmap to address * Brough in code from PR #2264 (but commented) * Tons of code cleanup * Fighting with ScrollView * Fixing bugs * Fixed TabView tests * Fixed View.Visible test that was not really working * Fixed unit tests * Cleaned up clipboard APIs in attempt to track down unit test failure * Add Cut_Preserves_Selection test * Removed invalid code * Removed invalid test code; unit tests now pass * EscSeq* - Adjusted naming, added more sequences, made code more consistent, simplified, etc... * Added CSI_SetGraphicsRendition * NetDriver code cleanup * code cleanup * Cleaned up color handling in NetDriver * refixed tabview unit test * WindowsDriver color code cleanup * WindowsDriver color code cleanup * CursesDriver color code cleanup * CursesDriver - Adding _BOLD has no effect. Further up the stack we cast the return of ColorToCursesColor from int to short and the _BOLD values don't fit in a short. * CursesDriver color code - make code more accurate * CursesDriver color code - make code more accurate * Simplified ConsoleDriver.GetColors API * Simplified ConsoleDriver.GetColors API further * Improved encapslation of Attribute; prep for TrueColor & other attributes like blink * Fixes #2249. CharacterMap isn't refreshing well non-BMP code points on scroll. * Use GetRange to take some of the runes before convert to string. * Attempting to fix unit tests not being cleaned up * Fixes #2658 - ConsoleDriver.IsRuneSupported * Fixes #2658 - ConsoleDriver.IsRuneSupported (for WindowsDriver) * Check all the range values and not only the max value. * Reducing code. * Fixes #2674 - Unit test process doesn't exit * Changed Cell to support IsDirty and list of Runes * add support for rendering TrueColor output on Windows merging veeman & tznind code * add colorconverter changes * fixed merged v2_develop * Fixing merge bugs * Fixed merge bugs * Fixed merge bugs - all unit tests pass * Debugging netdriver * More netdriver diag * API docs for escutils * Update unicode scenario to stress more stuff * Contents: Now a 2D array of Cells; WIP * AddRune and ClearContents no longer virtual/abstract * WindowsDriver renders correctly again * Progress on Curses * Progress on Curses * broke windowsdriver * Cleaned up FakeMainLoop * Cleaned up some build warnings * Removed _init from AutoInitShutdown as it's not needed anymore * Removed unused var * Removed unused var * Fixed nullabiltiy warning in LineCanvas * Fixed charmap crash * Fixes #2758 in v2 * Port testonfail fix to v2 * Remove EnableConsoleScrolling * Backport #2764 from develop (clear last line) * Remove uneeded usings * Progress on unicode * Merged in changes from PR #2786, Fixes #2784 * revamp charmap rendering * Charmap option to show glyph widths * Fixed issue with wide glpyhs being overwritten * Fixed charmap startcodepoint change issue * Added abiltiy to see ncurses verison/lib * Fought with CursesDriver; giving up for now. See notes. * Leverage Wcwidth nuget library instaed of our own tables * enhanced charmap Details dialog * Final attempt at fixing curses --------- Co-authored-by: BDisp <[email protected]> Co-authored-by: adstep <[email protected]>

* Added ClipRegion; cleaned up driver code * clip region unit tests * api docs * Moved color stuff from ConsoleDriver to Color.cs * Removes unused ConsoleDriver APIs * Code cleanup and Removes unused ConsoleDriver APIs * Code cleanup and Removes unused ConsoleDriver APIs * Work around #2610 * adjusted unit tests * initial commit * Made Rows, Cols, Top, Left virtual * Made Clipboard non-virtual * Made EnableConsoleScrolling non-virtual * Made Contents non-virtual * Pulled Row/Col up * Made MoveTo virtual; fixed stupid FakeDriver cursor issue * Made CurrentAttribute non-virtual * Made SetAttribute non-virtual * Moved clipboard code out * Code cleanup * Removes dependecy on NStack from ConsoleDrivers - WIP * Fixed unit tests * Fixed unit tests * Added list of unit tests needed * Did some perf testing; tweaked code and charmap to address * Brough in code from PR #2264 (but commented) * Tons of code cleanup * Fighting with ScrollView * Fixing bugs * Fixed TabView tests * Fixed View.Visible test that was not really working * Fixed unit tests * Cleaned up clipboard APIs in attempt to track down unit test failure * Add Cut_Preserves_Selection test * Removed invalid code * Removed invalid test code; unit tests now pass * EscSeq* - Adjusted naming, added more sequences, made code more consistent, simplified, etc... * Added CSI_SetGraphicsRendition * NetDriver code cleanup * code cleanup * Cleaned up color handling in NetDriver * refixed tabview unit test * WindowsDriver color code cleanup * WindowsDriver color code cleanup * CursesDriver color code cleanup * CursesDriver - Adding _BOLD has no effect. Further up the stack we cast the return of ColorToCursesColor from int to short and the _BOLD values don't fit in a short. * CursesDriver color code - make code more accurate * CursesDriver color code - make code more accurate * Simplified ConsoleDriver.GetColors API * Simplified ConsoleDriver.GetColors API further * Improved encapslation of Attribute; prep for TrueColor & other attributes like blink * Fixes #2249. CharacterMap isn't refreshing well non-BMP code points on scroll. * Use GetRange to take some of the runes before convert to string. * Attempting to fix unit tests not being cleaned up * Fixes #2658 - ConsoleDriver.IsRuneSupported * Fixes #2658 - ConsoleDriver.IsRuneSupported (for WindowsDriver) * Check all the range values and not only the max value. * Reducing code. * Fixes #2674 - Unit test process doesn't exit * Changed Cell to support IsDirty and list of Runes * add support for rendering TrueColor output on Windows merging veeman & tznind code * add colorconverter changes * fixed merged v2_develop * Fixing merge bugs * Fixed merge bugs * Add outline for FileSystemColorProvider * Add other known folders * Added remaining cases and test for file colors * Use color provider in FileDialog * Fix default color when UseColors to white * Remove `TestDirectoryContents_Windows_Colors` * Remove unused helper method * Fix formatting * Fixed merge bugs - all unit tests pass * Debugging netdriver * More netdriver diag * API docs for escutils * Update unicode scenario to stress more stuff * Contents: Now a 2D array of Cells; WIP * AddRune and ClearContents no longer virtual/abstract * WindowsDriver renders correctly again * Progress on Curses * Progress on Curses * broke windowsdriver * Cleaned up FakeMainLoop * Cleaned up some build warnings * Removed _init from AutoInitShutdown as it's not needed anymore * Removed unused var * Removed unused var * Fixed nullabiltiy warning in LineCanvas * Fixed charmap crash * Fixes #2758 in v2 * Remove accidentally re-added test * Removed unit test xml file * Remove redundant test --------- Co-authored-by: Tig Kindel <[email protected]> Co-authored-by: BDisp <[email protected]> Co-authored-by: adstep <[email protected]>

Fixes gui-cs#2657. Rune parameter in ConsoleDriver.IsRuneSupported is…

bf2337a

… never invalid

BDisp requested review from migueldeicaza and tig as code owners May 22, 2023 11:38

Fix a typo.

9bb67cf

tig requested changes May 22, 2023

View reviewed changes

Merge branch 'v2_develop' into v2_IsRuneSupported-fix_2657

c4690fa

tig closed this May 26, 2023

BDisp deleted the v2_IsRuneSupported-fix_2657 branch May 26, 2023 10:22

tig added a commit to tig/Terminal.Gui that referenced this pull request May 26, 2023

Fixes gui-cs#2658 - ConsoleDriver.IsRuneSupported

47a75b3

tig added a commit to tig/Terminal.Gui that referenced this pull request May 26, 2023

Fixes gui-cs#2658 - ConsoleDriver.IsRuneSupported (for WindowsDriver)

279deaa

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #2657. Rune parameter in ConsoleDriver.IsRuneSupported is never invalid #2658

Fixes #2657. Rune parameter in ConsoleDriver.IsRuneSupported is never invalid #2658

BDisp commented May 22, 2023

tig May 22, 2023

BDisp May 22, 2023

tig commented May 26, 2023

tig commented May 26, 2023

BDisp commented May 26, 2023 •

edited

Loading

Fixes #2657. Rune parameter in ConsoleDriver.IsRuneSupported is never invalid #2658

Fixes #2657. Rune parameter in ConsoleDriver.IsRuneSupported is never invalid #2658

Conversation

BDisp commented May 22, 2023

Pull Request checklist:

tig May 22, 2023

Choose a reason for hiding this comment

BDisp May 22, 2023

Choose a reason for hiding this comment

tig commented May 26, 2023

tig commented May 26, 2023

BDisp commented May 26, 2023 • edited Loading

BDisp commented May 26, 2023 •

edited

Loading