-
Notifications
You must be signed in to change notification settings - Fork 94
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 #968a: Group data sets’ favorites by profile and allow extenders to activate before loading profiles #984
Conversation
…ng by profile Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
…on click Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
… of favorited profile name Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
…ation Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
…click Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
…ialized at activation Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
…n Favorites and Session sections) Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
…tes and Session sections and tooltip update after member rename Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
…Favorites Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lauren-li this PR looks really good, but I do have a few questions.
You mention some existing issues so I am wondering if these are included as well:
- Saving a profile name without a search to go along with it results in an empty space in the favorites.
2. Renaming a member in the favorites results in an error
- There is a right-click action to save just a member, but when I got to the favorites folder the data set is saved with the member inside. So should the member itself be saved or should add to favorites not be an option to the member only the data set?
Thanks @JillieBeanSim! Points 1 and 3 (being able to favorite a profile without a search, and not being able to favorite a DS member without the parent PDS) are both pre-existing behaviors in the master branch. I believe both are good candidates to address as future improvements to Zowe Explorer. Regarding the second point with the error when renaming a DS member from, I am working to try and recreate the issue on my branch, but have been unable to thus far. |
@JillieBeanSim The error in point 2 may have to do with the new DS member name being too long. Do you still get the error when renaming to a DS member name that is 8 characters or less? I was able to recreate the errors in the Marketplace version with a 9-character rename, as well. |
correct, after retesting within the naming restrictions it worked, maybe we need a different error when naming conventions aren't followed vs a 500 error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lauren-li thanks for you work on this, functionality works great! tests and checks passing with the exception of known failing integration tests
…s and add unit test Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
264f165
Thank you to all those who have looked at this PR so far! To explain the most recent change (as it occurred after approvals were made):
The change is quite minor, but please feel free to take another look if you have the chance to. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works great! I love the new functionality, nodes are refreshing properly when favorites are added/removed and when nodes are added/deleted, from favorites or from the regular sessions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍 This is awesome!
No need to address conflicts yet
…-zowe into load-favs-by-profile-2 Signed-off-by: zFernand0 <fernando.rijocedeno@broadcom.com> # Conflicts: # __tests__/__unit__/dataset/DatasetTree.unit.test.ts # src/dataset/DatasetTree.ts # src/dataset/actions.ts
4742805
Signed-off-by: zFernand0 <fernando.rijocedeno@broadcom.com>
Signed-off-by: zFernand0 <fernando.rijocedeno@broadcom.com>
…de-extension-for-zowe into load-favs-by-profile-2 Signed-off-by: zFernand0 <fernando.rijocedeno@broadcom.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good to go!
Thanks @lauren-li
others have already approved! |
Proposed changes
Addresses the following issues for the Data Sets tree:
#968 (Favorites created by other extensions should not throw errors as the other extensions are not activated yet)
#168 (Group Favorites section based on profile)
This PR groups favorites for the Data Set tree by profile. Data set favorite nodes are still created when Zowe Explorer activates, but the profile/session properties of those nodes are not set until the user actually clicks to expand the relevant profile grouping node in the Favorites section for the first time. This lazy loading of profiles allows extenders to activate before profile loading is requested, and also prevents Zowe Explorer from throwing errors for invalid favorites/favorites’ profiles upon VS Code/Zowe Explorer startup when the user may not even be looking to interact with Zowe Explorer/data set favorites.
The profile/session values that are set when the user first clicks to expand a profile grouping node in the Favorites section are stored with the relevant favorites’ nodes, so that favorites’ profile loading should only need to occur once per profile group during a VS Code session. Any subsequent clicks to expand the same profile grouping node should just reference the stored values.
Gif of UI change:
Release Notes
Milestone: 1.9
Changelog: Group Data Set favorites by profile, and load/set favorites’ profiles only when the user clicks to expand the relevant profile grouping node in the Favorites section.
Types of changes
What types of changes does your code introduce to Zowe Explorer?
Put an
x
in the boxes that applyChecklist
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 reviewernpm run vscode:prepublish
has been executedFurther comments
Note that this PR addresses both issues #968 and #168, but does not close either of them. Similar modifications should also be made to the USS and Jobs favorites before closing these issues.
Related issues
The following issues found in master branch were fixed incidentally as part of the work for this PR:
Error running command zowe.deleteDataset: Cannot read property 'getLabel' of null. This is likely caused by the extension that contributes zowe.deleteDataset.
With this PR, all functionality with the Data Sets tree (both related and unrelated to favorites) should be working at least as well as they were originally, but some pre-existing issues (listed below) were found while working on this PR. I believe these issues can be fixed in future PRs, as they are already in the master branch, and not regressions caused by this PR. Note that some of these may already be filed in our Issues section, but I wanted to note them here as a convenient reference in case reviewers encountered any of them during testing:
Favorites-specific issues
General issues
createFile
function, which was not modified in this PR.):rejected promise not handled within 1 second: Error: TreeError [zowe.explorer] Data tree node not found: [object Object]
Syncing issues between Favorites and Sessions sections