A GUI toolkit. #1878

WazWaz · 2016-11-23T02:35:47Z

I'd like a lot more input from others before I'd even want this in kOS of course, but this is presented here for discussion.

Dunbaratu · 2016-11-23T04:39:08Z

While this is cool, I'm not sure how I feel about leaving the telnet control option out in the cold like this.
As long as the gui is nothing more than fields, checkboxes, buttons, and sliders, it should be possible to make a "text-ified" variant of it that works on the terminal window too, and abstract away the difference for people. (So you can just select if you want the text or the graphical version of things when invoking the gui).

But I'm not sure how much it matters. I don't really see a lot of people using the telnet option much. Up until now we haven't introduced anything that makes a big difference. I'm a bit torn on whether or not introducing this is good. It's a very cool option I've been wanting for a while, but I've always entertained the notion that it could be do-able entirely on the terminal window itself.

WazWaz · 2016-11-23T06:37:22Z

I was considering the terminal approach, but it takes up a lot of screen realestate (especially with all the settings gadgets on it now). In contrast, having a boot program that starts up a simple custom GUI means kOS users could even develop entire "mods within a mod" for non-kOS users (hmm... ROM modules in Parts.......).

As for Telnet, I see these users as an entirely different breed from those who would want to use/write a GUI. How horrible would Windows have been if it had a TTY renderer. I think ultimately people will write useful functions (like "launch to orbit" and "execute node" and "land" that I'm sure we've all written), then bind them to a GUI while still being easily able to execute them from the command line, in the same way that MacOSX is Unix underneath.

One thing I forgot to mention is that I had no luck working out how ScopeLost works. It almost never seemed to be called. So I settled for the idea that programs really should manually call Hide() on the window and when power is lost that windows close themselves.

Dunbaratu · 2016-11-23T06:56:29Z

What I mean is that "here's a list - pick something from it", and "here's a checkbox you could select" are concepts that are not gui-specific in the slightest. People have coded TUI toolkits on top of things like curses that do all that. But that sort of thing is sparse and hard to find in the C# world.

But what using the existing gui toolkit buys us is that we don't have to code all the stuff to deal with the user input and updating the visual look based on what the user did, and all that jazz. The fact that it's on a GUI isn't much of an advantage. The fact that if we use the Unity GUI system then the work has been done already by other people, however, is, so I might begrudgingly accept doing it that way even though it helps drive the artificial wedge between interactive and command line usage by making it possible to write scripts that cannot be used from a remote connection.

WazWaz · 2016-11-23T09:15:22Z

Note that while it uses the older Unity IMGUI, it's presented as object-based so should in theory port easily to UGUI should the need arise. Ideally, that abstraction should be kept however it goes forward.

Fix up documentation (now actually tested to go through sphinx).

Split PADDING into HPADDING and VPADDING Add ADDHLAYOUT and ADDVLAYOUT - like HBOX/VBOX, but default to entirely no style or borders, just pure layout.Honour scoping so that UIs can be usefully made with locals, not just globals. Optimize styles to avoid creating new ones until actually changed. Make LABEL and SLIDER honour HSTRETCH being set to false (and it will now default to false too on LABEL) Tweak initial hslider style.

Dunbaratu · 2016-11-25T20:45:17Z

I suppose I shouldn't be the only one deciding things. I'm just one person and kOS is a team effort that nobody really singly owns. So since it seems the GUI idea is gaining traction it's better to include it rather than keep it as a messy separate fork. (I admit that by adding sound I moved things a bit away from the "remote text allows complete control" direction myself, but sound was a gui-only output thing so it wasn't quite the same situation as having a gui-only input method would be.)

In the end, really to be honest, the problem is only a problem because I've been dragging my feet on the text facilities so they're not up to snuff to compete against the easier to implement GUI stuff (easier because, again, the toolkit is right there ready to be used in Unity and not something we'd have to make from scratch.) I mean, I never even implemented a "type a line of text" input to a program yet, and that's really quite basic.

So yeah, I shouldn't be objecting to it. It's only a problem of my own making by dragging my feet on implementing the textual alternative. That's no reason to nay-say someone else getting the task done quicker and simpler and (let's be honest) in a way most users probably would prefer anyway.

I would at least like to maybe put in a little effort on trying to make the UI look a bit more KOS-like, but that can be done later by manipulating the UI skin settings. It's not a reason to block progress.

Dunbaratu · 2016-11-25T22:24:49Z

Incidentally, @WazWaz : Can you add an issue to associate with this Pull Request? Our preferred organizational way to deal with major feature enhancements like this is to separate the implementation (PR) from the high level feature description (Issue). Even in cases like this where you already implemented it, it's still nice to submit both, to have the pairing to fit together.

(It also helps because it gives a good description of "This is what it needs to do" that's separate from "This just happens to be what I did to get the job done." It makes it more clear what sorts of changes would be more of a deal-breaker for you.)

WazWaz · 2016-11-26T00:09:08Z

I'd like to leave it in a branch for a little longer as I'm still discovering "deep" changes (like the scope thing). When I think it's at least at "beta", that would be a good time to pull to main.

Issue: will do.

Dunbaratu · 2016-11-26T00:18:30Z

Fair enough. Let me tag this PR with the label "Not Ready" then, to remind us not to merge it.

I don't know if you have the permissions to alter labels. If you don't then let us know when you do consider it ready and we can remove the "Not Ready" label.

Revert scope-based widget deletion. Add explicit widget disposal (removal from parent). Make top-level window auto-resize. Fix blank transparent space window margins.

Add "CHANGED" attribute to PopupMenu (like TextField). Make it easy to use objects other than strings in a PopupMenu.

WazWaz · 2016-12-01T00:29:42Z

The "Not Ready" can come off. I believe this is now good for release. Of course, I'm happy to be told "do X, Y, and Z first" (eg. I've made sure it's documented, but does it need more examples, tests, or anything?).

I'm assuming the big red crosses above are because things unrelated to this branch are causing test failures.

Dunbaratu · 2016-12-01T01:17:28Z

doc/source/structures/misc/gui.rst

+    HOVER         The widget is under the mouse.
+    ON            The widget is on (eg. button is PRESSED).
+    ============= =====================================================
+


I started reading through this today a bit and this part is very confusing. You say there are 8 variants of the image but there's only a capacity to select one file mentioned, the BG file. Is the BG_FOCUSED, BG_ACTIVE, etc all just procedurally generated from the one file?

I'm guessing there's some files missing (i.e. the git add command wasn't run for them). I only see one PNG file for the button, and I don't see any of these suffixes corresponding to any AddSuffix calls within the C# code (that C# file is probably missing from the PR).

BG_* - It's just trying to shorthand the documentation. There are 8 attributes, but it looked cluttered and confusing to list them all in the normal section, so I put it in a note where they could be properly explained.

I'm not sure which PNG files you mean. There should be some in the documentation, and 3 in the GFX directory:

commDelay - the "busy" icon

popupmenu_bg_hover - the yellow color seen on PopupMenus.

radiobutton - wrong name I just realized, it's the icon for the left side of PopupMenus.

@Dunbaratu check here: https://github.com/KSP-KOS/KOS/pull/1878/files#diff-4ac6900d9949c6bfbb7c3bdd9b5b3ec1R93

@WazWaz I get the idea that the various BG suffixes getting confusing, but every suffix really should be documented directly for the sake of consistency and clarity (the few we don't document now eventually need to get updated). If the group of suffixes is confusing, maybe it would make more sense to create some kind of a "background configuration" structure, so that the syntax might get cleaned up like this:

set widget:bgconfig:focused to "myFocused.png"

If there are 8 different suffixes, this was not clear from the docs. (Remember that the C# code that implements what this document is talking about is missing from your PR (I saw no calls to AddSuffix within the diff output listed on the PR page, which probably means the C# file that implements them wasn't git add-ed to the branch yet. So I only see the docs and not the implementation. (Which is a pretty good test of how understandable the docs are, as that's the situation we should expect a typical user of kOS to be in).

As to the URL you linked, It looks like you have added more to the PR than was there when I looked at it a few hours ago. That file was definitely missing before. I'll go have a look now.

They're in src/kOS/Suffixed/Widgets.cs in the original PR (src/kOS/Suffixed/Widget/Button.cs now) - note that github elides it because it's a long file, so CTRL-F won't find it.

Add SKIN to GUI allowing single-point-of-change in styles, and changing of sub-part styles.

WazWaz · 2016-12-01T23:49:38Z

I reconsidered what @hvacengi said about a "bg config" structure. I still didn't do that, but moving all the style attributes to their own structure cleans up the structs (and docs) nicely, and the concept of a SKIN comes for free.

I'm unsure of the namespace consequences of these GUI elements. I think they should affect users (because only functions should have that problem, of which there is only GUI(width)), and the C# class is "WidgetSkin", not just "Skin", but does the kOS struct name (Skin) have any consequence?

… while up). Allow BORDER, etc. to be completely specified (left, right, top, bottom), not just H and V.

Add "VISIBLE" for reading HIDE/SHOW state. Tweak radiobuttons to work reliably over comm delay.

Add SELECTIONCOLOR attribute to SKIN. USE configured skin (for other settings like font, etc.)

hvacengi · 2016-12-10T22:09:36Z

Looks like your most recent commits are missing the changes to kOS.csproj to account for WidgetStyle.cs and WidgetSkin.cs

…el." I accidentally committed my local install changes.

WazWaz · 2016-12-10T22:40:43Z

I committed them in ffa8408, but accidentally also committed my local "install to KSP" changes, which I've now reverted.

… copying the whole GUIStyles too, unnecessarily in this case. Save and restore GUISkin around GUI operation (because other GUI might not set anything, eg. dev console).

# Conflicts: # src/kOS/SharedObjects.cs

Dunbaratu · 2017-01-27T19:19:14Z

@WazWaz - can you remove the "Not Ready" label on this PR to state officially that you're okay with merging it now? (Do you have the privileges to change labels in this github? If not then just post a comment telling us to remove the label.)

Also, is this up to date?
In the slack channel, you said yesterday "I've merged and pushed. Just one minor conflict in SharedObjects" - but in this PR I don't see it claiming anything has changed since Dec 13th. Are you sure your push made it all the way to here?

hvacengi · 2017-01-27T19:41:11Z

Last commit was yesterday: 5e05b1e

It looks like he's a member of the "contrib" team, which only has read access to the repository. I assume that applies to labels and such as well.

Dunbaratu · 2017-01-27T19:43:46Z

In that case, github's presentation is totally bogus.

It's indented thusly:

WazWaz added some commits on Dec 13, 2016
- @WazWaz Stop sphinxbuild help spamming stdout on version test. 172b020
- @WazWaz Use Object.Instantiate to copy Skin. Util function is suspect, and is 42839fb
- @WazWaz Merge branch 'develop' of https://github.com/KSP-KOS/KOS into gui 5e05b1e

Implying all three of those commits are underneath the heading "some commits on Dec 13th". If they're not all part of that, they shouldn't be indented that way.

WazWaz · 2017-01-27T23:10:52Z

The merge commit from yesterday appear to be included in "Commits" on this PR, but no, I can't edit labels.

But I'll say here I believe this is Ready.

Dunbaratu

Some things I noticed recently on my second read-through of this code. (I still intend to approve the PR - these are just notes and questions for comment before I do so.)

Dunbaratu · 2017-01-29T22:05:09Z

doc/source/tutorials/gui.rst

+The AddTabWidget function should however accept any VBOX as the parameter::
+
+    LOCAL gui TO GUI(500).
+    LOCAL tabwidget TO AddTabWidget(gui).


Note To self: Just to make it scan better, change the "TO" to "IS" in this and all the other places where the documentation says LOCAL ___ TO ____.

Dunbaratu · 2017-01-29T22:59:40Z

src/kOS/Screen/GUIWindow.cs

+            }
+            catch(NullReferenceException)
+            {
+            }


Should there be a return; in this catch body?

What is the expected situation where you'd still want to execute the rest of OnGUI() after finding that there was a null in the above code block somewhere? I know that in the case of kOS.Safe.Utilities.SafeHouse.Config if that's not there then most of kOS is going to go wrong and crash anyway. It needs to get set up before the rest of kOS can work. The only thing I don't know about is FlightResultsDialog -is it normal for that to remain null for a long time during KSP's run? Or is it something that just is null for a short moment during scene changes or reloads?

It looks like this was copied from TermWindow.cs, and was part of the initial commit by Nivekk. That being said, it would probably be worthwhile to look into this behavior. It may not be an issue at all anymore, or it may be possible to detect the null reference without throwing an exception (which is expensive, and particularly an issue inside of GUI update methods). At the very least, catching the excepting should log a message of some kind (I'd prefer an error message) so that we can find out if this exception is ever being thrown. I've checked and it looks like internally FlightResultDialog.isDisplaying includes a null check for the private Instance field.

Yes, this was from TermWindow.

Dunbaratu · 2017-02-01T23:31:09Z

src/kOS/Screen/GUIWindow.cs

+        }
+
+        public PopupMenu currentPopup;
+


Do we have an official standard that says all the member fields and properties are declared up at the top rather than in-line mixed up with the methods? I don't know if we officially state it, but it seems to be the pattern we usually follow. I've noticed in several places in this pull request where they occur sandwiched between methods, usually just before the first method that happens to use the field.

While not "gospel", the referenence guidlines state:

Classes member should be alphabetized, and grouped into sections (Fields, Constructors, Properties, Events, Methods, Private interface implementations, Nested types)

I'm not one to get too hung up on it, cause it isn't a huge issue (particularly the alphabetizing). But if most of the fields/properties are declared at the top, it does make it easier to read the file if that location is consistent. I would probably move them if I were doing the review.

Dunbaratu · 2017-02-01T23:34:34Z

src/kOS/SharedObjects.cs

@@ -12,15 +12,27 @@ public class SharedObjects : Safe.SafeSharedObjects
        public ProcessorManager ProcessorMgr { get; set; }
        public Part KSPPart { get; set; }
        public TermWindow Window { get; set; }
+        public List<KOSManagedWindow> ManagedWindows { get; }


Was it the intention that we move all KOSManagedWindows into this list, and manage all of them with the same technique? If so then there are also changes that would have to occur in the popup text editor too, where it uses KOSManagedWindow objects.

If, on the other hand, it was not the intention to move all KOSManagedWindows into this system, and instead we only want to have some of them here, then I'd like to see the name of this changed. The rather generic all-encompassing name ManagedWindows implies this is where you should be able to find all the Managed Windows, rather than just a subset of them.

The TermWindow would also technically qualify. This list should also be cleared, and the contained windows destroyed with UnityEngine.Object.Destroy, in the DestroyObjects method, unless they have no reference to SharedObjets to create a circular reference.

Ideally yes, but I didn't want to tread on the code that references SharedObjects.Window. That change should be an Issue.

I'm compiling a list of all these "should be addressed, but shouldn't prevent the GUI from being merged" kinds of issues and I added them to the issues list. This is one of them, here: #1952

Dunbaratu · 2017-02-03T00:09:40Z

doc/source/structures/misc/gui.rst

+    :attr:`VERTICALSLIDERTHUMB`            :struct:`Style`             Style for the thumb of vertical :struct:`Slider` widgets.
+    :attr:`LABEL`                          :struct:`Style`             Style for :struct:`Label` widgets.
+    :attr:`SCROLLVIEW`                     :struct:`Style`             Style for :struct:`ScrollBox` widgets.
+    :attr:`TEXTFIELD`                      :struct:`Style`             Style for :struct:`TextField widgets.


:struct:`Textfield widgets.

This is missing a terminating backtick delimiter. (This showed up in github's diff view because it rendered the rest of this document underlined.)

Dunbaratu · 2017-02-03T01:53:59Z

doc/source/structures/misc/gui.rst

+        LOCAL gui TO GUI(200).
+        // Add widgets to the GUI
+        LOCAL label TO gui:ADDLABEL("Hello world!").
+        SET label:ALIGN TO "CENTER".


The example doesn't match what's in the mod. Label objects have no such suffix "ALIGN".

Dunbaratu · 2017-02-03T03:16:23Z

doc/source/structures/misc/gui.rst

+    Example::
+
+	local popup to gui:addpopupmenu().
+	set popup:OPTIONSUFFIX to "NAME".


I had to read through the C# code several times to finally understand what OPTIONSUFFIX is for. It was really vague and unclear from this documentation. This part of the documentation should explain it a bit better.

Something like this will make more sense:

"The list always displays string values in it. If the list contains items that are not strings, then by default their TOSTRING (link to generic Structure:ToSTRING should go here) suffixes will be used to display them as strings.

You can change this default behaviour by setting the popupmenu's :OPTIONSUFFIX to a different suffix name other than "TOSTRING". In the example below which builds a list of bodies for the pulldown list, the body:name suffix will be used instead of the body:tostring suffix for all the items in the list."

Explicitly mentioning how this setting is an alternative to the default behavior of TOSTRING helps make it more clear what this suffix is doing and why it exists.

Dunbaratu · 2017-02-03T03:27:46Z

doc/source/structures/misc/gui.rst

+                   Every suffix of :struct:`BUTTON`
+    -----------------------------------------------------------------------------------
+    :attr:`OPTIONS`                       :struct:`List`(Any)             List of options to display.
+    :attr:`OPTIONSUFFIX`                  :struct:`string`                Name of the suffix that names the options.


I suggest the terse description here again mention that this is in lieu of using TOSTRING:

"Name of the suffix used for display names. Default = TOSTRING".

WazWaz added 5 commits September 26, 2016 08:59

Merge branch 'develop' of https://github.com/hvacengi/KOS into develop

012590d

Merge branch 'develop' of https://github.com/hvacengi/KOS into develop

d10c30d

Merge branch 'develop' of https://github.com/KSP-KOS/KOS into develop

c6d1170

Add a GUI toolkit.

d9644a0

index on develop: d9644a0 Add a GUI toolkit.

5f73b63

WazWaz added 4 commits November 24, 2016 11:26

Add "ENABLED" attribute to Widget classes.

19beab7

Fix up documentation (now actually tested to go through sphinx).

Add a checkbox style of button.

e747930

Allow 9-slice borders to be specified.

ca3cd5c

WazWaz mentioned this pull request Nov 26, 2016

Feature: GUI Toolkit #1881

Open

Dunbaratu added enhancement Something new (not a bug fix) is being requested Not Ready Author is warning Reveiwer not to merge this PR yet. More edits are expected. labels Nov 26, 2016

WazWaz added 7 commits November 26, 2016 14:52

Add RADIOBUTTON, SCROLLBOX and POPUPMENU widgets.

61b974e

Revert scope-based widget deletion. Add explicit widget disposal (removal from parent). Make top-level window auto-resize. Fix blank transparent space window margins.

Make height optional in GUI(width,height) function.

0d3b2bd

Fix a possible out-of-range error.

4164a60

Add connectivity and signal delay support to GUI.

f43f0d5

Add "CHANGED" attribute to PopupMenu (like TextField). Make it easy to use objects other than strings in a PopupMenu.

doc

9a2295a

Fix Button:PRESSED breakage in alpha4.

b1d2544

Merge branch 'develop' of https://github.com/KSP-KOS/KOS into gui

7cce2c9

Avoid unnecessary GUISkin copying.

f4f77b2

Dunbaratu reviewed Dec 1, 2016

View reviewed changes

Move style attributes to a class of their own.

168fc27

Add SKIN to GUI allowing single-point-of-change in styles, and changing of sub-part styles.

WazWaz added 6 commits December 2, 2016 11:00

doc fixes

a077f5e

Allow TEXTCOLOR for all widget states (eg. black while pressed, white…

4d07213

… while up). Allow BORDER, etc. to be completely specified (left, right, top, bottom), not just H and V.

Add TabWidget tutorial to doc.

ae10add

Add "VISIBLE" for reading HIDE/SHOW state. Tweak radiobuttons to work reliably over comm delay.

Fix images in GUI tutorial.

53291da

A suggestion for making custom skins.

c3016d5

Add FONT attribute to SKIN and STYLE.

b840c3e

Add SELECTIONCOLOR attribute to SKIN. USE configured skin (for other settings like font, etc.)

WazWaz force-pushed the gui branch from ceffd8f to b840c3e Compare December 10, 2016 06:28

WazWaz added 2 commits December 10, 2016 16:36

Merge branch 'master' of https://github.com/KSP-KOS/KOS into gui

59b14ae

Go back to having HSTRETCH default to true for Label.

ffa8408

Partial Revert of "Go back to having HSTRETCH default to true for Lab…

f89ae98

…el." I accidentally committed my local install changes.

WazWaz added 3 commits December 13, 2016 16:52

Stop sphinxbuild help spamming stdout on version test.

172b020

Use Object.Instantiate to copy Skin. Util function is suspect, and is…

42839fb

… copying the whole GUIStyles too, unnecessarily in this case. Save and restore GUISkin around GUI operation (because other GUI might not set anything, eg. dev console).

Merge branch 'develop' of https://github.com/KSP-KOS/KOS into gui

5e05b1e

# Conflicts: # src/kOS/SharedObjects.cs

Dunbaratu removed the Not Ready Author is warning Reveiwer not to merge this PR yet. More edits are expected. label Jan 28, 2017

Dunbaratu reviewed Feb 7, 2017

View reviewed changes

Doc and style improvements.

24b725a

Dunbaratu added this to the v1.1.0 milestone Feb 10, 2017

Dunbaratu merged commit 24b725a into KSP-KOS:develop Feb 14, 2017

A GUI toolkit. #1878

A GUI toolkit. #1878

Conversation

WazWaz commented Nov 23, 2016

Dunbaratu commented Nov 23, 2016

WazWaz commented Nov 23, 2016

Dunbaratu commented Nov 23, 2016

WazWaz commented Nov 23, 2016

Dunbaratu commented Nov 25, 2016

Dunbaratu commented Nov 25, 2016 • edited Loading

WazWaz commented Nov 26, 2016

Dunbaratu commented Nov 26, 2016

WazWaz commented Dec 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WazWaz Dec 1, 2016 • 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

WazWaz commented Dec 1, 2016

hvacengi commented Dec 10, 2016 • edited Loading

WazWaz commented Dec 10, 2016

Dunbaratu commented Jan 27, 2017 • edited Loading

hvacengi commented Jan 27, 2017

Dunbaratu commented Jan 27, 2017

WazWaz commented Jan 27, 2017

Dunbaratu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hvacengi Feb 7, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dunbaratu commented Nov 25, 2016 •

edited

Loading

WazWaz Dec 1, 2016 •

edited

Loading

hvacengi commented Dec 10, 2016 •

edited

Loading

Dunbaratu commented Jan 27, 2017 •

edited

Loading

hvacengi Feb 7, 2017 •

edited

Loading