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

Only restore expansion state when there are items. #36539

Merged
merged 1 commit into from Oct 26, 2017

Conversation

Projects
None yet
3 participants
@guw
Contributor

guw commented Oct 19, 2017

This modifies the change for #16031 to prevent the tree from collapsing
in case a debugger needs more time providing content.

@mjbvz mjbvz assigned sandy081 and isidorn and unassigned sandy081 Oct 19, 2017

@isidorn isidorn added this to the October 2017 milestone Oct 20, 2017

@isidorn isidorn added the debug label Oct 20, 2017

@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Oct 20, 2017

Contributor

@guw thanks for your pr. I have looked into it and here is some feedback:

  • I do not see what is the benefit of introducing a global variable this.expandedElements in your code, you can do that all with a local
  • The only functionaly difference compared to the original code is that the if (stackFrame) is moved a couple of lines up, which looks great to me
  • Please format the code as the indentation is completly broken
  • Does this actually fix the issue on your side? If yes I would gladly merge it in once you address my feedback from above.

Thanks again

Contributor

isidorn commented Oct 20, 2017

@guw thanks for your pr. I have looked into it and here is some feedback:

  • I do not see what is the benefit of introducing a global variable this.expandedElements in your code, you can do that all with a local
  • The only functionaly difference compared to the original code is that the if (stackFrame) is moved a couple of lines up, which looks great to me
  • Please format the code as the indentation is completly broken
  • Does this actually fix the issue on your side? If yes I would gladly merge it in once you address my feedback from above.

Thanks again

@sandy081 sandy081 requested a review from isidorn Oct 20, 2017

@guw

This comment has been minimized.

Show comment
Hide comment
@guw

guw Oct 20, 2017

Contributor

@isidorn Thanks for the feedback. I think there is an issue with the global variable. I'll revisit the logic.

Re: formatting - Is there a trick? I do get a warning from the pre-commit hook. However, I was expecting VS Code to format on save. Also the format action seems to screw up the whole file.

Contributor

guw commented Oct 20, 2017

@isidorn Thanks for the feedback. I think there is an issue with the global variable. I'll revisit the logic.

Re: formatting - Is there a trick? I do get a warning from the pre-commit hook. However, I was expecting VS Code to format on save. Also the format action seems to screw up the whole file.

@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Oct 23, 2017

Contributor

@guw yes please revisit the logic. In the latest I see that you are still using the global variable.
For formating, I just do right click in editor -> format document. Please try that

Contributor

isidorn commented Oct 23, 2017

@guw yes please revisit the logic. In the latest I see that you are still using the global variable.
For formating, I just do right click in editor -> format document. Please try that

@guw

This comment has been minimized.

Show comment
Hide comment
@guw

guw Oct 24, 2017

Contributor

@isidorn The global variable is required because we need to persist the list of expanded elements between different executions of the function. It's implementing what you proposed in #16031.

Contributor

guw commented Oct 24, 2017

@isidorn The global variable is required because we need to persist the list of expanded elements between different executions of the function. It's implementing what you proposed in #16031.

@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Oct 24, 2017

Contributor

@guw ok, makes sense.
Can you please elaborote on my other questions from above:

  • Please format the code as the indentation is completly broken
  • Does this actually fix the issue on your side? If yes I would gladly merge it in once you address my feedback from above.
    and
  • Is the check on line 82 really necessery? I would assume not
Contributor

isidorn commented Oct 24, 2017

@guw ok, makes sense.
Can you please elaborote on my other questions from above:

  • Please format the code as the indentation is completly broken
  • Does this actually fix the issue on your side? If yes I would gladly merge it in once you address my feedback from above.
    and
  • Is the check on line 82 really necessery? I would assume not
@guw

This comment has been minimized.

Show comment
Hide comment
@guw

guw Oct 26, 2017

Contributor

@isidorn Do you have any suggestions on formatting? Whenever I try to format the document using VS Code it makes a ton of changes.

Contributor

guw commented Oct 26, 2017

@isidorn Do you have any suggestions on formatting? Whenever I try to format the document using VS Code it makes a ton of changes.

@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Oct 26, 2017

Contributor

@guw I will format. Can you please ansewer my questions from above.

Contributor

isidorn commented Oct 26, 2017

@guw I will format. Can you please ansewer my questions from above.

Store expansion state globally only when there are items.
When a debugger needs more time while stepping, the tree will go empty first
(while running) and then come back with data (when stopped again). This
modifies the change for #16031 to allow the tree to properly restore
state in case a debugger needs more time providing content.

The expanded state is saved in a global variable. A check ensures that an
intermittent empty tree does not override the global variable. Future refreshes
of an empty tree will then restore the expanded state of variables.
@guw

This comment has been minimized.

Show comment
Hide comment
@guw

guw Oct 26, 2017

Contributor

@isidorn patch rebased, simplified and formatted.

  • Confirmed that it fixes the issue
  • check on line 82 is gone
Contributor

guw commented Oct 26, 2017

@isidorn patch rebased, simplified and formatted.

  • Confirmed that it fixes the issue
  • check on line 82 is gone
@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Oct 26, 2017

Contributor

@guw looks good to me, thanks a lot for your contribution 🍻

Contributor

isidorn commented Oct 26, 2017

@guw looks good to me, thanks a lot for your contribution 🍻

@isidorn isidorn merged commit a798bb8 into Microsoft:master Oct 26, 2017

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
license/cla All CLA requirements met.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment