Tree view control for displaying data performantly. #151

shana · 2017-07-31T10:40:04Z

Note: This branch targets enhancements/branches-view-rollup #464

This PR introduces a tree view control to display data in a tree view in a performant way, since the layouting system can't really handle a lot of the data we throw into it.

The approach here is to separate the data from the UI - the data gets put into a data structure (with whatever necessary processing), and then passed into the UI structure that will render it. Both can be cached separately and independently.

This tries to solve a number of problems:

Unity can't serialize data recursively (like a tree of nodes that have child nodes), so the data that is fed to the UI must be serialized in a flat structure
The layout system has problems when nodes are collapsed, changing the padding and spacing of items
We need to cache the UI so that it doesn't disappear when Unity reloads the domain. We also need to cache the data that feeds the UI so we can show existing data while we're refreshing it.

Missing in this PR

Scrolling support, rendering only the area that needs to be displayed, so that we don't try to render a 1000 objects

This is currently applied to the branches view but the Treeview control should be generic enough to be applied to the changes view as well, hopefully fixing the performance problems there.

StanleyGoldman · 2017-09-13T15:43:06Z

Testing the latest, it is looking good. Noticed this display issue with remotes and multiple remotes at that.

StanleyGoldman · 2017-10-17T18:39:36Z

I sent this PR a PR.. #372

…e when we're not in playmode

StanleyGoldman · 2017-10-20T15:32:26Z

Sending you a pull request in #378

StanleyGoldman · 2017-10-20T19:32:04Z

Making an edit to say this depends on #384

# Conflicts: # src/GitHub.Api/Cache/CacheManager.cs

…w-fixes

# Conflicts: # src/UnityExtension/Assets/Editor/GitHub.Unity/UI/BranchesView.cs

CapnRat · 2017-11-28T14:47:14Z

src/UnityExtension/Assets/Editor/GitHub.Unity/UI/TreeControl.cs

+
+        [SerializeField] public GUIStyle FolderStyle;
+        [SerializeField] public GUIStyle TreeNodeStyle;
+        [SerializeField] public GUIStyle ActiveTreeNodeStyle;


Normally I wouldn't serialize GUIStyles/Textures, instead I load them on demand (typically in OnEnable). This makes it more difficult to iterate on the source styles/textures without recreating the serialized object completely. Nothing particularly wrong with this approach though.

CapnRat

A few things to clean up, but overall looks good.

CapnRat · 2017-11-28T14:49:01Z

src/UnityExtension/Assets/Editor/GitHub.Unity/UI/TreeControl.cs

