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: prevent StaggeredLayout floating point errors #573

Merged

Conversation

Poker-sang
Copy link
Contributor

@Poker-sang Poker-sang commented Nov 28, 2024

Fixes

closes #571

PR Type

What kind of change does this PR introduce?

What is the current behavior?

What is the new behavior?

PR Checklist

Please check if your PR fulfills the following requirements:

  • Created a feature/dev branch in your fork (vs. submitting directly from a commit on main)
  • Based off latest main branch of toolkit
  • Tested code with current supported SDKs
  • New component
    • Documentation has been added
    • Sample in sample app has been added
    • Analyzers are passing for documentation and samples
    • Icon has been created (if new sample) following the Thumbnail Style Guide and templates
  • Tests for the changes have been added (if applicable)
  • Header has been added to all new source files
  • Contains NO breaking changes

Other information

@Poker-sang Poker-sang force-pushed the prevent-floating-point-errors branch from 03ca1cb to b02060f Compare November 28, 2024 02:17
@michael-hawker
Copy link
Member

@Poker-sang thanks for submitting a PR! 🦙❤️ Though, looks like you accidentally submitted a change to the tooling submodule?

@Poker-sang Poker-sang force-pushed the prevent-floating-point-errors branch from 2fc7319 to 88e1946 Compare November 28, 2024 08:12
@Poker-sang
Copy link
Contributor Author

@Poker-sang thanks for submitting a PR! 🦙❤️ Though, looks like you accidentally submitted a change to the tooling submodule?

Sorry I didn't notice this before so I just committed all the changes. I didn't realize that submodule changes would be committed to the main repository as well. I'm still not familiar with git's management of submodules.

@michael-hawker michael-hawker added this to the 8.2 milestone Dec 2, 2024
@Poker-sang Poker-sang force-pushed the prevent-floating-point-errors branch from b8090d5 to 30bc857 Compare December 3, 2024 02:38
@Poker-sang Poker-sang force-pushed the prevent-floating-point-errors branch from 30bc857 to 802caaf Compare December 3, 2024 02:40
Copy link
Member

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

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

Quick style pass comment, will checkout and test out soon. Thanks!

@Poker-sang
Copy link
Contributor Author

Poker-sang commented Dec 4, 2024

@michael-hawker I hadn't meant to use this. But the .editorconfig file instructed me that I should use this

Windows/.editorconfig

Lines 89 to 92 in c756dd5

dotnet_style_qualification_for_event = true:silent
dotnet_style_qualification_for_field = true:silent
dotnet_style_qualification_for_method = true:silent
dotnet_style_qualification_for_property = true:silent

@Arlodotexe
Copy link
Member

Arlodotexe commented Dec 5, 2024

@michael-hawker I hadn't meant to use this. But the .editorconfig file instructed me that I should use this

Windows/.editorconfig

Lines 89 to 92 in c756dd5

dotnet_style_qualification_for_event = true:silent
dotnet_style_qualification_for_field = true:silent
dotnet_style_qualification_for_method = true:silent
dotnet_style_qualification_for_property = true:silent

Adding for context https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/ide0003-ide0009#overview

The intellisense here has been silenced. The value is set to true, but it should be set to false.

@Poker-sang
Copy link
Contributor Author

Yes. Though it is silenced. It can still affect code snippets/fixer

@AndrewKeepCoding
Copy link

I ran the fixed sample and confirmed that the flickering is gone.

Recording.2024-12-06.174845.mp4

@AndrewKeepCoding
Copy link

I didn't record a video but I also confirmed that ItemsStrech: None continue to work without flickering.

Copy link
Member

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

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

Approving and merging based on our prior code reviews and discussions, and on behalf of the great validation work done by @AndrewKeepCoding, thanks Andrew! 🦙❤️

@Poker-sang
Copy link
Contributor Author

The ItemsStrech=None is fixed length, so it wouldn't have flickering. Or we wouldn't be able to see it even if it did. Though thank for your test❤.

@michael-hawker michael-hawker enabled auto-merge (squash) December 6, 2024 09:16
@Poker-sang
Copy link
Contributor Author

I thought this is transient and not caused by this pr

Error Message:
   Assert.AreEqual failed. Expected:<2>. Actual:<1>. Unexpected token count after adding.
  Stack Trace:
     at RichSuggestBoxExperiment.Tests.Test_RichSuggestBox.<>c__DisplayClass2_0.<<Test_RichSuggestBox_DeleteTokens>b__0>d.MoveNext() in C:\a\Windows\Windows\components\RichSuggestBox\tests\Test_RichSuggestBox.cs:line 143
--- End of stack trace from previous location ---
   at CommunityToolkit.WinUI.DispatcherQueueExtensions.<>c__DisplayClass3_0.<<EnqueueAsync>b__1>d.MoveNext() in /_/components/Extensions/src/Dispatcher/DispatcherQueueExtensions.cs:line 180
--- End of stack trace from previous location ---
   at RichSuggestBoxExperiment.Tests.Test_RichSuggestBox.Test_RichSuggestBox_DeleteTokens(String token1, String token2) in C:\a\Windows\Windows\components\RichSuggestBox\tests\Test_RichSuggestBox.cs:line 133
   at Microsoft.VisualStudio.TestPlatform.MSTestAdapter.PlatformServices.ThreadOperations.ExecuteWithAbortSafety(Action action)

@michael-hawker
Copy link
Member

Yup, I'll rekick, we have open issues to investigate those, they're super frustrating, but intermittent, and they only seem to happen in our CI, so they've been super hard for us to understand what's happening.

@michael-hawker michael-hawker merged commit 044f82f into CommunityToolkit:main Dec 6, 2024
24 checks passed
@Poker-sang Poker-sang deleted the prevent-floating-point-errors branch December 7, 2024 03:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

StaggeredLayout ItemsStretch Fill unstable when resizing
4 participants