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

drag&drop a VSIX file on the extensions viewlet #94152

Closed
wants to merge 21 commits into from
Closed

drag&drop a VSIX file on the extensions viewlet #94152

wants to merge 21 commits into from

Conversation

lszomoru
Copy link
Member

@lszomoru lszomoru commented Apr 1, 2020

This PR fixes #12090

Add the capability to drag&drop a VSIX file on the extensions viewlet and have it installed. While there is still room for improvement as well as some code duplication that I would like to explore removing, I want to get some early feedback on these changes. Thanks!

@lszomoru
Copy link
Member Author

lszomoru commented Apr 1, 2020

Tagging @joaomoreno and @sandy081 for review. Thanks!

@sandy081 sandy081 self-requested a review April 1, 2020 16:24
@sandy081 sandy081 added this to the April 2020 milestone Apr 1, 2020
@sandy081 sandy081 added the extensions Issues concerning extensions label Apr 1, 2020
@joaomoreno joaomoreno self-requested a review April 2, 2020 05:46
@lszomoru
Copy link
Member Author

lszomoru commented Apr 2, 2020

I have updated the pull request to get rid of the code duplication (onDrop will invoke the existing install VSIX action), as well as I have fixed a bug due to which the @installed view was not refreshed after an extension was installed using the VSIX though the action or by dropping the VSIX on the extensions viewlet.

@sandy081
Copy link
Member

sandy081 commented Apr 6, 2020

@lszomoru Thanks for the PR. I will take a look and get back to you.

BTW I updated the branch to make it easy for me to try out.

Copy link
Member

@sandy081 sandy081 left a comment

Choose a reason for hiding this comment

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

Changes look good to me except for some minor feedback (provided).

@@ -431,6 +438,44 @@ export class ExtensionsViewPaneContainer extends ViewPaneContainer implements IE
}
}));

// Register DragAndDrop support
this._register(new DragAndDropObserver(this.root, {
Copy link
Member

Choose a reason for hiding this comment

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

image

Shall we have drop overlay only on extensions views excluding the search box ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Based on my conversation with @joaomoreno, we wanted to keep things simple and that is why we are set up the drop target/drop overlay to be the shown on the whole extensions view. If you feel strongly about this, or there is a prior pattern not to show the drop overlay over the search box, I am happy to update it.

@sandy081
Copy link
Member

sandy081 commented Apr 6, 2020

as well as I have fixed a bug due to which the @installed view was not refreshed after an extension was installed using the VSIX though the action or by dropping the VSIX on the extensions viewlet.

Thanks for fixing this. Just to let you know there is a general bug that extension views do not get updated automatically when an extension is modified.

@lszomoru
Copy link
Member Author

lszomoru commented Apr 6, 2020

as well as I have fixed a bug due to which the @installed view was not refreshed after an extension was installed using the VSIX though the action or by dropping the VSIX on the extensions viewlet.

Thanks for fixing this. Just to let you know there is a general bug that extension views do not get updated automatically when an extension is modified.

@sandy081, do you have a link to the issue? Happy to take a look at it and submit a PR with the fix.

@sandy081
Copy link
Member

sandy081 commented Apr 6, 2020

do you have a link to the issue? Happy to take a look at it and submit a PR with the fix.

Here is the issue - #67603
I do not think the fix that is made here will help. We need a general fix - view (enabled/installed) should be updated if the view gets changed. It's not an issue that is suggested for contribution.

@joaomoreno
Copy link
Member

@sandy081, I think you forgot this

@lszomoru
Copy link
Member Author

Looks like storage.ts made it into this PR which is out of scope of this change. Working on removing it.

@lszomoru
Copy link
Member Author

@sandy081, fixed up the pull request to revert the changes that were not in scope. Thanks for the feedback.

@lszomoru
Copy link
Member Author

Abandoning the PR since my fork got into a funky state. Will re-create it from a clean fork.

@lszomoru lszomoru closed this Apr 29, 2020
@sandy081
Copy link
Member

@joaomoreno nopes. Waiting for changes and discussing with @lszomoru over slack.

@lszomoru
Copy link
Member Author

lszomoru commented May 4, 2020

I have resubmitted these changes in a new pull request - #96907

@github-actions github-actions bot locked and limited conversation to collaborators Jun 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
extensions Issues concerning extensions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement VSIX drag and drop to extension viewlet
3 participants