Merge pull request #692 from uni-bremen-agst/679-scrollable-popupmenu

Scrollable PopupMenu and Bugfixes

Assets/Resources/Prefabs/UI/PopupMenuButton.prefab

@@ -331,8 +331,8 @@ RectTransform:
   m_LocalEulerAnglesHint: {x: 0, y: 0, z: 0}
   m_AnchorMin: {x: 0, y: 0}
   m_AnchorMax: {x: 1, y: 1}
-  m_AnchoredPosition: {x: 17.5, y: 0}
-  m_SizeDelta: {x: -55, y: 0}
+  m_AnchoredPosition: {x: 10, y: 0}
+  m_SizeDelta: {x: -70, y: 0}
   m_Pivot: {x: 0.5, y: 0.5}
 --- !u!222 &5059500120611322657
 CanvasRenderer:

Assets/Resources/Prefabs/UI/PopupMenuHeading.prefab

@@ -129,7 +129,7 @@ MonoBehaviour:
   m_VertexBufferAutoSizeReduction: 0
   m_useMaxVisibleDescender: 1
   m_pageToDisplay: 1
-  m_margin: {x: 0, y: 0, z: 0, w: 0}
+  m_margin: {x: 5, y: 0, z: 23, w: 0}
   m_isUsingLegacyAnimationComponent: 0
   m_isVolumetricText: 0
   m_hasFontAssetChanged: 0

Assets/SEE/Controls/Actions/ContextMenuAction.cs

@@ -82,7 +82,7 @@ private static IEnumerable<PopupMenuAction> GetCommonOptions(GraphElement graphE
 
             if (gameObject != null)
             {
-                actions.Add(new("Highlight", Highlight, Icons.LightBulb));
+                actions.Add(new("Show in City", Highlight, Icons.LightBulb));
             }
 
             if (graphElement.Filename() != null)

Assets/SEE/Controls/Actions/ShowTree.cs

@@ -1,4 +1,6 @@
+using System.Collections;
 using System.Collections.Generic;
+using System.Linq;
 using Cysharp.Threading.Tasks;
 using SEE.Game;
 using SEE.Game.City;
@@ -28,7 +30,7 @@ public class ShowTree : MonoBehaviour
         /// <summary>
         /// Displays the tree view window for each code city.
         /// </summary>
-        private void ShowCodeView()
+        private void ShowTreeView()
         {
             GameObject[] cities = GameObject.FindGameObjectsWithTag(Tags.CodeCity);
             if (cities.Length == 0)
@@ -45,9 +47,9 @@ private void ShowCodeView()
                     {
                         continue;
                     }
-                    if (!gameObject.TryGetComponent(out TreeWindow window))
+                    if (!cityObject.TryGetComponent(out TreeWindow window))
                     {
-                        window = gameObject.AddComponent<TreeWindow>();
+                        window = cityObject.AddComponent<TreeWindow>();
                         window.Graph = city.LoadedGraph;
                     }
                     treeWindows.Add(city.name, window);
@@ -76,14 +78,21 @@ async UniTaskVoid SetupManager()
 
         /// <summary>
         /// Close all tree view windows.
+        /// It is assumed that this method is called from a toggle action.
         /// </summary>
-        private void HideCodeView()
+        private void HideTreeView()
         {
+            // If none of the windows were actually closed, we should instead open them.
+            bool anyClosed = false;
             foreach (string city in treeWindows.Keys)
             {
-                space.CloseWindow(treeWindows[city]);
+                anyClosed |= space.CloseWindow(treeWindows[city]);
             }
             treeWindows.Clear();
+            if (!anyClosed)
+            {
+                ShowTreeView();
+            }
         }
 
         private void Update()
@@ -92,11 +101,11 @@ private void Update()
             {
                 if (treeWindows.Count == 0)
                 {
-                    ShowCodeView();
+                    ShowTreeView();
                 }
                 else
                 {
-                    HideCodeView();
+                    HideTreeView();
                 }
             }
         }

Assets/SEE/Controls/WindowSpaceManager.cs

@@ -116,7 +116,7 @@ public void UpdateSpaceFromValueObject(string playerName, WindowSpace.WindowSpac
                 }
 
                 // Close windows which are no longer open
-                closedWindows.ForEach(windowSpaces[playerName].CloseWindow);
+                closedWindows.ForEach(x => windowSpaces[playerName].CloseWindow(x));
 
                 // Set active window if it changed
                 if (windowSpaces[playerName].ActiveWindow.Title != valueObject.ActiveWindow.Title)

Assets/SEE/UI/PopupMenu/PopupMenu.cs

@@ -33,7 +33,23 @@ public class PopupMenu : PlatformDependentComponent
         /// <summary>
         /// The transform under which the entries are listed.
         /// </summary>
-        private RectTransform entryList;
+        private RectTransform actionList;
+
+        /// <summary>
+        /// The scroll view containing the popup items.
+        /// </summary>
+        private RectTransform scrollView;
+
+        /// <summary>
+        /// The scale factor of the canvas.
+        /// Should only be accessed through the <see cref="ScaleFactor"/> property.
+        /// </summary>
+        private float? scaleFactor;
+
+        /// <summary>
+        /// The scale factor of the canvas.
+        /// </summary>
+        private float ScaleFactor => scaleFactor ??= Canvas.MustGetComponent<Canvas>().scaleFactor;
 
         /// <summary>
         /// The content size fitter of the popup menu.
@@ -67,7 +83,8 @@ protected override void StartDesktop()
             menu = (RectTransform)PrefabInstantiator.InstantiatePrefab(menuPrefabPath, Canvas.transform, false).transform;
             contentSizeFitter = menu.gameObject.MustGetComponent<ContentSizeFitter>();
             menuCanvasGroup = menu.gameObject.MustGetComponent<CanvasGroup>();
-            entryList = (RectTransform)menu.Find("Action List");
+            scrollView = (RectTransform)menu.Find("Scroll View");
+            actionList = (RectTransform)scrollView.Find("Viewport/Action List");
 
             // The menu should be hidden when the user moves the mouse away from it.
             PointerHelper pointerHelper = menu.gameObject.MustGetComponent<PointerHelper>();
@@ -89,8 +106,6 @@ protected override void StartDesktop()
 
             // We hide the menu by default.
             menu.gameObject.SetActive(false);
-
-            // TODO (#679): Make this scrollable once it gets too big.
         }
 
         /// <summary>
@@ -126,7 +141,7 @@ public void AddEntry(PopupMenuEntry entry)
         /// <param name="action">The action to be added.</param>
         private void AddAction(PopupMenuAction action)
         {
-            GameObject actionItem = PrefabInstantiator.InstantiatePrefab("Prefabs/UI/PopupMenuButton", entryList, false);
+            GameObject actionItem = PrefabInstantiator.InstantiatePrefab("Prefabs/UI/PopupMenuButton", actionList, false);
             ButtonManagerBasic button = actionItem.MustGetComponent<ButtonManagerBasic>();
             button.buttonText = action.Name;
 
@@ -153,7 +168,7 @@ void OnClick()
         /// <param name="heading">The heading to be added.</param>
         private void AddHeading(PopupMenuHeading heading)
         {
-            GameObject headingItem = PrefabInstantiator.InstantiatePrefab("Prefabs/UI/PopupMenuHeading", entryList, false);
+            GameObject headingItem = PrefabInstantiator.InstantiatePrefab("Prefabs/UI/PopupMenuHeading", actionList, false);
             TextMeshProUGUI text = headingItem.MustGetComponent<TextMeshProUGUI>();
             text.text = heading.Text;
         }
@@ -181,7 +196,7 @@ public void ClearEntries()
                 return;
             }
 
-            foreach (Transform child in entryList)
+            foreach (Transform child in actionList)
             {
                 Destroyer.Destroy(child.gameObject);
             }
@@ -193,11 +208,11 @@ public void ClearEntries()
         /// <param name="position">The position to which the menu should be moved.</param>
         public void MoveTo(Vector2 position)
         {
-            float scaleFactor = Canvas.MustGetComponent<Canvas>().scaleFactor;
-            if (position.y < MenuHeight * scaleFactor)
+            AdjustMenuHeight();
+            if (position.y < MenuHeight * ScaleFactor)
             {
                 // If the menu is too close to the bottom of the screen, expand it upwards instead.
-                position.y += MenuHeight * scaleFactor;
+                position.y += MenuHeight * ScaleFactor;
                 // The mouse should hover over the first menu item already rather than being just outside of it,
                 // so we move the menu down and to the left a bit.
                 position += new Vector2(-5, -5);
@@ -211,6 +226,17 @@ public void MoveTo(Vector2 position)
             menu.position = position;
         }
 
+        /// <summary>
+        /// Adjusts the height of the menu (specifically, the scroll view) to fit the content
+        /// while ensuring that the menu does not take up more than 40% of the screen.
+        /// </summary>
+        private void AdjustMenuHeight()
+        {
+            // Menu should not take up more than 40% of the screen.
+            float height = Mathf.Clamp(actionList.sizeDelta.y, 100f, Screen.height / (2.5f*ScaleFactor));
+            scrollView.sizeDelta = new Vector2(scrollView.sizeDelta.x, height);
+        }
+
         /// <summary>
         /// Activates the menu and fades it in.
         /// This asynchronous method will return once the menu is fully shown.
@@ -226,6 +252,7 @@ public async UniTask ShowMenuAsync()
             contentSizeFitter.enabled = false;
             await UniTask.WaitForEndOfFrame();
             contentSizeFitter.enabled = true;
+            AdjustMenuHeight();
             await UniTask.WhenAll(menu.DOScale(1, animationDuration).AsyncWaitForCompletion().AsUniTask(),
                                   menuCanvasGroup.DOFade(1, animationDuration / 2).AsyncWaitForCompletion().AsUniTask());
         }

Assets/SEE/UI/Window/TreeWindow/TreeWindowContextMenu.cs

@@ -245,15 +245,24 @@ private void UpdateSortMenuEntries()
                 entries.Add(new PopupMenuHeading("Items ordered by group count."));
             }
 
-            // These are the attributes we want to sort by for the time being. We might want to include
-            // all other attributes in the future, in which case the following code needs to be adapted.
-            entries.Add(SortActionFor("Source Name", x => x is Node node ? node.SourceName : null, false));
-            entries.Add(SortActionFor("Source Line", x => x.SourceLine(), true));
-            entries.Add(SortActionFor("Filename", x => x.Filename(), false));
+            // In addition to the type, we want to be able to sort by all existing numeric and string attributes.
             entries.Add(SortActionFor("Type", x => x.Type, false));
+            entries.AddRange(searcher.Graph.AllNumericAttributes().Select(NumericAttributeSortAction));
+            entries.AddRange(searcher.Graph.AllStringAttributes().Select(StringAttributeSortAction));
 
             contextMenu.ClearEntries();
             contextMenu.AddEntries(entries);
+            return;
+
+            PopupMenuAction NumericAttributeSortAction(string attributeKey)
+            {
+                return SortActionFor(attributeKey, x => x.TryGetNumeric(attributeKey, out float value) ? value : null, true);
+            }
+
+            PopupMenuAction StringAttributeSortAction(string attributeKey)
+            {
+                return SortActionFor(attributeKey, x => x.TryGetString(attributeKey, out string value) ? value : null, false);
+            }
         }
 
         /// <summary>
@@ -301,7 +310,8 @@ private static char SortIcon(bool numeric, bool? descending)
         {
             return (numeric, descending) switch
             {
-                (_, null) => ' ',
+                (true, null) => Icons.Hashtag,
+                (false, null) => Icons.Text,
                 (true, true) => Icons.SortNumericDown,
                 (true, false) => Icons.SortNumericUp,
                 (false, true) => Icons.SortAlphabeticalDown,

Assets/SEE/UI/Window/WindowSpace.cs

@@ -89,16 +89,14 @@ public void AddWindow(BaseWindow window)
         /// </summary>
         /// <param name="window">The window which should be closed.</param>
         /// <exception cref="ArgumentNullException">If the given <paramref name="window"/> is <c>null</c>.</exception>
-        public void CloseWindow(BaseWindow window)
+        /// <returns><c>true</c> if the window was closed, <c>false</c> otherwise.</returns>
+        public bool CloseWindow(BaseWindow window)
         {
-            if (window == null)
+            if (ReferenceEquals(window, null))
             {
                 throw new ArgumentNullException(nameof(window));
             }
-            else if (windows.Contains(window))
-            {
-                windows.Remove(window);
-            }
+            return windows.Remove(window);
         }
 
         /// <summary>

Assets/SEE/Utils/Icons.cs

@@ -34,6 +34,8 @@ public static class Icons
         public const char SortAlphabeticalDown = '\uF15D';
         public const char SortNumericUp = '\uF163';
         public const char SortNumericDown = '\uF162';
+        public const char Hashtag = '#';
+        public const char Text = '\uF031';
         public const char EmptyRadio = '\uF111';
         public const char CheckedRadio = '\uF192';
         public const char CircleMinus = '\uF056';

Assets/SEETests/TestDashboard.cs

@@ -12,7 +12,7 @@ namespace SEETests
     /// <summary>
     /// Class which tests the dashboard retrieval, i.e. everything in the <see cref="SEE.Net.Dashboard"/> namespace.
     /// </summary>
-    //[Category("NonDeterministic")]
+    [Category("NonDeterministic")]
     public class TestDashboard
     {
         /**
@@ -34,7 +34,6 @@ public IEnumerator TestDashboardVersionCorrect() => UniTask.ToCoroutine(async ()
             DashboardVersion version = await DashboardRetriever.Instance.GetDashboardVersionAsync();
             Assert.AreEqual(DashboardVersion.SupportedVersion.MajorVersion, version.MajorVersion);
             Assert.AreEqual(DashboardVersion.SupportedVersion.MinorVersion, version.MinorVersion);
-
         });
 
         [UnityTest]