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

viewer: bring focus to editor if it already exists #5631

Merged
merged 3 commits into from
Dec 6, 2024
Merged

Conversation

isabelizimm
Copy link
Contributor

@isabelizimm isabelizimm commented Dec 5, 2024

Previously, a new editor pane populated each time you opened it from the Viewer.

Note: if you

  • open an HTML file in Viewer
  • open in an editor
  • close file in Viewer
  • reopen in Viewer
  • open in an editor again

it WILL open a new tab since the url to open HTML files is regenerated each time. I thought about using the file name instead, but it does not include directories, eg, index.html and files/index.html would be considered identical. I can look into something more elegant if we want to handle for the above situation, but I'm not sure how often it will happen/if this behaviour is actually desired.

QA Notes

Clicking Open the content in an editor pane. multiple times will bring focus to opened tab rather than opening new ones.

@juliasilge
Copy link
Contributor

Addresses #5527

Copy link
Contributor

@juliasilge juliasilge left a comment

Choose a reason for hiding this comment

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

Works great! 🙌

I understand that this only applies to one open in the Viewer pane, and that additional instances of the Viewer pane being opened will open a new editor tab. I don't think we need to fix this now. Would you open a followup issue on that, after this gets merged?

When I open HTML in the editor tab, it get the little eye icon for the command "Open in Viewer". If I click on that (which is a bit silly, given that it is already open in the Viewer), I see the error:

Check dependency list! Synchronous require cannot resolve module 'path'. This is the first mention of this module!

error-viewer-editor

Is it possible to do something low lift to resolve this? Like only apply the command to HTML in source mode? It would be fine for this to be a followup PR, or to open an issue for it and send it to triage if it is not straightforward to solve currently.

@isabelizimm
Copy link
Contributor Author

I understand that this only applies to one open in the Viewer pane, and that additional instances of the Viewer pane being opened will open a new editor tab. I don't think we need to fix this now.

Correct. IMO, it seems like we would want the ideal behavior to be pretty close to what we have now; that is, we would want to be able to have multiple different editor tabs from the Viewer (eg, multiple rendered quarto documents). What is missing for multiple-editor-tabs is the ability to track if an editor is populated from a Viewer pane that was closed and then reopened--which I can open an issue for once this is merged!

I see the error: Check dependency list! Synchronous require cannot resolve module 'path'. This is the first mention of this module!

Let me check out turning this off! FWIW: I am seeing this error even for source HTML files, and the popped out editor tabs Open in Viewer button is totally busted.

@@ -22,14 +22,14 @@
"menus": {
"explorer/context": [
{
"when": "resourceLangId == html",
"when": "resourceLangId == html && resourceScheme == file",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

since the tab is technically not "looking at a file", this allows source html to keep the eye icon to open in viewer, but it should not show up at all for viewer->editor tabs!

Copy link
Contributor

Choose a reason for hiding this comment

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

Works great!

@midleman
Copy link
Contributor

midleman commented Dec 5, 2024

This looks good 🎉 . I'm likely going into extreme edge case scenario over here... What should happen in a "split" editor situation? (see vid)

Screen.Recording.2024-12-05.at.3.55.14.PM.mov

@juliasilge
Copy link
Contributor

Split editors do work well for the Data Explorer and Plots in an editor tab. Is it doable to get them working for Viewer tabs as well?

@isabelizimm
Copy link
Contributor Author

I'm almost certain the split editor is related to #5500, as both are issues with moving the webview overlay. I'll fix that in a followup PR!

Copy link
Contributor

@juliasilge juliasilge left a comment

Choose a reason for hiding this comment

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

Sounds good! Thank you so much 🚀

@isabelizimm isabelizimm merged commit 67e4835 into main Dec 6, 2024
5 checks passed
@isabelizimm isabelizimm deleted the viewer-focus branch December 6, 2024 16:37
@github-actions github-actions bot locked and limited conversation to collaborators Dec 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants