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

feature: replace electron File.path with electron webUtils #213031

Merged

Conversation

SimonSiefke
Copy link
Contributor

From the electron docs:

Warning: The path property that Electron adds to the File interface is deprecated and will be removed in a future Electron release. We recommend you use webUtils.getPathForFile instead.


This PR replaces the deprecated File.path property with the new electron webUtils api.


For testing:

Native Drag and Drop in the File Explorer:

  1. Drag and drop a file from the native explorer to the vscode file tree
  2. Verify the file is added to the vscode explorer

Normal Drag and Drop in the File Explorer:

  1. Drag and drop a file from the vscode file tree to somewhere else in the vscode file tree
  2. Verify the file is moved as expected in the vscode file explorer

Drag and Drop in the Terminal:

  1. Drag and drop a file from the native explorer to the vscode terminal
  2. Verify the file path appears in the terminal

@bpasero bpasero assigned bpasero and unassigned lramos15 May 20, 2024
@bpasero bpasero added this to the On Deck milestone May 20, 2024
@bpasero
Copy link
Member

bpasero commented May 20, 2024

Thanks, I wasn't actually sure how to approach this when I saw it, so I will learn from your changes 🙏

@SimonSiefke
Copy link
Contributor Author

Thanks!

Initially, I only added electron webUtils to the preload script and exposed it as a global variable webUtils. However, it didn't seem ideal for browser/fileActions.ts to depend on electron-sandbox/globals.ts. That's why I created the webUtils service, inspired by the webViewService.

@bpasero
Copy link
Member

bpasero commented May 20, 2024

But I think a service that cannot be implemented for web is also not ideal. I think the right thing would be to depend on globals.ts from electron-sandbox and then move the actions that depend on it to electron-sandbox layers.

Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

@SimonSiefke
Copy link
Contributor Author

Thanks for the feedback.

Do you have an example of similar code where that is applied? I'm not sure how I would proceed here:

// browser/fileActions.ts
export const pasteFileHandler = async (accessor: ServicesAccessor, fileList?: FileList) => {
	if (toPaste.type === 'paths' && item instanceof URI) {
		return item.path
	}
	// how to call something some electron-sandbox here without importing the file from electron-sandbox?
}

Thanks again!

@bpasero
Copy link
Member

bpasero commented May 21, 2024

Yeah e.g. here:

const shellEnv = process.shellEnv();

For this, the action needs to be moved to a electron-sandbox namespace if possible.

@bpasero
Copy link
Member

bpasero commented May 21, 2024

Since this action seems to be used in web as well, it probably should be subclassed on desktop to provide the extra functionality and then contributed per-web and per-desktop.

@SimonSiefke
Copy link
Contributor Author

SimonSiefke commented May 30, 2024

I like reusing the IHostService to simplify the code and added some changes to implement it. What do you think?

@bpasero bpasero modified the milestones: On Deck, May 2024, June 2024 May 31, 2024
@bpasero
Copy link
Member

bpasero commented May 31, 2024

@SimonSiefke I pushed c0298e5 to cleanup, I found many warnings (missing semicolons, etc), did you see them in the editor before committing? please check if things still work.

@bpasero bpasero self-requested a review May 31, 2024 13:40
@SimonSiefke
Copy link
Contributor Author

I forgot I had ESLint disabled, thanks for the fixes! Pasting files in the explorer and terminal is also still working for me!

Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

Thanks!

@bpasero bpasero merged commit acfe0e2 into microsoft:main Jun 2, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants