[UUM-133530] Ensure "Set Double Sided" is enabled when active selection is proper#660
[UUM-133530] Ensure "Set Double Sided" is enabled when active selection is proper#660
Conversation
…e enabled in the contextual menu due to a missing override.
![u-pr[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
| @@ -38,9 +38,11 @@ public class MakeFacesDoubleSided : MenuAction | |||
| /// <returns></returns> | |||
There was a problem hiding this comment.
The XML documentation tag is empty and is generally used for methods rather than properties. For properties, the
or tags are standard. Consider removing the empty tag to keep the documentation clean.
🤖 Helpful? 👍/👎
varinotmUnity
left a comment
varinotmUnity
left a comment
There was a problem hiding this comment.
nice, users have been asking for this for a while on forums :)
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## master #660 +/- ##
==========================================
+ Coverage 36.05% 37.04% +0.98%
==========================================
Files 277 277
Lines 34909 34953 +44
==========================================
+ Hits 12588 12948 +360
+ Misses 22321 22005 -316
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
/test_plan |
Test Plan
Summary of Changes & Risk AssessmentSummary of ChangesThis PR fixes the "Set Double Sided" sample action in the ProBuilder Editor samples. It ensures the action correctly enables/disables based on the selection context (via Risk Assessment
Test ScenariosFunctional Testing
Regression Testing
Edge Cases
💡 This test plan updates automatically when 🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr |
unity-kristinn
left a comment
unity-kristinn
left a comment
There was a problem hiding this comment.
suggested by u-pr test plan
Face Selection Validation: Enter Face mode. With no faces selected, verify the action is disabled. Select one or more faces and verify the action becomes enabled.this seems to not be the case for me.
since it’s only a sample, i don’t think it matters much. is this something you would like to fix?
approving anyhow
…tions, nor fileMenuEntry associated to it
There was indeed an issue, we weren't checking the status of the MenuAction when the action doesn't an ²fileMenuEntry` and options. It is now fixed in my commit: 9e40c04 The |
…Technologies/com.unity.probuilder into bugfix/sample-menuaction-disabled
| m_PBMesh = ShapeFactory.Instantiate(typeof(UnityEngine.ProBuilder.Shapes.Plane)); | ||
| } | ||
|
|
||
| public void TearDown() |
There was a problem hiding this comment.
doesn't this need [Teardown] ?
| /// It doesn't cover the cases where a MenuAction has a file menu entry. | ||
| /// </summary> | ||
| [ProBuilderMenuAction] | ||
| public class ConfigurableMenuAction : MenuAction |
There was a problem hiding this comment.
Does this need to be public?
| } | ||
| return (foundInMenu.status == DropdownMenuAction.Status.Normal); | ||
| } | ||
| } |
There was a problem hiding this comment.
lots of duplicated code, can't we combine this in a single method?
There was a problem hiding this comment.
Haven;t tested yet, but quickly with AI, it proposed this:
private DropdownMenuAction GetMenuActionFromContext(SelectMode selectMode)
{
MeshSelection.SetSelection(m_PBMesh.gameObject);
ToolManager.SetActiveContext<PositionToolContext>();
ProBuilderEditor.selectMode = selectMode;
ActiveEditorTracker.sharedTracker.ForceRebuild();
var ctx = Resources.FindObjectsOfTypeAll<PositionToolContext>()?[0];
Assume.That(ctx, Is.Not.Null);
var menu = new DropdownMenu();
ctx.PopulateMenu(menu);
menu.PrepareForDisplay(null);
foreach (var item in menu.MenuItems())
{
if (item is DropdownMenuAction menuAction &&
menuAction.name == ConfigurableMenuAction.actionName)
{
return menuAction;
}
}
return null;
}
There was a problem hiding this comment.
and your test becomes
[Test]
[TestCase(true, ExpectedResult = false)]
[TestCase(false, ExpectedResult = true)]
public bool MenuActionWithoutMenuItem_hasFileMenuEntry(bool hasFileMenuEntry)
{
ConfigurableMenuAction.userHasFileMenuEntry = hasFileMenuEntry;
ConfigurableMenuAction.userSelectMode = SelectMode.Any;
ConfigurableMenuAction.userEnabled = true;
var foundInMenu = GetMenuActionFromContext(SelectMode.Face);
Assert.That(foundInMenu, Is.Not.Null, "MenuAction should be present...");
return (foundInMenu.status == DropdownMenuAction.Status.Normal);
}
|
|
||
| /// <summary> | ||
| /// Test the construction of the contextual menu in the Scene View. | ||
| /// It doesn't cover the cases where a MenuAction has a file menu entry. |
There was a problem hiding this comment.
I'm not sure I understand this comment. Aren't you doing a file menu entry test below?
MenuActionWithoutMenuItem_hasFileMenuEntry
3 participants
DO NOT FORGET TO INCLUDE A CHANGELOG ENTRY
Purpose of this PR
Implementation of the menu action was missing two elements:
base.enabledthat does some extra checks to validate that the selection context is correct with the providedvalidSelectModes. It's a common pattern in ProBuilder (look at all PB's menu action).hasFileMenuEntryneeds to be set to false. Base implementation returnstruebut that would indicate that the custom action exists somewhere in the top menu as the code would try to resolve the action by looking up at the one from the top menu.Also fixed the contextual menu builder so it pulls the status of the MenuAction. In cases where the action doens't have a fileMenuEntry, nor options, it was assumed that the option is always enabeld. I have added some tests to cover these cases.
Links
Jira: UUM-133530