Skip to content
This repository was archived by the owner on Dec 5, 2024. It is now read-only.

Changes TreeView Rollup #471

Merged
merged 143 commits into from
Dec 10, 2017
Merged

Conversation

StanleyGoldman
Copy link
Contributor

@StanleyGoldman StanleyGoldman commented Nov 27, 2017

  • Implementation of ChangesTreeControl and integration into ChangesView
    • Adding support for "Icon badges" in the rendering of Tree
  • Refactoring Tree to create testable base structure while making it extendable to provide customization.
    • Mainly the ability to incorporate additional data points in ITreeData for use in building nodes
  • Extracting a dedicated BranchesTreeControl to handle the display of branches
  • Adding functionality to make tree nodes display a checkbox and convey checked states.
  • Integrating focus and selection state of the tree nodes with the rest of Unity
  • Distinct styles to handle the intersection of tree node states and tree focus

Comprised of:

  • Added GitStatusEntryTreeData an object to track a GitStatusEntry object in a Tree
  • Functionality to check/uncheck all nodes
  • Some code cleanup in BranchesView
  • ChangesTreeControl implementation and utilization in ChangesView
  • Updates to the layout mechanics in Tree

This branch has the first few sets of improvements that were needed in order to make the tree view amenable to displaying changes. These are changes and improvements to the Tree display functionality.
This pull request should not change any behavior in the BranchesView

Extraction of tree loading functionality:

  • The code involved in loading a tree's data is complex and I would like to wrap it in some unit tests
  • The loading code utilizes UnityEngine and UnityEditor and must be fully isolated from those libraries in order to be tested
  • TreeLoader provides the logic to load a tree. The interface ITree was created in order to return relevant data and expose necessary functions to TreeLoader while not exposing any objects from UnityEngine and UnityEditor
  • TreeLoader can now be tested in isolation from UnityEngine and UnityEditor
  • Blank test fixture for testing TreeLoader

Separating ITreeData from GitBranch

  • Adding some decoupling between these objects.
  • We will need to send more data than just GitBranch to the TreeLoader about the data being loaded
  • Renamed Name to Path as that is more appropriate for the data being displayed

Changes to Tree

  • Functionality to set the root node's text
  • Functionality to set the path separator
  • Functionality to control if the root node is displayed
  • Initial functionality to handle the checking of checkboxes

Changes to TreeNode

  • Functionality to display a checkbox next to tree nodes

Functionality to toggle checked state in Tree.
To properly handle checks and unchecks we have two major tasks:

  • Ripple checks on folders down to all children recursively
  • Ripple checks on items up through their folders recursively

What makes this tricky is that nodes is a flat list. It is not an object with a Children property that can be traversed.

Also added functionality to perform a check on spacebar.

Initially tree loading functionality was isolated to GitHub.Api in order to be able to be tested.
This is not the only functionality around Tree that needs to be testable.
The functionality to toggle visibility and checked state is also quite complex and could serve to be isolated.
Also with some crafty use of generics we can make the additional tree types easier to implement.

  • Created TreeBase<TNode, TData>
    • Changed Tree to Tree<TNode, TData>: TreeBase<TNode, TData>
    • Moved Load from TreeLoader
      • Deleted ITree
    • Moved ToggleNodeVisibility, ToggleNodeChecked from Tree to TreeBase

Additionally added an IconBadge to TreeNode and it's Render

  • Separating the style objects that convey "Active" nodes vs "Selected" nodes when the Tree has focus or not
  • Using a ScriptableObject in Selection to track the selection of a tree node
  • Using a keyboard controlId to distinguish focus on the tree vs another control in the ChangesView

Unity serialization works best when the serialized fields are all located in the derived class.

https://answers.unity.com/questions/245604/does-unity-serialization-support-inheritence.html

@StanleyGoldman StanleyGoldman force-pushed the enhancements/changes-tree-view-rollup branch from 9b0224f to 4fc10a8 Compare November 30, 2017 13:23
StanleyGoldman and others added 6 commits December 8, 2017 16:14
# Conflicts:
#	src/UnityExtension/Assets/Editor/GitHub.Unity/UI/TreeControl.cs
…hanges-view-tree-focus

# Conflicts:
#	src/UnityExtension/Assets/Editor/GitHub.Unity/UI/TreeControl.cs
…-serailized-fields

Moving serialized fields down to derived classes for best milage
@drguthals
Copy link

This this is the overall PR for this work, can you provide a couple of sentences at the top of the general gist of what it is? Just for historical purposes?

Copy link

@drguthals drguthals left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!! This look good to me!

@StanleyGoldman StanleyGoldman merged commit fa1ffde into master Dec 10, 2017
@StanleyGoldman StanleyGoldman deleted the enhancements/changes-tree-view-rollup branch December 10, 2017 21:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants