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

feat: add silentMode to DownloadHelper (#121) #130

Merged
merged 4 commits into from
Mar 7, 2022

Conversation

lstocchi
Copy link
Contributor

it resolves #121

Signed-off-by: Luca Stocchi <lstocchi@redhat.com>
@lstocchi lstocchi requested a review from jeffmaury February 25, 2022 14:37
Copy link
Member

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

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

For better compatibility, I think the parameter should be part of the tool json file.
Now I did not test your stuff but it looks to me that there will be still a dialog be shown and I think this should be hidden or a background task to make sure user does not cancel it.

@lstocchi
Copy link
Contributor Author

lstocchi commented Feb 25, 2022

For better compatibility, I think the parameter should be part of the tool json file. Now I did not test your stuff but it looks to me that there will be still a dialog be shown and I think this should be hidden or a background task to make sure user does not cancel it.

What if we remove the cancel button from the progressindicator dialog? Atleast the user sees something. Because if we hide everything the user is not aware of what's happening and they see an empty view. If they have a slow connection they don't understand what's going on.
Or we can go with a background task (i guess an indicator is shown on the bottom right side, no?)
loadingcli

@jeffmaury
Copy link
Member

TBH having something visible will always lead to user complaining. And user should see a Loading item in the tree, so it's my preference 👍

Signed-off-by: Luca Stocchi <lstocchi@redhat.com>
@lstocchi
Copy link
Contributor Author

Now nothing is shown. Only a label (2 processes running in my case as knative is downloading kn and func clis) in the bottom right. If clicked all processes are displayed but not cancellable.
background-2

@lstocchi lstocchi requested a review from jeffmaury February 28, 2022 12:01
Copy link
Member

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

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

The only issue is that the UI will be frozen because the code is executed in the UI thread and once the background task is launched, the UI thread will be blocked on command.get()

Signed-off-by: Luca Stocchi <lstocchi@redhat.com>
@lstocchi lstocchi requested a review from jeffmaury March 2, 2022 11:17
Signed-off-by: Luca Stocchi <lstocchi@redhat.com>
@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 2, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Member

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

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

LGTM

@lstocchi lstocchi merged commit fcdb582 into redhat-developer:main Mar 7, 2022
@lstocchi lstocchi deleted the i121 branch March 7, 2022 08:33
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.

DownloadHelper should offer a silent mode
2 participants