SeeItSayItLabel improvements (#593) #792
Conversation
anonymous2585
requested review from
marlenaklein-msft,
AMollis,
shaynie and
a team
as code owners
anonymous2585
force-pushed
the
feature/translation-593
branch
from
3424f03
to
c38bbb9
Compare
|
Changes looks good to me. I would like a second pair of eyes to review before committing |
|
@anonymous2585 the PR LGTM but Could you please update your branch to pickup the latest commits from main and let the pipeline re-run Unity-tests? |
anonymous2585
force-pushed
the
feature/translation-593
branch
from
c38bbb9
to
bafff12
Compare
|
@ms-RistoRK I have rebased my branch on |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@anonymous2585 thank you for your contribution. The changes in this PR are breaking a Unity-test run by the pipelines. The root-cause is that speech recognition is disabled in pipelines. Could you please update your PR so that Unity-test run correctly in pipelines with your changes? Here is a commit with an example on how to disable speech recognition during pipeline unity-tests. |
|
I can't view your first link because it has restriced access. I'm not sure how to replicate the pipeline configuration on my machine. When I run the powershell script |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Thank you for your reply. I apologize to refer to a commit you couldn't access. The piece of code I wanted to share is this one: |
Hello @anonymous2585, I understand your situation. I would recommend to use the pipelines we have in place in this repo already. One easy way to trigger the execution of the pipelines is by just adding a comment in the PR with following content: '/azp run' for example: As for disabling the SpeechInteractor in your test you can add code similar to this at the beginning of your test: |
|
@ms-RistoRK I can see the piece of code from the beginning. What I can't see is the per-test results details of the pipeline on https://dev.azure.com/. Even when I log to my microsoft account, I'm unauthorized. I don't understand why my new test should disable the speech interactor while the existing tests are ok with it enabled. I copy/pasted the structure of them. |
My bad, I was in another context (XRI3 migration). Whenever you need a detailed result of a failing test please post a comment in your PR requesting the detailed result and we will gladly provide it to you. Unfortunately, we cannot share access to our internal systems to external personnel. As for your latest run here is the detailed output of the failing test, hope this helps: |
|
/azp run |
|
Commenter does not have sufficient privileges for PR 792 in repo MixedRealityToolkit/MixedRealityToolkit-Unity |
anonymous2585
force-pushed
the
feature/translation-593
branch
from
1f2d6ca
to
1560278
Compare
|
@ms-RistoRK Thank you for the details, it has helped a lot!
As you can see, I can't run the pipeline by myself, please start it to see if it's fixed now. |
anonymous2585
force-pushed
the
feature/translation-593
branch
from
1560278
to
5001c9b
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@anonymous2585 Hello, we had a broken build pipeline after a VM image update. The issue has been fixed and available in main branch. You may want to update your branch to pick that change. Have a great weekend! :) |
anonymous2585
force-pushed
the
feature/translation-593
branch
from
5001c9b
to
30fdf21
Compare
|
@ms-RistoRK Hello, no problem. |
anonymous2585
force-pushed
the
feature/translation-593
branch
from
30fdf21
to
16396ff
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@anonymous2585, As others have said. Thank you for your contribution. My apologies for not reviewing this sooner. Wanted to leave a note that I am approving and merging this change. Thanks again! |
Signed-off-by: David Kline <[email protected]>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
org.mixedrealitytoolkit.uxcore/SeeItSayIt/SeeItSayItLabelEnabler.cs
Outdated
Show resolved
Hide resolved
| #if UNITY_LOCALIZATION_PRESENT | ||
| [SerializeField] | ||
| [Tooltip("The LocalizedString that define the label pattern. Use a smart string with one argument that will be replaced by the button's speech recognition keyword (e.g: \"Say '{0}'\").")] | ||
| private LocalizedString localizedPattern; | ||
| #else | ||
| [SerializeField] | ||
| [Tooltip("The patern for the see-it say-it label using string.Format()")] | ||
| private string pattern = "Say '{0}'"; |
There was a problem hiding this comment.
Do we have any prefabs with this script on them in an MRTK package? If so, I wonder if this will lead to Unity logs (Saving Prefab to immutable folder is not allowed) around trying to reserialize immutable files.
There was a problem hiding this comment.
Great question @keveleigh! I missed that one.
There was a problem hiding this comment.
Yes, some prefab use this script ("Action Button", "Action Button Checkbox", "SimpleActionButton" and maybe more).
I use this improvement in my main project for a long time, and never seen this message. In what situation to you think it can happen? User can't save (= reserialize) prefabs that are in a package.
…er.cs Co-authored-by: Kurtis <[email protected]> Signed-off-by: David Kline <[email protected]>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Oh, good catch @david-c-kline! It never caused me errors in my project, but could have. Thank you for the review! |
Improved the SeeItSayItLabel:
Added automated test for the label update when the speech keyword is changed.
Fixes #593