DUALITY-150 - Resource Selection Dialog #593

BobGneu · 2017-12-14T05:48:42Z

Overview

This PR Provides the requested interface addition detailed in #150, to allow a user to select a resource from a filtered list. It provides all available resources to the user in a form that looks like the following:

As can be seen in the screenshots, the header of the form changes to explain to the user the type of resource they are currently looking for. It filters from the list of resources provided by the IContentProvider interface.

There are three major parts of this change proposal:

A new form for filtered content selection
Updates to ObjectRefPropertyEditor interaction model:
- Single clicking the resource readout panel now results in highlighting the currently referenced element in the project hierarchy
- The button that was previously for highlighting now opens the filtered content form.
Updates to the ObjectRefPropertyEditor derivative elements to track the type of the editor

I took a look through the test cases and I didn't see anything in the way of UI test cases for representative test cases, but I did run our current nunit test cases and they are all passing. I am quite interested in the thoughts you all have as to how you would like to proceed with automated test cases on a change of this nature.

Manual Test Cases & Validation

I am open to further test cases. =)

TODO

Further Enhancements

We can change the double click behavior to result in selecting the currently referenced resource, but this functionality was not readily available and I wanted to make sure we followed the simple/small PR rule without too much scope creep.
Add preview of the resource to the content selection form. This also seemed to be a good addition, but the steps to implement it seemed to be beyond the obvious scope of the project.
Ignore replacing a reference with the same value as is currently referenced.

BobGneu · 2017-12-14T07:56:53Z

Added icon as follows:

documenting public method

…oise.

… is shown.

ilexp · 2017-12-14T19:41:28Z

Thanks for the PR! I have assigned the team of community contributors for a first review session and will take a look myself as well soon.

SirePi · 2017-12-15T08:03:12Z

Looks good to me. Well formatted and structured 👍

ChristianGreiner

Looks good so far!
I'm not sure if the new icon is the best choice: 🙈

BobGneu · 2017-12-15T15:41:27Z

Is there an icon you think would be more fitting?

ilexp

General note on Duality code style: Please prefer explicit type names over var. I've seen this in some places, but didn't want to flag each of them.

ilexp · 2017-12-15T17:23:40Z

Source/Editor/DualityEditor/Forms/ContentSelectionDialog.cs

@@ -0,0 +1,82 @@
+namespace Duality.Editor.Forms
+{
+	using System;


Duality Code Style: Using directives should be at the top of the file, outside of the namespace and separated via newline.

Noted. I'll update this.

Hopefully you are also aware of the subtlety in resolution when the using statements are outside of the namespace declaration.

ilexp · 2017-12-15T17:23:54Z

Source/Plugins/EditorBase/PropertyEditors/ObjectRefPropertyEditor.cs

@@ -18,9 +18,12 @@

 namespace Duality.Editor.Plugins.Base.PropertyEditors
 {
+	using Forms;


Duality Code Style: Using directives should be at the top of the file, outside of the namespace and separated via newline.

ilexp · 2017-12-15T17:31:15Z

Source/Plugins/EditorBase/PropertyEditors/ObjectRefPropertyEditor.cs

 			{
-				if (this.buttonShowPressed && this.buttonShowHovered) this.ShowReferencedContent();
+				if (this.buttonShowPressed && this.buttonShowHovered)


Now that the "show" button has been repurposed to "select", we should also rename the variables referring to it, such as buttonShowPressed and potential others.

Roger. This is where I thought refactoring out the input elements would be beneficial. For now I will focus on just addressing the comment, but ill make a note to come back to this later.

ilexp · 2017-12-15T17:33:07Z

Source/Plugins/EditorBase/PropertyEditors/ObjectRefPropertyEditor.cs

+			{
+				var tmpDataObject = new DataObject();
+
+				tmpResourceSelectionForm.SerializeToData(tmpDataObject);


I don't think we de/serialization here - maybe just expose the selection via property and access it?

I was hoping to keep this as minimal a change as possible, and given the way paste, copy, drag and drop are used I'm leery of deviating too far. Can you elaborate further on what you would like to see?

Sure! So the reason for serialization at the point where I assume you copied it from is that we're stashing things in the Windows clipboard via Ctrl+C / Ctrl+V. Since this means that data can, in some situations, travel outside of Duality, serialization is a requirement to make this work.

With the new dialog, however, there is no need for data to ever leave Duality. Just hand it over like you would normally, e.g. ask the dialog what the user selected directly. One way to go about this would be to add a SelectedResult property to the dialog class, which is retrieved where you also check for the DialogResult.

ilexp · 2017-12-15T18:15:11Z

Okay, so I did a quick code review (changes above) and also took a deeper look at behavior and implementation. Very clean PR so far, nicely minimal and to-the-point, big 👍 for that!

We still have some things ahead before merging though, here's a list of things I found:

The "select" button is displayed greyed out when the property value is null. This is a relic from its former times as a "show" button. Please adapt!
There is currently no way to select other things than Resources, e.g. Components (deriving from / implementing Type X) and GameObjects. You already prepared for that in your addition of the ReferenceType property, what's missing is just the dialog support for these cases.
Since the dialog will be used to select other things than Resources, it shouldn't be named after content ref selection - how about ObjectRefSelectionDialog to mirror the general ObjectRefPropertyEditor from which it originates?
The ExplorerListView is currently used to display items, which is a control I'm trying to phase out. Can we use a TreeViewAdv instead? Shouldn't be much more code, has a lot more degrees of freedom with customization in mind, and it's overall a lot nicer to work with. Usage examples that can serve as a basis are the CreateObjectDialog and WelcomeDialog.
There's a lot of complexity in the structure of the selection dialog, mostly due to the nested split panels. I don't think we really need them, as the same behavior can be achieved with regular WinForms anchors. I'd like to go a bit more into the direction of the CreateObjectDialog here, displayed below:
https://i.imgur.com/8jt6dJz.png
https://i.imgur.com/C74BXAJ.png
UI Design-wise, we need a bit of empty space on the edges to align with similar dialogs. Compared against the CreateObjectDialog:
https://i.imgur.com/9pKaq2E.png
https://i.imgur.com/layxmEw.png

As you will notice, some of them are high-level quality or consistency control, so we're on the right track here :) Let us know if you have questions or there's more to discuss.

(Also, thanks to @AdamsLair/duality-contributors for the first level review - feel free to stay around and join the discussion!)

BobGneu · 2017-12-16T05:23:44Z

Updated the description of the PR to reflect requested changes. Will get into them as time permits.

Have you looked into using a linter or a service like SonarQube/SonarSource for quality controls and validation of the day to day contributions?

Up and down now navigate up and down in the tree view

BobGneu · 2018-01-03T20:20:26Z

@ilexp

I am unable to reproduce the scrollbar situation in the latest build of the branch. I had already resized the dialog, but the behavior itself does not look to have been changed.

Attempted Repro Steps:

Drop 50 Game Objects on the scene
Open the ObjectRefSelectionDialog and observe the layout before resizing
Resize the form
ensure that the scrollbars are always in the appropriate position on the TreeViewAdv control

Repeat for 50 bitmaps

Repeat for 50 Components

ilexp · 2018-01-04T15:53:39Z

I am unable to reproduce the scrollbar situation in the latest build of the branch. I had already resized the dialog, but the behavior itself does not look to have been changed.

Huh, weird. Thanks for checking! I'll take a look myself in the next review stage. See if I can find a fix or workaround with the repro on my machine.

Ping me when you're ready for the code review 👍

ChristianGreiner · 2018-01-04T16:15:42Z

I think without the "Filter"-Label and the placeholder text in the textbox, it looks much "cleaner".
...like in your previous screenshot: 👌

BobGneu · 2018-01-04T16:44:12Z

Okay. I switched over to the label and text field due to the sample form @ilexp pointed to. I can switch back pretty quickly, so I'll hold off for a bit on any other feedback.

BobGneu · 2018-01-04T20:19:25Z

I think I am ready for the next review. @ilexp

ilexp · 2018-01-04T20:59:48Z

Added to my ToDo, will get back to you as soon as I manage 👍

If any of the @AdamsLair/duality-contributors want to user test the latest changes or start with the code review, go ahead. I'll join in.

BobGneu · 2018-01-04T23:00:13Z

@ilexp

What do you think about the cuetextbox vs. Text box and a label?

Barsonax

Saw some minor formatting issues but other than that looks clean. This the first review I did so if you have any suggestions feel free to say so.

Barsonax · 2018-01-05T12:17:22Z

Source/Editor/DualityEditor/Forms/ObjectRefSelectionDialog.cs

+					tmpNode.Name.ToLowerInvariant().Contains(tmpFilterValue) ||
+					tmpNode.Path.ToLowerInvariant().Contains(tmpFilterValue)
+				);
+		}


Minor thing but formatting seems to be a bit off

Replaced the spaces with tabs.

Barsonax · 2018-01-05T12:19:34Z

Source/Plugins/EditorBase/Properties/EditorBaseResCache.cs

@@ -14,6 +14,7 @@ public static class EditorBaseResCache
 	{
 		public static readonly Bitmap	DropdownSettingsBlack		= EditorBaseRes.DropdownSettingsBlack;
 		public static readonly Icon		IconEye						= EditorBaseRes.IconEye;


Formatting is a bit off here

This might actually be due to using tabs for alignment (as opposed to indentation) and GitHub using a different tab size. If it looks good in VS with a tab size of 4, that's alright.

The proper solution would be to use spaces for alignment (but not indentation), which is a style guide that I only started implementing later in the project, so this file not doing it that way is me not having updated it yet. Since it has nothing to do with this PR, I'd just make sure the tab solution works in VS / with tab size 4 and not change anything else.

Understood.

Barsonax · 2018-01-05T12:21:35Z

Source/Plugins/EditorBase/PropertyEditors/ObjectRefPropertyEditor.cs

-		protected	bool		buttonShowHovered	= false;
-		protected	bool		buttonShowPressed	= false;
+		protected	bool		buttonSelectHovered	= false;
+		protected	bool		buttonSelectPressed	= false;
 		protected	bool		panelHovered		= false;
 		protected	Point		panelDragBegin		= Point.Empty;
 		protected	Bitmap		prevImage			= null;


Formatting is a bit off here

Barsonax · 2018-01-05T12:21:59Z

Source/Plugins/EditorBase/PropertyEditors/ObjectRefPropertyEditor.cs

-		protected	bool		buttonShowHovered	= false;
-		protected	bool		buttonShowPressed	= false;
+		protected	bool		buttonSelectHovered	= false;
+		protected	bool		buttonSelectPressed	= false;
 		protected	bool		panelHovered		= false;
 		protected	Point		panelDragBegin		= Point.Empty;
 		protected	Bitmap		prevImage			= null;


Formatting is a bit off here

See tab alignment comment above.

ilexp · 2018-01-05T18:12:11Z

What do you think about the cuetextbox vs. Text box and a label?

I don't have a strong opinion either way, maybe slight favor towards the cue text one, if any.

ilexp

Code looks good overall! Some change requests and suggestions on the code style side. I think we're good to go after addressing them, ping me when you're ready.

ilexp · 2018-01-06T08:06:51Z

Source/Editor/DualityEditor/Forms/ObjectRefSelectionDialog.cs

+
+				this.ComponentReference = component;
+			}
+


Code Style:

Fields should be next to fields, properties should be next to properties and methods should be next to methods. Do not mix them if it can be avoided within reason.

Keep statics at the top, instance methods below.

If the number of members of the same type grows beyond what can be easily observed with a full overview, try to group semantically where it makes sense.

Use two newlines to separate different member types, one newline to separate semantic groups within the same type, no newline between items of the same semantic group and type.

Example:

public class Foo { private int field1; private int field2; private int field3; public int Property1 { get { /* ... */ } set { /* ... */ } } public int Property2 { get { /* ... */ } set { /* ... */ } } public int Property3 { get { /* ... */ } set { /* ... */ } } public Foo() { // ... } public void Method1() { // ... } public void Method2() { // ... } public void Method3() { // ... } }

ilexp · 2018-01-06T08:07:34Z

Source/Editor/DualityEditor/Forms/ObjectRefSelectionDialog.cs

+				this.ComponentReference = component;
+			}
+
+			private string _name;


Code Style: Do not use underscore prefixes for fields or parameters.

ilexp · 2018-01-06T08:09:59Z

Source/Editor/DualityEditor/Forms/ObjectRefSelectionDialog.cs

+			this.objectReferenceListing.BeginUpdate();
+			this.Model.Nodes.Clear();
+
+			if (this.FilteredType.IsSubclassOf(typeof(GameObject)) || this.FilteredType == typeof(GameObject))


Suggestion: Use typeof(GameObject).IsAssignableFrom(this.FilteredType) to wrap this up in a single check. Also covers cases where you'd have an interface type instead of GameObject.

ilexp · 2018-01-06T08:18:19Z

Source/Editor/DualityEditor/Forms/ObjectRefSelectionDialog.cs

+				{
+					ReferenceNode tmpNode = new ReferenceNode(currentObject);
+
+					this.Model.Nodes.Add(tmpNode);


Suggestion: The newline between the two lines of the inner loop doesn't seem to be necessary. Keep it if you want (not a change request), but as a general rule of thumb, I try to make every newline express something meaningful and avoid those where it's not clear what exactly they're separating, like logical code blocks. Here, instantiating the node and adding the node each are so short and self-documenting that they might as well be regarded as a single logical unit, in which case they don't really need the newline.

ilexp · 2018-01-06T08:22:41Z

Source/Editor/DualityEditor/Forms/ObjectRefSelectionDialog.cs

+
+				foreach (Component currentComponent in Scene.Current.FindComponents(this.FilteredType))
+				{
+					ReferenceNode tmpNode = new ReferenceNode(currentComponent);


Suggestion: This is also true for other places in code, but why is this variable prefixed with a tmp? It isn't really any more temporary than any other local variable, so might as well skip the prefix and give it a regular name. node would be just fine here as long as the context is clear and there's no ambiguity in scope.

ilexp · 2018-01-06T08:27:19Z

Source/Plugins/EditorBase/Properties/EditorBaseResCache.cs

@@ -14,6 +14,7 @@ public static class EditorBaseResCache
 	{
 		public static readonly Bitmap	DropdownSettingsBlack		= EditorBaseRes.DropdownSettingsBlack;
 		public static readonly Icon		IconEye						= EditorBaseRes.IconEye;


This might actually be due to using tabs for alignment (as opposed to indentation) and GitHub using a different tab size. If it looks good in VS with a tab size of 4, that's alright.

The proper solution would be to use spaces for alignment (but not indentation), which is a style guide that I only started implementing later in the project, so this file not doing it that way is me not having updated it yet. Since it has nothing to do with this PR, I'd just make sure the tab solution works in VS / with tab size 4 and not change anything else.

ilexp · 2018-01-06T08:28:40Z

Source/Plugins/EditorBase/PropertyEditors/ObjectRefPropertyEditor.cs

-		protected	bool		buttonShowHovered	= false;
-		protected	bool		buttonShowPressed	= false;
+		protected	bool		buttonSelectHovered	= false;
+		protected	bool		buttonSelectPressed	= false;
 		protected	bool		panelHovered		= false;
 		protected	Point		panelDragBegin		= Point.Empty;
 		protected	Bitmap		prevImage			= null;


See tab alignment comment above.

ilexp · 2018-01-06T08:29:19Z

Source/Plugins/EditorBase/PropertyEditors/ObjectRefPropertyEditor.cs

+					if (tmpPropertyInfo != null)
+					{
+						t = tmpPropertyInfo.PropertyType;
+					} else if (tmpFieldInfo != null) {


Code Style: This should be

} else if (...) {

ilexp · 2018-01-06T08:29:50Z

Source/Plugins/EditorBase/PropertyEditors/ObjectRefPropertyEditor.cs

@@ -201,7 +230,7 @@ protected override void OnPaint(PaintEventArgs e)
 				rectText,
 				format);

-			ControlRenderer.DrawBorder(e.Graphics, 
+			this.ControlRenderer.DrawBorder(e.Graphics, 


Good call adding the missing this. 👍

ilexp · 2018-01-06T08:32:04Z

Source/Editor/DualityEditor/Forms/ObjectRefSelectionDialog.cs

+				}
+			}
+
+			this.objectReferenceListing.NodeFilter += this.NodeFilter;


Why is this event subscription separate from the others in the constructor?

BobGneu · 2018-01-07T03:05:01Z

Ill be going through the notes as soon as my munchkins are down.

Thanks for the feedback @ilexp && @Barsonax

…onentReference to AutoProperties.

BobGneu · 2018-01-07T04:59:27Z

I think I have addressed the formatting issues and code quality issues raised in the earlier comments.
I have also switched back over to the CueTextBox.

I think we are good to go. @ilexp

ilexp · 2018-01-07T12:10:31Z

Sounds great, code looks good as well. Thank you for your work on this! I think I can take over from here with my side of the ToDo:

Investigate and fix the window open layout bug that seems to be reproducible on my machine, but not elsewhere so far.
Do a quick, final functionality test.
Merge the PR into master.
Trigger a binary release.
Create a new issue for the outlook items that weren't pulled into this PR:
- Adding icons for nodes.
- When opening the dialog, focus on the search field for direct keyboard input, auto-select the topmost list item and close the dialog on return, so it's 100% usable via keyboard.
- I believe you did this already: Open dialog via return key on property editor

I'll post here with my progress on this.

#FIX: Fixed a bug where entering a non-existent filter value would spawn an exception in the selection code.

#CHANGE: Adjusted column header background color using a header draw callback.

#CHANGE: Components now display the full path of their GameObjects in the Path column, and a combo of short name and type name in the Name column.

ilexp · 2018-01-07T13:46:15Z

Added some final touches to the code, merged to master and triggered a binary release. The new dialog will be out there in about 15 minutes.

I'll close issue #150 and proceed to create one for the remaining optional items.

ilexp · 2018-01-07T14:09:56Z

Created new issues for the remaining items.

We're done here. Thanks! 😃

BobGneu added 9 commits December 13, 2017 09:44

Adding reference type property

4003e26

Adding type to derivative types

822db3b

adjusting highlight behavior, moving it to single click behavior.

2eb8ede

Adding content selection dialog form

90fef96

Adding layout for content selection dialog

9fdcf82

Updating current value

9ad3080

Following standard of source providing data envelope

d888bb0

Saving interface to provide data

c415412

Adding double click behavior

76a0a4f

BobGneu mentioned this pull request Dec 14, 2017

Add a "Select Object" Button next to Reference Editors #150

Closed

Switching to application_side_expand.png for the icon.

b0a23f5

BobGneu added 5 commits December 14, 2017 00:01

Removing unused imports

60b5e12

documenting public method

Comment on public property

37f1f7f

Moving hooked event to proper location instead of creating a ton of n…

0b0932b

…oise.

Correcting tab indices and focusing on the tile display when the form…

451ad17

… is shown.

Removing log messages

814f9ac

ilexp requested a review from a team December 14, 2017 19:39

ilexp added Editor Area: Duality editor or support libraries Feature It doesn't exist yet, but I want it Nice2Have Beneficial, but only very slightly so Usability Related to API and UI usability labels Dec 14, 2017

ilexp added this to the General milestone Dec 14, 2017

ChristianGreiner reviewed Dec 15, 2017

View reviewed changes

ilexp requested changes Dec 15, 2017

View reviewed changes

Switching to selection changed

0959bb1

Up and down now navigate up and down in the tree view

Barsonax requested changes Jan 5, 2018

View reviewed changes

ilexp requested changes Jan 6, 2018

View reviewed changes

BobGneu added 9 commits January 6, 2018 20:23

Replacing spaces with tab for formatting.

bb9248a

Switched back to cuetextbox

053a38e

Converted Name, Path, ResourceReference, GameObjectReference and Comp…

99678fd

…onentReference to AutoProperties.

Switched to isAssignableFrom

f6e997c

dropping tmp prefix on local variable names

cf113b5

formatting

3d1c541

formatting

ef4b916

Relocating NodeFilter subscription

c54bbb0

Trapping empty list filter attempt

cbdaadb

ilexp approved these changes Jan 7, 2018

View reviewed changes

ilexp added 3 commits January 7, 2018 14:24

ObjectRef Selection Dialog Bugfix

1200b83

#FIX: Fixed a bug where entering a non-existent filter value would spawn an exception in the selection code.

Column Header Background Color

4fb72ef

#CHANGE: Adjusted column header background color using a header draw callback.

Component Path Display

81d98b2

#CHANGE: Components now display the full path of their GameObjects in the Path column, and a combo of short name and type name in the Name column.

ilexp merged commit bf11b15 into AdamsLair:master Jan 7, 2018

DUALITY-150 - Resource Selection Dialog #593

DUALITY-150 - Resource Selection Dialog #593

Conversation

BobGneu commented Dec 14, 2017 • edited Loading

Overview

Manual Test Cases & Validation

TODO

Further Enhancements

BobGneu commented Dec 14, 2017

ilexp commented Dec 14, 2017

SirePi commented Dec 15, 2017

ChristianGreiner left a comment

Choose a reason for hiding this comment

BobGneu commented Dec 15, 2017 • edited Loading

ilexp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilexp commented Dec 15, 2017

BobGneu commented Dec 16, 2017 • edited Loading

BobGneu commented Jan 3, 2018 • edited Loading

Attempted Repro Steps:

Repeat for 50 bitmaps

Repeat for 50 Components

ilexp commented Jan 4, 2018

ChristianGreiner commented Jan 4, 2018

BobGneu commented Jan 4, 2018

BobGneu commented Jan 4, 2018

ilexp commented Jan 4, 2018

BobGneu commented Jan 4, 2018

Barsonax left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilexp commented Jan 5, 2018

ilexp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BobGneu commented Jan 7, 2018

BobGneu commented Jan 7, 2018

ilexp commented Jan 7, 2018 • edited Loading

ilexp commented Jan 7, 2018

ilexp commented Jan 7, 2018

BobGneu commented Dec 14, 2017 •

edited

Loading

BobGneu commented Dec 15, 2017 •

edited

Loading

BobGneu commented Dec 16, 2017 •

edited

Loading

BobGneu commented Jan 3, 2018 •

edited

Loading

Barsonax left a comment •

edited

Loading

ilexp commented Jan 7, 2018 •

edited

Loading