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 #3499. Finishes Dim.Auto implementation #3615

Merged
merged 65 commits into from
Jul 25, 2024

Conversation

tig
Copy link
Collaborator

@tig tig commented Jul 14, 2024

Fixes

Proposed Changes/Todos

  • Todo 1

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

@tig tig self-assigned this Jul 14, 2024
Copy link
Collaborator

@dodexahedron dodexahedron left a comment

Choose a reason for hiding this comment

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

A couple of suggestions around completeness/formality and performance.

Otherwise, 👍

Terminal.Gui/Application/Application.cs Show resolved Hide resolved
Terminal.Gui/Drawing/Glyphs.cs Show resolved Hide resolved
Terminal.Gui/Text/TextFormatter.cs Outdated Show resolved Hide resolved
Terminal.Gui/Text/TextFormatter.cs Show resolved Hide resolved
Terminal.Gui/Text/TextFormatter.cs Outdated Show resolved Hide resolved
Terminal.Gui/Text/TextFormatter.cs Outdated Show resolved Hide resolved
@tig
Copy link
Collaborator Author

tig commented Jul 16, 2024

@dodexahedron - This is a draft. I think it would be best if you hold off reviewing until I say it's ready for review.

@dodexahedron
Copy link
Collaborator

Yep I was aware. I figure if a PR has been created, may as well take a gander at it and share any thoughts that occur, in case any items impact the unfinished work. 🤷‍♂️

@dodexahedron
Copy link
Collaborator

I'm gonna branch from your branch real quick and shoot you over a PR to your branch for a couple of biggies that keep bugging me in TextFormatter and some of the StringExtensions stuff.

I'll do them in a conditional compile so you can compare easily before taking any replacements and to avoid conflicts.

I suppose I should specify I'll do it when the debugging and refinements here settle down a bit.

@tig
Copy link
Collaborator Author

tig commented Jul 21, 2024

I'm done for now. Go at it.

@dodexahedron
Copy link
Collaborator

Coolio.

I'll try to keep it as small and contained as I can. 😅

Gonna do some benchmarks to verify as well.

Do you have your ARM system handy? I'd love to see how some of the SIMD code behaves on that.

@BDisp
Copy link
Collaborator

BDisp commented Jul 21, 2024

Because the UICatalog.cs file encoded as CRLF on Windows, C#'s verbatim and raw string quotes get screwed up.

Changing the code to oldskool fixes it:

Finally you understanded that using string quotes on unit tests screw the result many times 😃

@dodexahedron
Copy link
Collaborator

dodexahedron commented Jul 21, 2024

I think we might actually have a problem related to newlines that I need to check out, that those comments and some changes I'm working on brought to mind.

I need to test it on other platforms though, so I'll do that another time.

@BDisp
Copy link
Collaborator

BDisp commented Jul 21, 2024

@tig I confirm that the about dialog is now working well on all drivers.
On cross platform using C++ projects that use CMake, automatically the source is copied to the WSL and run the apps or unit tests from there and thus don't have this problem. Using C# this magic isn't automatically done, I think.

@BDisp
Copy link
Collaborator

BDisp commented Jul 21, 2024

Here the print screen:

image

@tig did you can run these "113 Not Run" unit tests?

@tig
Copy link
Collaborator Author

tig commented Jul 21, 2024

Here the print screen:
image

@tig did you can run these "113 Not Run" unit tests?

Github action:

image

Local Windows:

image

Local Ubuntu/WSL:

image

@BDisp
Copy link
Collaborator

BDisp commented Jul 21, 2024

Local Ubuntu/WSL also give me 113 Not Run unit tests. I think it's related for you are using VS2022 preview version and I'm not.

image

I saw your Local Windows unit tests was done with ReSharper Unit Test Explorer. Does the VS2022 Test Explorer showed some Not Run tests?

@tig
Copy link
Collaborator Author

tig commented Jul 21, 2024

Local Ubuntu/WSL also give me 113 Not Run unit tests. I think it's related for you are using VS2022 preview version and I'm not.

image

Click on the blue "Not Run" filiter button. What are the 113 tests? That might provide some clue to what's going on?

I saw your Local Windows unit tests was done with ReSharper Unit Test Explorer. Does the VS2022 Test Explorer showed some Not Run tests?

My version - I'm about to upgrade to Preview 4.0 tho:

image

Test results using Test Explorer. Zero "Not Run" for both Windows and Ubuntu:

image

image

But clearly VS Test Explorer is tracking different things or doesn't know how to count...

Resharper counts differently. Frankly, I trust its ability to count more that VS Test Explorer.

image

Note dotnet test has it's own version of world events:

Github Action (Windows):
image

Local dotnet test (Windows):
image

Local dotnet test (WSL with independent clone of tig/v2_3499-Finish-DimAuto):
image

@BDisp
Copy link
Collaborator

BDisp commented Jul 21, 2024

Click on the blue "Not Run" filiter button. What are the 113 tests? That might provide some clue to what's going on?

image

My VS2022 version:

image

Never mind. It's probably anything already fixed on the preview version. Thanks for your attention.

@tig
Copy link
Collaborator Author

tig commented Jul 21, 2024

I updated my previous post while you were typing, BTW. May want to look at it again.

I doubt this has anything to do with VS preview. Have you tried a dotnet clean and VS restart?

@tig
Copy link
Collaborator Author

tig commented Jul 21, 2024

Do you have your ARM system handy? I'd love to see how some of the SIMD code behaves on that.

@dodexahedron , yep! Just let me know how to run whatever tests you're running. I really want to learn to instrument and measure C# perf better!

@tig
Copy link
Collaborator Author

tig commented Jul 21, 2024

@dodexahedron, am I holding this correctly now?

image

@BDisp
Copy link
Collaborator

BDisp commented Jul 21, 2024

I doubt this has anything to do with VS preview. Have you tried a dotnet clean and VS restart?

Yes. See the count of unit tests differs from Test Explorer and ReSharper Unit Test Explorer and also from your count if unit tests. Strange.

image

image

@BDisp
Copy link
Collaborator

BDisp commented Jul 21, 2024

Finally all tests are ran, although the count of both difere.

image

image

@dodexahedron
Copy link
Collaborator

dodexahedron commented Jul 21, 2024

@dodexahedron, am I holding this correctly now?

image

🤣

Yes

There are several ways to feed it data.

If you want to feed the same values to a bunch of Theories, it's a good idea to just define a static array of it so you can reuse it for them all. That's what the [CombinatorialMemberData(nameof(TheArrayYouMade))] is for.

@dodexahedron
Copy link
Collaborator

Do you have your ARM system handy? I'd love to see how some of the SIMD code behaves on that.

@dodexahedron , yep! Just let me know how to run whatever tests you're running. I really want to learn to instrument and measure C# perf better!

Cool.

I have a bit of code for you to run on that machine if you don't mind. Basically just checking what .net thinks it can and can't do with advanced instruction sets and whatnot on your hardware, OS, and .net combo.

But it's on a machine that is currently being.... let's say temperamental.... Dell will be out tomorrow to replace it so unless I get bored and write that up again it'll be tomorrow. 😅

What chip is it?

@tig
Copy link
Collaborator Author

tig commented Jul 22, 2024

Snapdragon elite

@dodexahedron
Copy link
Collaborator

Coolio. That gets me the hardware info. Now just to see what the runtime reports once I get you that code.

What OS(es) you got on that bad boy?

@tig
Copy link
Collaborator Author

tig commented Jul 22, 2024

Coolio. That gets me the hardware info. Now just to see what the runtime reports once I get you that code.

What OS(es) you got on that bad boy?

image

@dodexahedron
Copy link
Collaborator

Do you happen to also have a bare metal linux distro on there as well?

If not, I can dig up a raspberry pi I'm not currently using for some tests on the chip that's got.

@tig
Copy link
Collaborator Author

tig commented Jul 23, 2024

Do you happen to also have a bare metal linux distro on there as well?

If not, I can dig up a raspberry pi I'm not currently using for some tests on the chip that's got.

nope

@tig
Copy link
Collaborator Author

tig commented Jul 25, 2024

@dodexahedron do you think you'll have your performance patch done today?

I'd really like to get this merged. I need to know if I should wait.

@dodexahedron
Copy link
Collaborator

Go ahead and merge it. I'll rebase.

@tig tig merged commit 0583d45 into gui-cs:v2_develop Jul 25, 2024
3 checks passed
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.

Finish Dim.Auto (Content)
4 participants