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

Collapsible manœuvres #2664

Merged
merged 2 commits into from
Aug 5, 2020
Merged

Collapsible manœuvres #2664

merged 2 commits into from
Aug 5, 2020

Conversation

eggrobin
Copy link
Member

@eggrobin eggrobin commented Aug 4, 2020

Once we add orbit analysis for coasts, the one-line coast descriptions will reflect that analysis (something like Coast for 2 d 14 h in Earth orbit (5.2 rev.)) and will be expandable to show:

  • the number of sidereal revolutions, with ±1 buttons;
  • the number of nodal revolutions, with ±1 buttons;
  • the number of anomalistic revolutions, with ±1 buttons;
  • in the distant future, the number of parent-body-synodic revolutions, with ±1 buttons;
  • a button to open the full orbit analysis (at least for the final trajectory).

All manœuvres minimized:
screenshot19
One expanded:
screenshot20

ksp_plugin_adapter/burn_editor.cs Show resolved Hide resolved
ksp_plugin_adapter/burn_editor.cs Show resolved Hide resolved
}
UnityEngine.GUILayout.Label(
frame_info,
info,
Style.RightAligned(Style.Info(UnityEngine.GUI.skin.label)));
if (UnityEngine.GUILayout.Button("Delete", GUILayoutWidth(2))) {
Copy link
Member

Choose a reason for hiding this comment

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

It might look nicer if this button was next to +/- but I can see the point of not having Eject and Format next to each other.

Could we use 🗑 (U+1F5D1) as a label for this button? That would be clear enough.

Copy link
Member Author

Choose a reason for hiding this comment

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

The font doesn’t support that.

@@ -56,29 +56,45 @@ class BurnEditor : ScalingRenderer {
ComputeEngineCharacteristics();
}

public enum Event {
None,
Copy link
Member

Choose a reason for hiding this comment

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

None is mysterious. Unchanged?

Copy link
Member Author

Choose a reason for hiding this comment

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

It’s Event.None, which makes some sense; Event.Unchanged is weird.

Sorry, something went wrong.

bool anomalous,
double burn_final_time,
Action delete) {
double burn_final_time) {
bool changed = false;
Copy link
Member

Choose a reason for hiding this comment

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

The code would be a bit cleaner if you removed this bool and had a local variable of type Event.

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.

We need a bool for the |=. We should probably refactor this quite a bit, in particular so that it doesn’t return changed on the first repaint, but not today.

ksp_plugin_adapter/flight_planner.cs Outdated Show resolved Hide resolved
ksp_plugin_adapter/flight_planner.cs Outdated Show resolved Hide resolved
ksp_plugin_adapter/flight_planner.cs Outdated Show resolved Hide resolved
ksp_plugin_adapter/flight_planner.cs Outdated Show resolved Hide resolved
initial_time,
index,
get_burn_at_index : burn_editors_.ElementAtOrDefault);
editor.minimized = false;
Copy link
Member

Choose a reason for hiding this comment

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

Could we just say that a burn editor is created maximized? Shooting at the member variable is ugly.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is added maximized, but created minimized when loading an existing flight plan.

This is a property which is publicly write-only, not a member variable.

@pleroy pleroy added the LGTM label Aug 5, 2020
@eggrobin eggrobin merged commit 6b44f7f into mockingbirdnest:master Aug 5, 2020
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