-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
adding suggestion to search for extension for files are of unknown mime type #40269
Conversation
guessMimeType sometimes returns MIME_UNKNOWN for known extensions. There appears to be a race between this logic and the registration of known mime types. |
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.
Nice work, left a few tiny comments. Take a look.
JSON.stringify(fileExtensionSuggestionIgnoreList), | ||
StorageScope.GLOBAL | ||
); | ||
return this.ignoreExtensionRecommendations(); |
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.
I believe this is a remnant of a copy paste :)
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.
I added the new ignore list to the store because I can imagine if a user has a specific file type that never will definitely not have an extension, they would be pretty annoyed by the new message if it repeated. As for the this.ignoreExtensionRecommendations(), I left this as well since this triggers the prompt for ignoring all recommendation notifications and will also work to disable this new message. Let me know what you think.
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.
I got the part about the new ignore list earlier. That's good.
Am not sure about re-purposing this.ignoreExtensionRecommendations()
for the file extension search.
Some users may not want any recommendations, but may be ok with the file search or vice versa
); | ||
return this.ignoreExtensionRecommendations(); | ||
case 2: | ||
return TPromise.as(null); |
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.
You can skip the 3 Promise.as(null)
here. They won't end up serving any purpose
this.choiceService.choose(Severity.Info, message, options, 3).done(choice => { | ||
switch (choice) { | ||
case 0: | ||
return searchMarketplaceAction.run(); |
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.
Can you add the telemetry events to track what the user does here just like in the other choiceService
?
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 change makes sense and worked for me as expected after adding more important tips
@sbatten There are few more things we need to hash out before merging this to master. Thanks for all the work though, this is nice feature to have |
30a64ba
to
c65e758
Compare
8fc3a29
to
0208b0c
Compare
When user loads a file of unknown mime type in the editor, suggest searching for an extension in the marketplace supporting that file extension.