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

mini-browser: fix source and preview #10047

Merged
merged 2 commits into from
Sep 15, 2021
Merged

mini-browser: fix source and preview #10047

merged 2 commits into from
Sep 15, 2021

Conversation

vince-fugnitto
Copy link
Member

@vince-fugnitto vince-fugnitto commented Sep 3, 2021

What it does

Fixes: #5110
Fixes: #5223

Closes: #5206

The change fixes the source and preview commands from the mini-browser to use the proper opener.
The change allows us to now successfully open the source of a preview, and vice-versa which was previously not possible (master).

The commit also aligns the behavior when opening the sources from a preview to open as tab-right instead of in a new editor group similarly to the behavior present in vscode.

How to test

  1. start the application with a workspace (use a workspace that contains images (.png, .svg, .jpg), and html files).
  2. double-click a file the mini-browser supports (ex: .svg).
  3. confirm the file is opened as preview (rendered content)
  4. confirm the same use-case when opening from the quick-file-open
  5. confirm that the open source toolbar item successfully opens the file's sources (a dialog might appear)
  6. close the preview (rendered content)
  7. confirm the open preview toolbar item opens the file's preview
mini-browser-fix.mp4

Review checklist

Reminder for reviewers

Signed-off-by: vince-fugnitto [email protected]
Co-authored-by: fangx [email protected]

@vince-fugnitto vince-fugnitto added the mini-browser issues related to the mini-browser label Sep 3, 2021
@vince-fugnitto vince-fugnitto self-assigned this Sep 3, 2021
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

I can confirm the issue exists on master and is addressed by this change nicely 👍

@colin-grant-work
Copy link
Contributor

This is working well for me, but I have one minor comment re: the open source behavior:

  • In VSCode, if you open the preview for a markdown file, (close the source, if necessary,) and then click the 'Show Source' toolbar item, the source file opens in the same group rather than to left. In Theia, it opens to the left.

Perhaps that's outside the scope of this PR, but it was the main difference between our behavior and theirs that I could detect.

Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

The behavior is correct and consistent. 👍

@vince-fugnitto
Copy link
Member Author

I'm still thinking of what the best behavior would be for the mini-browser to open resources (canHandle).
I believe the best solution would be to open the sources when possible (supported resource), and if it cannot be opened then the preview should have the highest opener.

Resources such as:

  • .svg
  • .html

Should likely have the sources as the primary or highest opener.

@vince-fugnitto
Copy link
Member Author

The changes should be good now if anyone is interested in another round of review 👍

The change:

  • fixes the 'open source' toolbar item
  • fixes the 'open preview' toolbar item
  • opens resources by default based on the handler with the highest priority (ex: svg opens with the code editor)
  • opens resources with tab-after similarly to vscode

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Behavior is aligned with vscode now 👍

vince-fugnitto and others added 2 commits September 13, 2021 14:10
The commit fixes issues related to `preview` and `source` in the
mini-browser extension.

The change:
- allows users to open the source of an image if they use the command,
  or toolbar item.
- allows users to open the preview of an image's sources if they use the
  command or toolbar item.

The fix should also apply to all supported extension types the
mini-browser declares (svg, html,...).

Signed-off-by: vince-fugnitto <[email protected]>
Co-authored-by: fangx <[email protected]>
The commit aligns the behavior when opening the source of a previewed
(rendered) markdown to open it as `tab-after` (aligning with the
behavior present in vscode).

Signed-off-by: vince-fugnitto <[email protected]>
@vince-fugnitto
Copy link
Member Author

@colin-grant-work did you have any additional feedback for the pull-request?

Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

I reiterate my approval and praise the additional steps taken to bring us into alignment with VSCode :-).

@vince-fugnitto vince-fugnitto merged commit 0221c61 into master Sep 15, 2021
@github-actions github-actions bot added this to the 1.18.0 milestone Sep 15, 2021
@vince-fugnitto vince-fugnitto deleted the vf/mini-browser branch September 15, 2021 23:36
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
3 participants