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.

Sign up for GitHub

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

Jump to bottom

Allow resetting VirtualizedDynamicScrollRectList (#540) #541

Merged
merged 14 commits into from
Dec 1, 2023
Merged

Allow resetting VirtualizedDynamicScrollRectList (#540) #541

merged 14 commits into from
Dec 1, 2023

Conversation

ahmed-shariff
Copy link
Contributor

@ahmed-shariff ahmed-shariff commented Nov 6, 2023 •
edited
Loading

In response to #540

This PR add a Reset public method to VirtualizedScrollRectList. The OnValidate method already contains the resetting functionality, this PR simply moves that to a new Reset method accessible from outside the class.

AMollis
AMollis requested changes Nov 7, 2023
View reviewed changes
Copy link
Contributor

@AMollis AMollis left a comment

Choose a reason for hiding this comment

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

Please add a unit test that validates the "reset" functionality.

org.mixedrealitytoolkit.uxcore/Experimental/List/VirtualizedScrollRectList.cs Outdated Show resolved Hide resolved
org.mixedrealitytoolkit.uxcore/Experimental/List/VirtualizedScrollRectList.cs Outdated Show resolved Hide resolved
@AMollis AMollis requested a review from keveleigh November 7, 2023 18:44
@AMollis AMollis added Type: Feature Request A request for a new feature that can be included with the next minor version release. Package: UX Core The Project ux core package is impacted by this issue. Status: Do Not Merge The author of the pull request does not want to merge this change yet. Needs: Unit Tests The pull request or issue needs to have unit tests created or defined. labels Nov 7, 2023
@ahmed-shariff
VirtualizedScrollRectList.Reset -> ResetLayout
90a95be 
Reset is a Monobehaviour method, hence renaming to avoid conflict
@ahmed-shariff
Copy link
Contributor Author

Thank you for reviewing the PR. I could use some insight into how the tests are structured. The VirtualizedScrollRectList has a "test" in the dev template:

MixedRealityToolkit-Unity/UnityProjects/MRTKDevTemplate/Assets/Scripts/VirtualizedScrollRectListTester.cs

Line 14 in f669f5a

public class VirtualizedScrollRectListTester : MonoBehaviour

This has some of the functionality I would use in the unit test. Would it be ok to move that to the package? or should I leave that script as it is?

@AMollis
Copy link
Contributor

AMollis commented Nov 9, 2023 •
edited
Loading

Thank you for reviewing the PR. I could use some insight into how the tests are structured. The VirtualizedScrollRectList has a "test" in the dev template:

MixedRealityToolkit-Unity/UnityProjects/MRTKDevTemplate/Assets/Scripts/VirtualizedScrollRectListTester.cs

Line 14 in f669f5a

public class VirtualizedScrollRectListTester : MonoBehaviour

This has some of the functionality I would use in the unit test. Would it be ok to move that to the package? or should I leave that script as it is?

@ahmed-shariff , it looks like the original owner did not write Unit Tests for VirtualizedDynamicScrollRectList, which is unfortunate. Given this, I'll be okay with one of the following:

  1. Setup a new unit test class here https://github.com/MixedRealityToolkit/MixedRealityToolkit-Unity/tree/main/org.mixedrealitytoolkit.uxcore/Tests/Runtime and write a test to validate the reset functionality.
  2. Update the Sample scene, VirtualizedScrollRectList, to demonstrate the "resetting" functionality. Maybe add a button to resize and reset the list?

I worry since this is not common/obvious functionality, there's a higher risk for regression if we don't provide a method to validate.

AMollis
AMollis requested changes Nov 10, 2023
View reviewed changes
Copy link
Contributor

@AMollis AMollis left a comment

Choose a reason for hiding this comment

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

Thank you so much for writing those Unit Test :) Thank you !

I have some very minor suggestions....

org.mixedrealitytoolkit.uxcore/Experimental/List/VirtualizedScrollRectList.cs Outdated Show resolved Hide resolved
org.mixedrealitytoolkit.uxcore/Experimental/List/VirtualizedScrollRectList.cs Outdated Show resolved Hide resolved
org.mixedrealitytoolkit.uxcore/Experimental/List/VirtualizedScrollRectList.cs Outdated Show resolved Hide resolved
AMollis
AMollis previously approved these changes Nov 10, 2023
View reviewed changes
@AMollis
Copy link
Contributor

AMollis commented Nov 10, 2023

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AMollis
Copy link
Contributor

AMollis commented Nov 10, 2023

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AMollis
Copy link
Contributor

AMollis commented Nov 10, 2023

@shaynie , @marlenaklein-msft , @keveleigh .

Once the AZP build completes, I'll merge unless you have feedback

@AMollis AMollis removed Needs: Unit Tests The pull request or issue needs to have unit tests created or defined. Status: Do Not Merge The author of the pull request does not want to merge this change yet. labels Nov 10, 2023
@AMollis AMollis linked an issue Nov 10, 2023 that may be closed by this pull request
[FEATURE REQUEST] Allow VirtualizedScrollRectList to be reused #540
Closed
@keveleigh
Copy link
Contributor

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

AMollis
AMollis requested changes Nov 11, 2023
View reviewed changes
Copy link
Contributor

@AMollis AMollis left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Changing my vote.... the Unit Tests seem to be failing. Please fix Unit Tests

MixedReality.Toolkit.UX.Runtime.Tests.VirtualizedScrollRectListTests.TestVirtualizedScrollRectList_ResetLayout
  Non of the expected items were found in the scollable list (set1).
  Expected: True
  But was:  False

@ahmed-shariff
Copy link
Contributor Author

Changing my vote.... the Unit Tests seem to be failing. Please fix Unit Tests

MixedReality.Toolkit.UX.Runtime.Tests.VirtualizedScrollRectListTests.TestVirtualizedScrollRectList_ResetLayout
  Non of the expected items were found in the scollable list (set1).
  Expected: True
  But was:  False

Not sure what is causing that. The tests were passing on my end. I have made a few updates to when frames are skipped, can we test if they pass again?

@keveleigh
Copy link
Contributor

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AMollis
Copy link
Contributor

AMollis commented Nov 29, 2023

Changing my vote.... the Unit Tests seem to be failing. Please fix Unit Tests

MixedReality.Toolkit.UX.Runtime.Tests.VirtualizedScrollRectListTests.TestVirtualizedScrollRectList_ResetLayout
  Non of the expected items were found in the scollable list (set1).
  Expected: True
  But was:  False

Not sure what is causing that. The tests were passing on my end. I have made a few updates to when frames are skipped, can we test if they pass again?

I'm guessing the Unit Tests were failing because of the previous WaitForEndOfFrame() calls which aren't supported in batch mode. The CI pipeline is running the Unit Tests in batch mode.

AMollis
AMollis previously approved these changes Nov 29, 2023
View reviewed changes
Copy link
Contributor

@AMollis AMollis left a comment

Choose a reason for hiding this comment

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

I approve. Once @keveleigh approve we will merge

org.mixedrealitytoolkit.uxcore/Experimental/List/VirtualizedScrollRectList.cs Show resolved Hide resolved
@ahmed-shariff @keveleigh
Update org.mixedrealitytoolkit.uxcore/Experimental/List/VirtualizedSc…
dc26d6f 
…rollRectList.cs

Co-authored-by: Kurtis <[email protected]>
Signed-off-by: Ahmed Shariff <[email protected]>
@AMollis AMollis enabled auto-merge (squash) November 29, 2023 18:04
@shaynie
Copy link
Contributor

shaynie commented Nov 30, 2023

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AMollis
Copy link
Contributor

AMollis commented Dec 1, 2023

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AMollis AMollis merged commit a09f5cd into MixedRealityToolkit:main Dec 1, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AMollis AMollis AMollis approved these changes

@keveleigh keveleigh keveleigh approved these changes

@marlenaklein-msft marlenaklein-msft marlenaklein-msft left review comments

@shaynie shaynie Awaiting requested review from shaynie shaynie is a code owner

Assignees

@ahmed-shariff ahmed-shariff

Labels
Package: UX Core The Project ux core package is impacted by this issue. Type: Feature Request A request for a new feature that can be included with the next minor version release.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEATURE REQUEST] Allow VirtualizedScrollRectList to be reused
5 participants
@ahmed-shariff @AMollis @keveleigh @shaynie @marlenaklein-msft