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

Fixes #1860 - progress bar terminal resize #2076

Merged
merged 5 commits into from
Sep 6, 2017

Conversation

Dunbaratu
Copy link
Member

Fixes #1860

To test:

  • Be sure RemoteTech is installed.
  • Start new sandbox game. Choose RemoteTechConnectivityManager.
  • Use Alt-F12 'set orbit' cheat to put a comsat into Kerbin orbit
    that can act as a relay to Duna.
  • Use Alt-F12 'set orbit' cheat to put a kOS-capable probe around Duna that
    relays through the satellite you made above.
  • Keep trying print "hello". in the probe's kOS Terminal,
    after resizing the terminal to different amounts. Notice it will
    react to changes to the terminal made during the progres bar's
    wait time too, re-drawing the bar to fit the new size at the same
    ratio of completion it had before.

To test:

  - Be sure RemoteTech is installed.
  - Start new sandbox game.  Choose RemoteTechConnectivityManager.
  - Use Alt-F12 'set orbit' cheat to put a comsat into Kerbin orbit
  that can act as a relay to Duna.
  - Use Alt-F12 'set orbit' cheat to put a kOS-capable probe around Duna that
  relays through the satellite you made above.
  - Keep trying ``print "hello".`` in the probe's kOS Terminal,
  after resizing the terminal to different amounts.  Notice it will
  react to changes to the terminal made during the progres bar's
  wait time too, re-drawing the bar to fit the new size at the same
  ratio of completion it had before.
@hvacengi hvacengi self-assigned this Aug 3, 2017
Copy link
Member Author

@Dunbaratu Dunbaratu left a comment

Choose a reason for hiding this comment

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

This is why I put on the not ready flag. I need to change this first.

{
progressBarSubBuffer = new SubBuffer();
RemoveResizeNotifier(CreateProgressBarSubBuffer);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I just realized this won't work. The destructor never gets called until after you orphan this instance. This instance can't orphan until after you call RemoveResizeNotifier(), so this can never actually get called. This has to be moved elsewhere in the logic, not in the destructor.

Copy link
Member Author

Choose a reason for hiding this comment

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

Addendum: Okay, actually it probably did work all along. I put it back, with the explanatory comment about why it probably was fine all along. But even so, I still added the hook removal to SharedObjects's Destroy work just to be explicit about it. Now there's two spots that can both catch this and do the job.

@Dunbaratu Dunbaratu added the Not Ready Author is warning Reveiwer not to merge this PR yet. More edits are expected. label Aug 8, 2017
This is a better place to remove the hooks, because
the destructor (where they were) cannot fire off
while the hook itself is preventing orphaning.

I left the old location in place as well, because
I think it can still fire too and it works
as a fallback if we don't always remember to remove
those hooks.
One of my files wasn't changed on disk yet.  Now it is.
Copy link
Member Author

@Dunbaratu Dunbaratu left a comment

Choose a reason for hiding this comment

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

oops - it was probably fine after all.

{
progressBarSubBuffer = new SubBuffer();
RemoveResizeNotifier(CreateProgressBarSubBuffer);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Addendum: Okay, actually it probably did work all along. I put it back, with the explanatory comment about why it probably was fine all along. But even so, I still added the hook removal to SharedObjects's Destroy work just to be explicit about it. Now there's two spots that can both catch this and do the job.

@Dunbaratu Dunbaratu removed the Not Ready Author is warning Reveiwer not to merge this PR yet. More edits are expected. label Aug 8, 2017
@hvacengi hvacengi merged commit b654cd3 into KSP-KOS:develop Sep 6, 2017
@Dunbaratu Dunbaratu added this to the v1.1.2.0 milestone Sep 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants