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

Update MiniBrowserEndpoint.defaultHandler() response to return content even when file type is not exists at mime-db #7356

Merged
merged 1 commit into from
Mar 19, 2020

Conversation

i501378
Copy link
Contributor

@i501378 i501378 commented Mar 17, 2020

What it does

Support other mime type files to be fetched on mini browser preview.

How to test

test.zip
Upload the file and unzip it into the root of workspace.
Right click on test.html file and select "Open With" -> "Preview"
The "TITLE" and "SUBTITLE" are replaced by the related translation texts from i18n.properties file.

Fixes #7231

Review checklist

Reminder for reviewers

@vince-fugnitto
Copy link
Member

@i501378 please be sure to sign the Eclipse ECA using the same email as your sign-off. It is the reason for the failed CI check.

@vince-fugnitto vince-fugnitto added the mini-browser issues related to the mini-browser label Mar 17, 2020
@akosyakov akosyakov requested a review from kittaakos March 17, 2020 15:51
@@ -180,7 +180,7 @@ export class MiniBrowserEndpoint implements BackendApplicationContribution, Mini
const mimeType = lookup(FileUri.fsPath(stat.uri));
if (!mimeType) {
this.logger.warn(`Cannot handle unexpected resource. URI: ${statWithContent.stat.uri}.`);
return response.send();
return response.send(content);
Copy link
Contributor

@kittaakos kittaakos Mar 17, 2020

Choose a reason for hiding this comment

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

Please

if (!mimeType) {
    this.logger.warn(`Cannot handle unexpected resource. URI: ${statWithContent.stat.uri}.`);
    response.contentType('application/octet-stream');
} else {
    response.contentType(mimeType);
}
return response.send(content);

Updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

According to mime-type:

No fallbacks. Instead of naively returning the first available type, mime-types simply returns false, so do var type = mime.lookup('unrecognized') || 'application/octet-stream'.

I have updated my snippet above 👆

Copy link
Member

Choose a reason for hiding this comment

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

@kittaakos Do we need to log warning? We don't have it for webviews and so far nobody complained.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to log warning? We don't have it for webviews and so far nobody complained.

I do not have a strong preference; if you feel we need it, please proceed. Otherwise, we can merge the PR.

Copy link
Member

Choose a reason for hiding this comment

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

It is to avoid unnecessary noise. I'm fine with merging as is.

@kittaakos kittaakos self-requested a review March 18, 2020 08:58
Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

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

Nice, it works nicely. I do not mind changing this behavior. @i501378, please squash your commits into one and then we can merge it. Thanks! 👍

…t when file type is not exists at mime-db

Signed-off-by: Lital Gilboa <lital.gilboa@sap.com>
@kittaakos kittaakos merged commit 5fb75c7 into eclipse-theia:master Mar 19, 2020
@i501378 i501378 deleted the extend-mini-browser-preview branch March 19, 2020 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mini-browser issues related to the mini-browser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

i18n.properties files cannot be viewed at command "Preview: Open URL"
4 participants