Skip to content
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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

jonored
Copy link

@jonored jonored commented Apr 29, 2016

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.

jonored added 2 commits April 29, 2016 18:00
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
@jonored jonored changed the title Enhanced telnet 1596 Enhanced telnet options Apr 29, 2016
@jonored jonored changed the title Enhanced telnet options Enhanced telnet options [Not Ready] Apr 29, 2016
@hvacengi hvacengi added the Not Ready Author is warning Reveiwer not to merge this PR yet. More edits are expected. label Apr 30, 2016
}
}

private class OuterMenu : MenuLevel
Copy link
Member

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.

Copy link
Author

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.

Copy link
Member

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.

Copy link
Author

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).

Copy link
Member

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.

Copy link
Author

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Not Ready Author is warning Reveiwer not to merge this PR yet. More edits are expected.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants