Skip to content

Remove line about selecting a language from the dropdown. #957

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Oct 1, 2021
Merged

Remove line about selecting a language from the dropdown. #957

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b40f648
Remove line about selecting a language from the dropdown.
marcnjaramillo Sep 30, 2021
3fb2c71
Merge branch 'main' into fix-lgtm-download-message
marcnjaramillo Oct 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions extensions/ql-vscode/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

## [UNRELEASED]

- Remove line about selecting a language from the dropdown when downloading database from LGTM. This makes the download progress visible when the popup is not expanded. [#894](https://github.com/github/vscode-codeql/issues/894)

## 1.5.5 - 08 September 2021

- Fix bug where a query is sometimes run before the file is saved. [#947](https://github.com/github/vscode-codeql/pull/947)
Expand Down
2 changes: 1 addition & 1 deletion extensions/ql-vscode/src/databases-ui.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,7 @@ export class DatabaseUI extends DisposableObject {
'codeQLDatabases.chooseDatabaseLgtm',
this.handleChooseDatabaseLgtm,
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional:
Here is a more involved suggestion for you to try out, which I think would work nicely. Consider it an option for further exploration: you can choose whether to include it as part of this PR, or complete this PR and try this suggestion in a follow-up PR.

If we look at the handleChooseDatabaseLgtm function, you can see it has a parameter progress: ProgressCallback. Progress callbacks work by allowing you to provide a progress message and step number, e.g. the code

progress({
  message: 'Choose project',
  step: 1,
  maxStep: 2
})

will update the progress notification box with the text title: message (e.g. "Adding database from LGTM: Choose project") and a 50% complete progress bar.

Instead of having the "Choose language" text in the title, which you've correctly removed in this PR, I think we could add two progress() calls -- one that says "Choose project" and one that says "Choose language". Then the popup box exactly describes the action we are asking the user to carry out in the UI at the top of the screen.

You may need to experiment to choose the best place to put those two progress calls. I think the function promptImportLgtmDatabase is a good place for the first one, and promptForLanguage is a good place for the second one. Both are reached from handleChooseDatabaseLgtm. The second one will require passing a progress parameter through to a few more functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this idea! I think for now I'll complete this PR so we can at least have some small improvement, and then I'll spend some time this afternoon and tomorrow working on it.

{
title: 'Adding database from LGTM. Choose a language from the dropdown, if requested.',
title: 'Adding database from LGTM',
})
);
this.push(
Expand Down
2 changes: 1 addition & 1 deletion extensions/ql-vscode/src/extension.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -771,7 +771,7 @@ async function activateWithInstalledDistribution(
) =>
databaseUI.handleChooseDatabaseLgtm(progress, token),
{
title: 'Adding database from LGTM. Choose a language from the dropdown, if requested.',
title: 'Adding database from LGTM',
})
);
ctx.subscriptions.push(
Expand Down