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

Explicitly show import progress #2022

Merged
merged 5 commits into from
Aug 6, 2021

Conversation

jdneo
Copy link
Collaborator

@jdneo jdneo commented Jul 13, 2021

This is the first part of #2021.

In this PR:

  • we show the job status more explicitly during the import stage. (Like Eclipse or IntelliJ, at the right-bottom corner, users can see what jobs are running)

  • If user click the hyper link in the notification, the build status terminal will pop up and the notification get dismissed.

  • After importing jobs are finished. A dialog shows up to let the user know.

import-progress.mp4

Signed-off-by: Sheng Chen sheche@microsoft.com

@fbricon
Copy link
Collaborator

fbricon commented Jul 13, 2021

need a preference to not show the progress

@jdneo
Copy link
Collaborator Author

jdneo commented Jul 13, 2021

need a preference to not show the progress

Ok

@rgrunber @snjeza @testforstephen @akaroml @Eskibear @CsCherrYY Since this change related to UX, feel free to let me know if you have any comments

@fbricon
Copy link
Collaborator

fbricon commented Jul 13, 2021

nitpick : can you please fix the typo in the commit message? (s/improt/import)

Signed-off-by: Sheng Chen <sheche@microsoft.com>
@jdneo
Copy link
Collaborator Author

jdneo commented Jul 13, 2021

nitpick : can you please fix the typo in the commit message? (s/improt/import)

Done, will have another commit to add the preference

@fbricon fbricon changed the title Explicitly show improt progress Explicitly show import progress Jul 13, 2021
Copy link
Member

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

Works well for me. Just a minor thing to address so far.

src/serverTaskPresenter.ts Outdated Show resolved Hide resolved
@jdneo
Copy link
Collaborator Author

jdneo commented Jul 14, 2021

About adding a setting to control the appearance of the progress notification.

I'm thinking that adding a setting called: java.showJobStatusOnStart, which has 3 options:

  • notification: show the job status via notification on start (default)
  • terminal: Show the job status via terminal on start
  • off: Do not show the job status on start

Meanwhile, deprecated java.showBuildStatusOnStart.enabled. (Still working but consider to remove it after several iterations). What do you think?

Signed-off-by: Sheng Chen <sheche@microsoft.com>
… of the progress notification

Signed-off-by: Sheng Chen <sheche@microsoft.com>
@jdneo
Copy link
Collaborator Author

jdneo commented Jul 15, 2021

PR update, finally I use the anyOf mechanism in package.json to reuse the setting java.showBuildStatusOnStart.enabled meanwhile provides backwards compatibility. User can set it to:

  • notification: Show the build status via progress notification.

  • terminal: Show the build status via terminal.

  • off: Do not show any build status.

    For backward compatibility, this setting also accepts boolean value, where true has the same meaning as notification and false has the same meaning as off.

Besides, the default value changes to notification

@fbricon @rgrunber

Signed-off-by: Sheng Chen <sheche@microsoft.com>
Copy link
Member

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

Change works well. Just one thing I noticed that may be annoying (given how much space the progress bar takes), although the same behaviour occurs with the status bar provider.

If you kill the syntax/standard server while it's importing/building the project (or it crashes), the progress notification will remain indefinitely on the screen. The restarted language server ultimately finishes the importing, but the progress bar remains. I think the same thing can happen for the status bar provider. I wonder if there's a way to deal with this.

@jdneo
Copy link
Collaborator Author

jdneo commented Jul 17, 2021

I can imagine that some users might not like these kind of pop-ups.

Or, maybe we can still use off as the default value and then in the terminal, always show some action links somewhere in it. One of the action is open the related setting to let user set the preference.

Anyway, we can discuss in our next sync meeting.

@jdneo jdneo mentioned this pull request Jul 21, 2021
Signed-off-by: Sheng Chen <sheche@microsoft.com>
@jdneo
Copy link
Collaborator Author

jdneo commented Jul 22, 2021

PR updated to simplify the text.

Meanwhile, I created a new issue to track the item that dealing with the situation when server crashes

Copy link
Member

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

LGTM. Feel free to squash and merge when ready.

@jdneo
Copy link
Collaborator Author

jdneo commented Aug 5, 2021

Hi @rgrunber, Jinbo(@testforstephen) is the only person who has the permission to merge PRs on our side. He is now on vacation until next week.

If you plan to make a release in this week, maybe we need your help to merge. :)

@rgrunber rgrunber merged commit 18e9821 into redhat-developer:master Aug 6, 2021
@rgrunber rgrunber added this to the End July 2021 milestone Aug 6, 2021
@jdneo jdneo deleted the cs/import-notification branch August 6, 2021 04:29
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.

None yet

3 participants