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

Fix Hanging Progress Bar #536

Merged
merged 3 commits into from
Jul 30, 2018
Merged

Fix Hanging Progress Bar #536

merged 3 commits into from
Jul 30, 2018

Conversation

allileong
Copy link
Contributor

What does this PR do?

Fixes the hanging progress bar that prevents

  • SFDX: Refresh SObject Definitions
  • SFDX: Turn on Apex Debug Log for Replay Debugger
  • SFDX: Get Apex Debug Logs...

from executing.

What issues does this PR fix or reference?

W-5227494

@allileong allileong requested a review from vazexqi July 30, 2018 21:15
execution: CommandExecution,
token: vscode.CancellationTokenSource
) {
await vscode.window.withProgress(
return vscode.window.withProgress(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my original pr (#527), I made the assumption that because withProgress returns a promise, I had to await the resolution of the promise before continuing on with the execution of the remaining command code. However, this was a wrong assumption. WithProgress sets up listeners that listen for the success/error exit codes of commands before resolving, and for a few commands, waiting on withProgress to resolve actually prevented the command from even being executed at all. As a result, the listeners on withProgress never received exit subjects, withProgress never resolved, the command code was blocked from being executed, and the looping continued.

Copy link
Contributor

@vazexqi vazexqi left a comment

Choose a reason for hiding this comment

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

I think this would prevent the hanging since you are no longer waiting for the progress indicator before continuing.

I have a question (and maybe @lcampos already answered this in the previous review) – what happens to the preference to show only in the status bar in https://github.com/forcedotcom/salesforcedx-vscode/pull/259/files?

@allileong
Copy link
Contributor Author

allileong commented Jul 30, 2018

@vazexqi I did not look into adding that preference to the progress notification. Right now, if you select that preference on one of the success popups, subsequent success messages will only be shown in the status bar, but the progress will still be shown in the progress notification popup. Should I add that preference to the progress notification to make the experience more consistent?

@codecov
Copy link

codecov bot commented Jul 30, 2018

Codecov Report

Merging #536 into develop will decrease coverage by 0.27%.
The diff coverage is 20%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #536      +/-   ##
===========================================
- Coverage    76.33%   76.05%   -0.28%     
===========================================
  Files          147      147              
  Lines         5793     5793              
  Branches       904      904              
===========================================
- Hits          4422     4406      -16     
- Misses        1151     1161      +10     
- Partials       220      226       +6
Impacted Files Coverage Δ
...de-core/src/commands/forceVisualforcePageCreate.ts 43.33% <0%> (ø) ⬆️
...x-vscode-core/src/commands/forceApexClassCreate.ts 43.33% <0%> (ø) ⬆️
...edx-vscode-core/src/commands/forceProjectCreate.ts 73.21% <0%> (ø) ⬆️
...core/src/commands/forceLightningComponentCreate.ts 43.33% <0%> (ø) ⬆️
...code-core/src/commands/forceGenerateFauxClasses.ts 31.03% <0%> (ø) ⬆️
...vscode-core/src/commands/forceApexTriggerCreate.ts 48.38% <0%> (ø) ⬆️
...rcedx-vscode-core/src/commands/forceApexExecute.ts 29.54% <0%> (ø) ⬆️
...cedx-vscode-core/src/commands/forceAuthWebLogin.ts 65.3% <0%> (ø) ⬆️
...ode-core/src/commands/forceLightningEventCreate.ts 43.33% <0%> (ø) ⬆️
...scode-core/src/commands/forceLightningAppCreate.ts 43.33% <0%> (ø) ⬆️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db32fe1...62cad0e. Read the comment docs.

Copy link
Contributor

@vazexqi vazexqi left a comment

Choose a reason for hiding this comment

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

It's OK. We don't need to add a new preference to it. We can show it for now. And rely on VS Code's hiding mechanism for the user to hide the progress bar.

But, do file a child bug for the Running Task view.

@allileong allileong merged commit 7f3cba2 into develop Jul 30, 2018
@allileong allileong deleted the alli/fixProgressNotification branch July 30, 2018 22:51
@allileong allileong changed the title *WIP* Fix Hanging Progress Bar Fix Hanging Progress Bar Jul 30, 2018
jkalloor0701 pushed a commit to jkalloor0701/salesforcedx-vscode that referenced this pull request Aug 10, 2018
* Remove await from ProgressNotification.show

* Remove async from ProgressNotification.show

* Revert async execute method

@W-5227494@
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