Improved Settings UI, consistent visibility and usability of SymbolBrowser, some refactoring

[domportera]

So I applied your feedback for the settings UI. It's a little bit awkward imo (i still preferred the table ;)) I made it a table this time around too with that final commit. let me know if the way i did it though works a little better, i just found it unpleasant to work with without that

also made a little settings display class to make rendering lists of settings easier

Also made some significant changes to how the SymbolBrowser organizes itself - now it should never clip off of the canvas, and it should stay in view. You can now also be confident the description panel will stay up so you can scroll and pull out examples.

Also did some refactoring for maintainability and editability

[domportera]

I dont know how this ended up here and I entirely blame visual studio

[pixtur]

I just looked at the code here on github.

Besides the definition of the settings and the async method everything looks great.
I added some comments though.

Great work with the symbol browser. But if in doubt, I would prefer smaller PRs.

[pixtur]

I'm still not convinced that mixing model and view is a good idea in this case.
But it should be easy to refactor.

One note though: I personally learned to love construction with object initializers and using parameters instead of constructors:

something like...

new Settings() {
Name: "Some name",
Tooltip: "Some tooltip",
};

much easier to read and easier to maintain.

[domportera]

im interested in your take on this, as to me this seems like a good way to enable Tooll contributors to add/remove user-facing settings with ease. i'm not deeply familiar with the model vs view concept though i think i know what you mean and share the sentiment in most cases. this case to me acts entirely as "view" - just a simple and consistent way to do so. Though with OnValueChanged it does allow that scope to creep

I like construction with object initializers too, but I chose to go this route due to the auto-generated unique labeling system (used to prevent certain imgui bugs that can be easy to run into). You'll notice that when making use of this constructor elsewhere, I essentially treat it like an object initializer, especially due to the optional parameters. Like this:

            new UIControlledSetting
            (
                label: "UI Scale",
                tooltip: "The global scale of all rendered UI in the application",
                guiFunc: (string guiLabel) => CustomComponents.DrawSingleValueEdit..... etc
            ),

I also want to make sure that anyone creating settings can't mess it up - they know they need at least two things (a label and a draw function), and if they have that it cant go wrong

[pixtur]

I personally think that this method is still too special to be added to Core.MathUtils.
Is it used in other places than the SymbolBrowser?

[domportera]

That's valid. Currently it's not, though i remember seeing another place it could have been. I can move it into the Symbol Browser if you prefer, or as an extension method for IList

[pixtur]

Nice solution. But why an out param instead of a Vector2 return value?

[domportera]

just because the description of what it is doing - if it's shifting something, a return value doesn't necessarily suggest that as intuitively as passing in a reference to something that will be shifted

[domportera]

but even by my own description that's not really any clearer and i should just be passing a ref if i want to go that route

that function is under construction per your feedback and i'll keep this in mind

[pixtur]

|| !ImGui.IsWindowHovered()

This sounds scary. Did you test this thoroughly for side effects?

[domportera]

I did test it, but i cant say i put it through the wringer. I'm not married to this idea, it just seemed strange that if my mouse is off-screen, i can press Tab and Enter and I can find out 10 minutes later I made an Execute node way off screen

[pixtur]

I know it's nitpicky, but ideally comments should be with space an upper case.
This helps me to distinguish it from commented code

Suggested change

	//if we've already started calculating this one, we can just return
	// If we've already started calculating this one, we can just return

[domportera]

Yes ive been trying to remember this, my bad!

[pixtur]

If there is nothing to say, just remove this line.

[pixtur]

Wow. It took me a while to understand what this method is doing.
Do we really need such a complex solution?

[pixtur]

I think you have to feel more comfortable with immediate user interface: The method is called every frame. So it would be MUCH easier to just store the start time and the check on every frame (I.e. on every redraw) if the time has exceeded. i'm pretty sure there is no need for async here.

[domportera]

yes it's very complex, I was wrestling with this for a while and I'm also not a huge fan of it. In retrospect this can very much be done in-line or non-async, but I liked the way the async method kept the timing logic and variables in one place. if this was that difficult for you of all people, i think it's definitely worth doing the other way

I can also remove the expiration-timer feature altogether - I implemented this in the case that you didnt want the symbol browser to keep the hovered-item description up indefinitely

[pixtur]

I did some testing with the SymbolBrowser fixes and found a couple of glitches:

We you want to append another op to an existing op, the layout is broken:

e.g.:

1. Create a [Group]
2. Now press TAB or click on output.

Also the Layout of the symbol browser input field should be horizontally aligned with the existing node. The idea behind this is, that the search field is an "operator in the making". If roughly fills the place the new op is going to have.

This is even more noticable when splitting a connection like:

I'm also not sure, if flipping the description layout is a good work around. Ideally I would prefer panning the canvas to make space or even not showing the description at all.

[domportera]

yeah sorry this pull request is so huge - i just ended up getting kind of scattershot. i'm new to this whole open-source-collaboration thing, and i'll be better about the size of my requests in the future

for your last comment, understood! i didnt take that into account but fortunately I baked in an easy way to offset it to align correctly again.

my changes are very much from the perspective of a new user, as I am one myself. personally, I always want to be able to see the descriptions, otherwise I often wont know why they're not there or if they exist at all if it's my first time using the program. But i can make that change no problem

overall it seems like my changes would require a different approach and are currently at odds with the paradigm here. Since I now know the idea is for that input field to essentailly be the next node, its positioning takes priority. so there are a couple of things that can be done to keep the symbol browser usable when at the ends of the screen

At the bottom of the screen, the symbol browser could show above the input field, so it remains in full view while the input field is in the correct position

Or, at the bottom of the screen, the symbol browser could be shorter so that you can scroll, even though your view of the browser would be significantly limited

At the right side of the screen, the symbol browser can shift to stay on screen while the input field remains where it should be

As for the description panel, I can just make it disappear on the right again

[domportera]

I adjusted a lot of this to fit your feedback and to fix some bugs I think neither of us were aware of too

to summarize:

• moved the Wrap function to a new class - CollectionUtils
  • i just think it can be useful down the line, but if you dont see any other potential use cases we can just chuck it into the SymbolBrowser and call it a day
• fixed some SymbolBrowser bugs I made 🙈
• made the SymbolBrowser text field be where it's supposed to be
• made the SymbolBrowser shrink vertically at the bottom of the screen so it's still navigable
• made the SymbolBrowser disable the description on default if there is no room, but added a setting to be able to allow it to shift to the left
  • for new users I think this behavior should be enabled by default, because with the default layout the description panel won't show up on almost the entire right half of the graph
• some minor optimizations in my SymbolBrowser logic
• no more async, much more sane. gotta stop doing this stuff at 4am 😬
• some other readability/style changes you mentioned
• made Settings tooltips work as expected

[pixtur]

Awesome, thanks a lot. Much better. I'm merging it on dev.
Some of the settings code I might adjust to a more functional ImGui style, though.

[pixtur]

Merged

[domportera]

Some of the settings code I might adjust to a more functional ImGui style, though.

looking forward to seeing your approach!

[pixtur]

It's basically a series of function calls. But the code is several hundred lines shorter.

            if (ImGui.TreeNode("User Interface"))
            {
                changed |= CustomComponents.DrawCheckboxParameter("Warn before Lib modifications",
                                                       ref UserSettings.Config.WarnBeforeLibEdit,
                                                       "This warning pops up when you attempt to enter an Operator that ships with the application.\n" +
                                                       "If unsure, this is best left checked.");
                
                changed |= CustomComponents.DrawCheckboxParameter("Use arc connections",
                                                                  ref UserSettings.Config.UseArcConnections,
                                                                  "Affects the shape of the connections between your Operators");
                

                changed |= CustomComponents.DrawCheckboxParameter("Use Jog Dial Control",
                                                                  ref UserSettings.Config.UseJogDialControl,
                                                                  "Affects the shape of the connections between your Operators");

I also cleaned up some method names in CustomComponents.cs.

[domportera]

It looks good! yeah I think I got too attached to my other solution with the checkbox-on-right approach, trying to make the settings look good regardless of what order they're in. in lieu of that, this approach is best. Sorry for overcomplicating an otherwise good task to onboard anyone else 😅

[pixtur]

Thanks again for the contribution! I believe that one of the greatest benefits of an open source project is the code ping pong. After a couple of iterations the code always gets better, so without your contribution the settings window would not only still be untouched, but there would also be no tooltips and the custom components would still be a mess.

I personally find it really hard to strike the right balance between being too careful (read: "too attached", "scared", "hesitant", "lazy") and too experimental (read: "use stuff without understanding it thoroughly", "change things because it's fun", "recklessness"). Frequently I'm too far on the "careful" side because people will complain when things break. But when too careful, we are stuck and everything fades into being outdated and obsolete.