-
Notifications
You must be signed in to change notification settings - Fork 230
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enhanced telnet options [Not Ready] #1628
base: develop
Are you sure you want to change the base?
Conversation
Adds menus and items for: * Switching vessel to any other owned object * Reverting to launch * Reverting to VAB/hangar And stubs for: * Launching vessels * Loading/Saving games
} | ||
} | ||
|
||
private class OuterMenu : MenuLevel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please split all of the sub classes into their own files? It helps us track down and isolate the individual portions of code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can, but i'll need to do more work on TelnetWelcomeMenu to make there be a real boundary between the menu classes and the welcome menu to change them from inner classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if you just make the parent class a partial
so that you can continue the nested class definition in another file, that would be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Odd to me, but okay. First time I've really done much of anything with C#, so not entirely surprising that the conventions seem odd (Day job is C++, anything I get a choice on is Haskell or a clearer lisp).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I'd prefer to make it so they aren't nested (just sub classes) which would eliminate the use of a partial class. If it ends up staying in the same file, we can break them up when reviewing. Honestly, @Dunbaratu will be doing this review anyways, so I guess if he doesn't care about breaking up the classes it won't be a big deal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And making them not nested is what'd make it so they need to have a method or two for making the state changes on TelnetWelcomeMenu they're currently making directly (Friend classes really do strike me as ugly). There's probably utility to making that change, really, I've just been resisting making this too elaborate just to hit a few useful operations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"I need my neighbour to house-sit for me while I'm gone. But because I want to uphold some weird esoteric principle, I refuse to do that by giving only him a copy of the key. Instead I'll trust the entire world and just leave the house unlocked entirely."
I'm probably in the minority here, but I find friend classes less ugly than the alternative, which is to just open up a method to the whole public, rather than just limiting it to the classes you are in control of.
That being said, friend classes are ugly in that they are like Access Control Lists in file systems, where you have to explicitly whitelist individual users, when the cleaner solution is to have group permissions and allow individuals to be members of multiple groups at the same time. But I'm not aware of any OOP languages that took that approach to the concept.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As to the nested classes, we do actually have one or two places in the mod already where that technique is used. I'm not as opposed to it as @hvacengi is, provided good justification can be provided for why the nested class is inherently part of the private implementation details of the class its inside of, rather than something the rest of the system could make use of too.
I do find putting two classes in the same file ugly when they're both at the outermost nesting level, though. Putting them in the same file because they're nested is fine because only the outermost class is seen by the rest of the system, and the filename can match that outermost class name. But when two classes that are both at the outermost level and thus are both seen by the rest of the system, are in the same file, that makes it impossible to have every public class name match the filename the class is inside of. At most it can match for only one of the two classes. There were a lot of things about Java that were annoying but one thing I think it definitely got right is that it actually enforced this design principle in the compiler itself. The directory path layout IS the namespace layout with Java. A class is called foo.bar.baz because the compiler found it in path foo/bar/baz.java, not because you added some text saying so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To summarize, @jonored don't bother trying to break out the nested classes unless you've already worked at it. @Dunbaratu will be doing final review, and if he's good with the nesting I don't have an issue having it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. The alternative to friend classes that I would consider is "I'll make an interface here that is meaningful to expose publicly", not "I'll just make this member public without further thought". I use "I /could/ just make it a friend class" as a proxy for "this functionality is poorly decomposed and should be improved". re: non-inner classes in the same file: C# also has proper lambda expressions, and the only reasons I have outer (but still not exposed) classes in the same file in C++ are for lambdas on <C++11, and (usually empty) for things in compile-time programming.
Fixes #1596
Extends the TelnetWelcomeMenu into a simple hierarchical menu system, with sub-menus for the current CPU selection (which is the entry point, on the theory that it will still be the most likely used), switching active vessel, and reverting to launch or assembly building/hangar.
Still to do is filling in a game load option and a vessel launch option, and adding some additional help strings.