+    {
+        [SerializeField] public float ItemHeight = EditorGUIUtility.singleLineHeight;
+        [SerializeField] public float ItemSpacing = EditorGUIUtility.standardVerticalSpacing;
+        [SerializeField] public float Indentation = 12f;


Doesn't look like these three fields ever change? Why serialize them, rather than just making them const values?

CapnRat · 2017-11-28T14:51:28Z

src/UnityExtension/Assets/Editor/GitHub.Unity/UI/TreeControl.cs

+                for (int i = 0; i < parts.Length; i++)
+                {
+                    var label = parts[i];
+                    var name = String.Join("/", parts, 0, i + 1);


The GC probably hates you for all of these joins/splits. You may want to take a look at how much garbage memory you're creating for this operation. And how much time is taking to clean it up.

I'm about to add functionality to the TreeControl to display a different type of tree. So these lines of codes are going to be a stomping ground for a little bit. I'm going to log an issue under tech-debt for this.

CapnRat · 2017-11-28T14:55:44Z

src/UnityExtension/Assets/Editor/GitHub.Unity/UI/TreeControl.cs

+        {
+            bool selectionChanged = false;
+            var clickRect = new Rect(0f, rect.y, rect.width, rect.height);
+            if (Event.current.type == EventType.MouseDown && clickRect.Contains(Event.current.mousePosition))


Looks like this would also intercept right click or middle click. Is that desirable?

That is correct. The fix to this is coming in another pull request.

CapnRat · 2017-11-28T15:16:16Z

src/UnityExtension/Assets/Editor/GitHub.Unity/UI/TreeControl.cs

+
+            if (Event.current.type == EventType.repaint)
+            {
+                nodeStyle.Draw(fillRect, "", false, false, false, isSelected);


GUIContent.none rather than empty string.

Using the Styles.TreeIndentation constant The value never changes so no need to make it serializable

CapnRat

Looks good!

drguthals

Typo on line 215 of TreeControl.cs

drguthals · 2017-11-29T03:00:18Z

src/UnityExtension/Assets/Editor/GitHub.Unity/UI/BranchesView.cs

-            {
-                Repository.CheckLocalAndRemoteBranchListChangedEvent(lastLocalAndRemoteBranchListChangedEvent);
-            }
+            Repository.CheckLocalAndRemoteBranchListChangedEvent(lastLocalAndRemoteBranchListChangedEvent);


Repository is not longer possibly null then?

By the time the BranchesView is displayed we always have a Repository

drguthals · 2017-11-29T03:05:42Z

src/UnityExtension/Assets/Editor/GitHub.Unity/UI/BranchesView.cs

        {
-            if (node == null || node.Children == null)
+            //if (IsFavorite(a.Name))


meant to be commented out? Should it just be removed?

That is getting cleaned up in #468

drguthals · 2017-11-29T03:06:07Z

src/UnityExtension/Assets/Editor/GitHub.Unity/UI/BranchesView.cs

        }

+        //private bool IsFavorite(string branchName)


Commented or deleted?

That is getting cleaned up in #468

drguthals · 2017-11-29T03:13:35Z

src/UnityExtension/Assets/Editor/GitHub.Unity/UI/TreeControl.cs

+                int directionX = Event.current.keyCode == KeyCode.LeftArrow ? -1 : Event.current.keyCode == KeyCode.RightArrow ? 1 : 0;
+                if (directionY != 0 || directionX != 0)
+                {
+                    if (directionY < 0 || directionY < 0)


Should be directionX on the second condition

Good catch!

drguthals · 2017-11-29T03:15:34Z

src/UnityExtension/Assets/Editor/GitHub.Unity/UI/TreeControl.cs

+            {
+                int directionY = Event.current.keyCode == KeyCode.UpArrow ? -1 : Event.current.keyCode == KeyCode.DownArrow ? 1 : 0;
+                int directionX = Event.current.keyCode == KeyCode.LeftArrow ? -1 : Event.current.keyCode == KeyCode.RightArrow ? 1 : 0;
+                if (directionY != 0 || directionX != 0)


I'm a bit confused by this set of if statements. I don't fully understand the functionality here, so that just might be my confusion, but it seems like a set of nested ifs with two different variables shouldn't just be an or?

You are right, the wrapping if statement is redundant.

The local and remote trees are two separate tree controls.
This code lets you travel from one tree control to the other via keystroke if the scenario is correct.

drguthals · 2017-11-29T03:19:09Z

src/UnityExtension/Assets/Editor/GitHub.Unity/UI/TreeControl.cs

+            return idx;
+        }
+
+        private bool HandleInput(Rect rect, TreeNode currentNode, int index, Action<TreeNode> singleClick = null, Action<TreeNode> doubleClick = null)


So are singleClick and doubleClick what gets displayed if a node gets single or double clicked?

Almost... they are the function delegates that get called when a node gets single or double clicked

Got it, thanks!

shana mentioned this pull request Jul 31, 2017

Refactoring and testing changes view tree model objects #121

Merged

2 tasks

StanleyGoldman added the work-in-progress label Jul 31, 2017

shana force-pushed the branches-view branch from 62c1516 to a484729 Compare September 11, 2017 22:50

StanleyGoldman mentioned this pull request Oct 11, 2017

Feature: Add functionality to discard changes #356

Closed

shana and others added 5 commits October 17, 2017 20:41

Update data before OnGUI whenever the repo event happens, and OnEnabl…

10f5e15

…e when we're not in playmode

Removing unused method

b7bb749

Redraw whenever data has changed

55883f2

Merge branch 'master' into shana/refreshing-the-branches-view

4104c4f

Well duh

4654cae

shana force-pushed the branches-view branch from a484729 to 4c57500 Compare October 17, 2017 19:14

Custom treeview control and cache manager

205da5b

shana force-pushed the branches-view branch from 4c57500 to 205da5b Compare October 19, 2017 13:27

StanleyGoldman added 2 commits October 20, 2017 11:30

treeLocals is never null

7cf80c0

Adding branchesHasChanged flag to signal rebuild of tree

99c4ed2

StanleyGoldman mentioned this pull request Oct 20, 2017

Edits to the branches-view branch #378

Merged

StanleyGoldman added 2 commits October 20, 2017 11:47

Adding missing reset of branchesHasChanged

13fcec0

Removing stray newline

466507f

StanleyGoldman changed the base branch from master to features/cache-manager October 20, 2017 19:33

StanleyGoldman changed the base branch from features/cache-manager to master October 20, 2017 19:34

StanleyGoldman added 7 commits October 20, 2017 15:43

Merge branch 'features/cache-manager' into stanley/branches-view-fixes

f37f2b0

# Conflicts: # src/GitHub.Api/Cache/CacheManager.cs

Merge branch 'features/cache-manager' into stanley/branches-view-fixes

560fc55

Removing initialize of BranchCache from BranchView

4cabc10

Merge branch 'master' into stanley/branches-view-fixes

a58a212

Fixes after merge

8921bd8

Merge branch 'features/using-cache-manager' into stanley/branches-vie…

3f2d2f9

…w-fixes

More fixes after the merge

733df51

StanleyGoldman added work-in-progress and removed ready-for-review labels Nov 21, 2017

Merge branch 'master' into branches-view

fff2fce

StanleyGoldman self-requested a review November 27, 2017 13:20

StanleyGoldman changed the base branch from master to enhancements/branches-view-rollup November 27, 2017 13:20

StanleyGoldman approved these changes Nov 27, 2017

View reviewed changes

Merge branch 'enhancements/branches-view-rollup' into branches-view

412fee8

# Conflicts: # src/UnityExtension/Assets/Editor/GitHub.Unity/UI/BranchesView.cs

StanleyGoldman added ready-for-review and removed work-in-progress labels Nov 27, 2017

This was referenced Nov 27, 2017

TreeView Control Rollup #464

Merged

Remote Branches List Error #465

Merged

Merge branch 'enhancements/branches-view-rollup' into branches-view

1410a01

This was referenced Nov 27, 2017

Handling branch right click #467

Merged

BranchesView Cleanup #468

Merged

Functionality to mark folder nodes as "root" #469

Merged

Making the folders selectable via mouse #470

Merged

CapnRat reviewed Nov 28, 2017

View reviewed changes

StanleyGoldman added 3 commits November 28, 2017 11:32

/branches-view-rollup:

f1daf3e

Using the Styles.TreeIndentation constant The value never changes so no need to make it serializable

Preventing serialization of some fields that don't change

65da890

Using GUIContent.none instead of an empty string

87c63f0

CapnRat approved these changes Nov 28, 2017

View reviewed changes

StanleyGoldman mentioned this pull request Nov 29, 2017

Maintaining collapsed state during reload #478

Merged

2 tasks

drguthals suggested changes Nov 29, 2017

View reviewed changes

StanleyGoldman added 3 commits November 28, 2017 22:42

Merge branch 'enhancements/branches-view-rollup' into branches-view

22931e2

Fixing focus typo

3a71d36

Removing redundant if statement

0f49079

drguthals approved these changes Nov 29, 2017

View reviewed changes

StanleyGoldman merged commit 8198c74 into enhancements/branches-view-rollup Nov 29, 2017

StanleyGoldman deleted the branches-view branch November 29, 2017 15:14

Tree view control for displaying data performantly. #151

Tree view control for displaying data performantly. #151

Uh oh!

Conversation

shana commented Jul 31, 2017 • edited by StanleyGoldman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

StanleyGoldman commented Sep 13, 2017

Uh oh!

StanleyGoldman commented Oct 17, 2017

Uh oh!

StanleyGoldman commented Oct 20, 2017

Uh oh!

StanleyGoldman commented Oct 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CapnRat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CapnRat left a comment

Choose a reason for hiding this comment

Uh oh!

drguthals left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shana commented Jul 31, 2017 •

edited by StanleyGoldman

Loading