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

Change the Dialog class to persist its data using a ConfigNode #2096

Merged
merged 5 commits into from
Mar 17, 2019

Conversation

pleroy
Copy link
Member

@pleroy pleroy commented Mar 17, 2019

Also centre the dialog by default.

@pleroy
Copy link
Member Author

pleroy commented Mar 17, 2019

retest this please

@eggrobin eggrobin added the LGTM label Mar 17, 2019
[KSPField(isPersistant = true)]
private int bad_installation_dialog_x_ = Dialog.XCentre;
[KSPField(isPersistant = true)]
private int bad_installation_dialog_y_ = Dialog.YCentre;
private Dialog bad_installation_dialog_ = new Dialog();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, the following are unsightly:

  • a non-null invalid Dialog;
  • a bool field equivalent to the nullness of another field.

Make the Dialogs null by default, remove the is_post_apocalyptic_ field, create the Dialogs when the bools would be set.
Take a message at construction in Dialog, so that there is no need for the "SHOW WITHOUT MESSAGE" thing.

For abstraction and readability, replace the is_post_apocalyptic_ field with a property:

private bool is_post_apocalyptic { get { return apocalypse_dialog_ != null; } }

is_bad_installation_ remains inequivalent to bad_installation_dialog != null; it can either become an auto-implemented property or remain a field.

if (is_post_apocalyptic_) {
  apocalypse_dialog_.Show();
}

becomes

apocalypse_dialog_?.Show();

Sorry, something went wrong.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, except that is_post_apocalyptic_ is really not needed anymore.

plugin_.HasEncounteredApocalypse(out revelation_);
if (apocalypse_dialog_ == null) {
String revelation = "";
if (plugin_.HasEncounteredApocalypse(out revelation)) {
Copy link
Member

@eggrobin eggrobin Mar 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (plugin_.HasEncounteredApocalypse(out string revelation)) {

@pleroy pleroy merged commit 9b3eb03 into mockingbirdnest:master Mar 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants