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

Update Code and Fixed Some Warning ⚠ #10526

Merged
merged 20 commits into from
Jun 21, 2022
Merged

Update Code and Fixed Some Warning ⚠ #10526

merged 20 commits into from
Jun 21, 2022

Conversation

BillyFrcs
Copy link
Contributor

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
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@BillyFrcs
Copy link
Contributor Author

Why do the pipelines make some errors?

@timGerken
Copy link
Contributor

@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
Contributor Author

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

@keveleigh
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@azure-pipelines
Copy link

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

@ghost
Copy link

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
@BillyFrcs BillyFrcs requested a review from RogPodge May 20, 2022 05:28
@RogPodge
Copy link
Contributor

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
Contributor Author

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
SECURITY.md Outdated Show resolved Hide resolved
@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
Assets/MRTK/Providers/WindowsMixedReality/XRSDK/WindowsMixedRealityEyeGazeDataProvider.cs
@@ -96,7 +97,7 @@ public override void Enable()
private async void EnableIfLoaderBecomesActive()
{
await new WaitUntil(() => IsActiveLoader.HasValue);
if (IsActiveLoader.Value)
if (IsActiveLoader != null && IsActiveLoader.Value)
Copy link
Contributor

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
Contributor

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
Copy link
Contributor

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
Contributor Author

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
Contributor

/azp run

@azure-pipelines
Copy link

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

@keveleigh keveleigh keveleigh approved these changes

@RogPodge RogPodge RogPodge approved these changes

@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

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.

None yet
5 participants
@BillyFrcs @keveleigh @timGerken @RogPodge @david-c-kline