Skip to content

Update Code and Fixed Some Warning ⚠#10526

Merged
RogPodge merged 20 commits intomicrosoft:mainfrom
BillyFrcs:UpdateCode-And-FixWarning
Jun 21, 2022
Merged

Update Code and Fixed Some Warning ⚠#10526
RogPodge merged 20 commits intomicrosoft:mainfrom
BillyFrcs:UpdateCode-And-FixWarning

Conversation

@BillyFrcs
Copy link
Copy Markdown
Contributor

@BillyFrcs BillyFrcs commented Apr 5, 2022

Overview

  1. Fixed redundant code.
  2. Fixed Possible System.InvalidOperationException warning.
  3. Change some public method to private because it's not being use in the other class and protected as well.
  4. Remove unused variables.
  5. Remove virtual method that hasn't been override.
  6. Fixed Possible System.NullReferenceException warning.

Changes

  • Fixes: Warning Code
  • Fix Possible System.NullReferenceException warning.
  • Fix Possible System.InvalidOperationException warning
  • Fix redundant code

@keveleigh
Copy link
Copy Markdown
Contributor

keveleigh commented Apr 5, 2022

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Apr 5, 2022

Azure Pipelines successfully started running 2 pipeline(s).

@BillyFrcs
Copy link
Copy Markdown
Contributor Author

BillyFrcs commented Apr 6, 2022

Why do the pipelines make some errors?

@timGerken
Copy link
Copy Markdown
Contributor

timGerken commented Apr 6, 2022

@BillyFrcs , neither Unity 2018 nor Unity 2019 support switch pattern matching.

https://dev.azure.com/aipmr/MixedRealityToolkit-Unity-CI/_build/results?buildId=20950&view=logs&j=9a40d849-de61-58a5-b9fb-0035311f3ef5&t=40a1ca36-8c03-582a-2c45-de22c125c2c7&l=11115

@BillyFrcs
Copy link
Copy Markdown
Contributor Author

BillyFrcs commented Apr 6, 2022

Alright understood, Thanks for the correction @timGerken I've already revert it.

@keveleigh
Copy link
Copy Markdown
Contributor

keveleigh commented Apr 11, 2022

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Apr 11, 2022

Azure Pipelines successfully started running 2 pipeline(s).

keveleigh
keveleigh suggested changes Apr 27, 2022
View reviewed changes
Comment thread Assets/MRTK/SDK/Features/UX/Scripts/HandCoach/MoveToTarget.cs Outdated
@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Apr 28, 2022

Commenter does not have sufficient privileges for PR 10526 in repo microsoft/MixedRealityToolkit-Unity

@ghost
Copy link
Copy Markdown

ghost commented Apr 28, 2022 •
edited by ghost
Loading

CLA assistant check
All CLA requirements met.

@BillyFrcs BillyFrcs requested a review from keveleigh May 1, 2022 09:59
RogPodge
RogPodge suggested changes May 20, 2022
View reviewed changes
Comment thread Assets/MRTK/SDK/Features/UX/Scripts/Pointers/BaseControllerPointer.cs Outdated
@BillyFrcs BillyFrcs requested a review from RogPodge May 20, 2022 05:28
@RogPodge
Copy link
Copy Markdown
Contributor

RogPodge commented May 20, 2022

Hey @BillyFrcs, I really appreciate you taking the time to address our feedback. We need all of the public fields that were changed to private/protected be reverted back to public, since they are breaking changes to our api.

We'd also like you to retarget this branch to prerelease/2.8.0 to get this in if you have the time.

@BillyFrcs
Copy link
Copy Markdown
Contributor Author

BillyFrcs commented May 24, 2022

Hey @BillyFrcs, I really appreciate you taking the time to address our feedback. We need all of the public fields that were changed to private/protected be reverted back to public, since they are breaking changes to our api.

We'd also like you to retarget this branch to prerelease/2.8.0 to get this in if you have the time.

Okay, I'll try to take it back.

@BillyFrcs BillyFrcs changed the base branch from main to prerelease/2.8.0 May 24, 2022 08:14
RogPodge
RogPodge suggested changes May 25, 2022
View reviewed changes
Comment thread SECURITY.md
keveleigh
keveleigh reviewed May 25, 2022
View reviewed changes
Comment thread Assets/MRTK/SDK/Features/UX/Scripts/Pointers/BaseControllerPointer.cs
Comment thread Assets/MRTK/SDK/Features/UX/Scripts/Pointers/TeleportPointer.cs Outdated
Comment thread Assets/MRTK/SDK/Features/UX/Scripts/Pointers/ShellHandRayPointer.cs Outdated
BillyFrcs and others added 2 commits May 25, 2022 13:27
@BillyFrcs BillyFrcs requested review from RogPodge and keveleigh May 25, 2022 04:29
@david-c-kline david-c-kline changed the base branch from prerelease/2.8.0 to main May 26, 2022 18:30
RogPodge
RogPodge reviewed Jun 20, 2022
View reviewed changes
Comment thread Assets/MRTK/Providers/WindowsMixedReality/XRSDK/WindowsMixedRealityEyeGazeDataProvider.cs
{
await new WaitUntil(() => IsActiveLoader.HasValue);
if (IsActiveLoader.Value)
if (IsActiveLoader != null && IsActiveLoader.Value)
Copy link
Copy Markdown
Contributor

@RogPodge RogPodge Jun 20, 2022

Choose a reason for hiding this comment

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

@keveleigh is the != null check necessary here?

Copy link
Copy Markdown
Contributor

@keveleigh keveleigh Jun 21, 2022

Choose a reason for hiding this comment

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

Shouldn't be, since we do it after we call IsActiveLoader.HasValue without a null check in the previous line.

RogPodge
RogPodge reviewed Jun 20, 2022
View reviewed changes
Comment thread Assets/MRTK/SDK/Features/UX/Scripts/Loader/LoaderController.cs Outdated
@RogPodge
Copy link
Copy Markdown
Contributor

RogPodge commented Jun 20, 2022

Sorry for the delay on accepting this PR. Left some last comments, after which I think this PR should be good to go!

@BillyFrcs
Copy link
Copy Markdown
Contributor Author

BillyFrcs commented Jun 20, 2022

Sorry for the delay on accepting this PR. Left some last comments, after which I think this PR should be good to go!

Alright, no problem!

@BillyFrcs BillyFrcs requested a review from RogPodge June 20, 2022 23:36
@RogPodge
Copy link
Copy Markdown
Contributor

RogPodge commented Jun 21, 2022

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Jun 21, 2022

Azure Pipelines successfully started running 1 pipeline(s).

@RogPodge RogPodge merged commit ec6cbf1 into microsoft:main Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@julenka julenka Awaiting requested review from julenka

@david-c-kline david-c-kline Awaiting requested review from david-c-kline

@cre8ivepark cre8ivepark Awaiting requested review from cre8ivepark

2 more reviewers

@keveleigh keveleigh keveleigh approved these changes

@RogPodge RogPodge RogPodge approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@david-c-kline david-c-kline

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@BillyFrcs @keveleigh @timGerken @RogPodge @david-c-kline