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

Issue #1428: Limit commands shown in context menu on multi-select #1438

Closed
wants to merge 14 commits into from

Conversation

katelynienaber
Copy link
Contributor

@katelynienaber katelynienaber commented Aug 12, 2021

Proposed changes

Fix for issue #1428.

This is only a change in the Datasets tree (because only Datasets tree supports multi-select at this time).

Also, if you multi-select datasets/members and then try to do something with the profile, it is disabled. I did this because you could multi-select profiles and datasets, and then try to run a profiles command. So, I think we should handle this in another PR and for now it is simply disabled.

deleteExampleDocs

Release Notes

Milestone: Backlog
Changelog: Commands which do not support multi-select are hidden when many elemene

Types of changes

What types of changes does your code introduce to Zowe Explorer?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Updates to Documentation or Tests (if none of the other choices apply)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This checklist will be used as reference for both the contributor and the reviewer

  • I have read the CONTRIBUTOR GUIDANCE wiki
  • PR title follows Conventional Commits Guidelines
  • PR Description is included
  • gif or screenshot is included if visual changes are made
  • yarn workspace vscode-extension-for-zowe vscode:prepublish has been executed
  • All checks have passed (DCO, Jenkins and Code Coverage)
  • I have added unit test and it is passing
  • I have added integration test and it is passing
  • There is coverage for the code that I have added
  • I have tested it manually and there are no regressions found
  • I have added necessary documentation (if appropriate)
  • Any PR dependencies have been merged and published (if appropriate)

Further comments

It would be really cool to merge this before I add multi-copy/paste ^^' That's why I added this to 21PI3, because that is the deadline on the multi-select completion.

@katelynienaber katelynienaber added this to the 21PI3 milestone Aug 12, 2021
@katelynienaber katelynienaber self-assigned this Aug 12, 2021
@katelynienaber katelynienaber linked an issue Aug 12, 2021 that may be closed by this pull request
@katelynienaber katelynienaber changed the title Finished limiting command appearance on multi-select Issue #1428: Limit commands shown in context menu on multi-select Aug 12, 2021
@jellypuno
Copy link
Contributor

Is it possible to convert the regex to a positive check rather than a negative check on listMultiSelection? So we only need to add it in one or two when property?

"when": "view == zowe.uss.explorer && viewItem =~ /^(?!.*_fav.*)(textFile.*|binaryFile.*|directory.*)/ && !listMultiSelection",

@katelynienaber
Copy link
Contributor Author

katelynienaber commented Aug 26, 2021

Is it possible to convert the regex to a positive check rather than a negative check on listMultiSelection? So we only need to add it in one or two when property?

"when": "view == zowe.uss.explorer && viewItem =~ /^(?!.*_fav.*)(textFile.*|binaryFile.*|directory.*)/ && !listMultiSelection",

I don't think so. Since you can only turn on multi-select for the whole tree, you have to put controls on the ones that don't support it. AFAIK VSCode doesn't have any other way to tell that the command is unsupported, and it will try to run the unsupported commands even when many items are selected.

Info about multi-select for custom trees (turned on when the TreeViewOptions are passed in on tree initialization): microsoft/vscode#76941

@katelynienaber
Copy link
Contributor Author

A thought from Venkat: will a phrase like Delete data set(s) translate well? Maybe we should choose something else jic...

@lauren-li
Copy link
Contributor

A thought from Venkat: will a phrase like Delete data set(s) translate well? Maybe we should choose something else jic...

@katelynienaber I have a vague recollection of the Zowe Explorer squad discussing this (or at least the wording of this context menu item), and I think you had proposed just changing it to "Delete" (for both data sets and members), as that's the wording VS Code uses for both files and folders (single or multiple selection). Using this wording would also cover the edge(?) case where a user multi-selects a mix of data sets and members.

Signed-off-by: Katelyn Nienaber <katelyn.nienaber@broadcom.com>
@katelynienaber
Copy link
Contributor Author

@lauren-li this has been added!

@JillieBeanSim
Copy link
Contributor

@katelynienaber can do the audit fix like so, the fix is also in Fernando's PR
image

Signed-off-by: Katelyn Nienaber <katelyn.nienaber@broadcom.com>
@katelynienaber
Copy link
Contributor Author

@JillieBeanSim Audit is fixed and pushed...

@jellypuno
Copy link
Contributor

Is it possible to convert the regex to a positive check rather than a negative check on listMultiSelection? So we only need to add it in one or two when property?
"when": "view == zowe.uss.explorer && viewItem =~ /^(?!.*_fav.*)(textFile.*|binaryFile.*|directory.*)/ && !listMultiSelection",

I don't think so. Since you can only turn on multi-select for the whole tree, you have to put controls on the ones that don't support it. AFAIK VSCode doesn't have any other way to tell that the command is unsupported, and it will try to run the unsupported commands even when many items are selected.

Info about multi-select for custom trees (turned on when the TreeViewOptions are passed in on tree initialization): microsoft/vscode#76941

@lauren-li Do you think a change in the docs is needed here?

@crawr crawr added the On-Hold label Sep 2, 2021
@lauren-li
Copy link
Contributor

Is it possible to convert the regex to a positive check rather than a negative check on listMultiSelection? So we only need to add it in one or two when property?
"when": "view == zowe.uss.explorer && viewItem =~ /^(?!.*_fav.*)(textFile.*|binaryFile.*|directory.*)/ && !listMultiSelection",

I don't think so. Since you can only turn on multi-select for the whole tree, you have to put controls on the ones that don't support it. AFAIK VSCode doesn't have any other way to tell that the command is unsupported, and it will try to run the unsupported commands even when many items are selected.
Info about multi-select for custom trees (turned on when the TreeViewOptions are passed in on tree initialization): microsoft/vscode#76941

@lauren-li Do you think a change in the docs is needed here?

@jellypuno Yes, it would be good to put a short explanation into docs/README-Extending.md#contextual-hooks about how if the Extender is adding a command that does not support multi-select to a Zowe Explorer menu, they will need to add the !listMultiSelection into their command's when clause. That would make sense to do as part of this PR.

@jellypuno
Copy link
Contributor

@zFernand0
Copy link
Member

@katelynienaber,
Just wanted to drop by and thank you for all the contributions (this one in particular 😋)
Just updated the branch one last time to prevent future merge conflicts.
We will be closing this PR for now.
The PR and the issue have been linked to the Multi Selection Epic (#1286).
We will get back to this for sure 😋

@zFernand0 zFernand0 closed this Apr 7, 2022
@zFernand0
Copy link
Member

Please refer to 7f97adf for the latest code changes.
git checkout 7f97adfeb2cdeecf7b93c994e56c9d01df45e6f2

@zFernand0 zFernand0 deleted the limit-multi-select branch April 7, 2022 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change context menus for multi-select
6 participants