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

[filesystem] File dialog seems to use wrong folder when opened #4862

Closed
federicobozzini opened this issue Apr 10, 2019 · 7 comments · Fixed by #4868
Closed

[filesystem] File dialog seems to use wrong folder when opened #4862

federicobozzini opened this issue Apr 10, 2019 · 7 comments · Fixed by #4868
Labels
bug bugs found in the application dialogs issues related to dialogs file dialog issues related to the file dialog help wanted issues meant to be picked up, require help

Comments

@federicobozzini
Copy link
Contributor

Description

When a file dialog is opened, it's possible to pass a folder parameter. What I would personally expect would be to see the file dialog getting opened in that folder, what instead happens is that the parent of the folder parameter is used.

This is not the behaviour I would expect and makes it impossible to open a folder that has no subfolders.

Is there a reason behind this?

@akosyakov
Copy link
Member

I think too that the dialog should open a given folder, not its parent. I think it is because of historical reasons. It was implemented for Open menu item there opening a parent is convenient, but then we should apply this logic only to Open client.

@akosyakov akosyakov added bug bugs found in the application dialogs issues related to dialogs file dialog issues related to the file dialog help wanted issues meant to be picked up, require help labels Apr 10, 2019
@vince-fugnitto
Copy link
Member

@federicobozzini apparently with our implementation we always go one level higher when opening the file dialog as seen here:

https://github.com/theia-ide/theia/blob/36eba77252634a6227ec5e98eaf3e08f67e08dd8/packages/filesystem/src/browser/file-dialog/file-dialog-service.ts#L77

This means that wether we pass a file or directory URI when calling the file dialog, we open with respect to the parent.

@federicobozzini
Copy link
Contributor Author

Fixing this would require breaking backward compatibility, are you OK with that?

@vince-fugnitto
Copy link
Member

vince-fugnitto commented Apr 10, 2019

Fixing this would require breaking backward compatibility, are you OK with that?

Maybe we don't necessary need to break the compatibility. Perhaps we can introduce a new optional parameter OpenDialogOptions? which can describe open options when calling the open dialog.

One such option can be where to open the dialog from (ex: folder, parent of folder, workspace root), and would default to our current implementation so that clients would not be broken by this change.

@federicobozzini
Copy link
Contributor Author

@vince-fugnitto I think it would be quite unusual to have a folder parameter and at the same time an option to use a different folder like the workspace root, but ithat's certainly an option.

@vince-fugnitto
Copy link
Member

@federicobozzini I agree, I'm just worried about breaking clients who use the open-dialog-service and notice different results. I'm fine however on making it right, so if breaking is the way to go I'm all for it! 😄

I thought that with OpenDialogOptions? we can essentially support many more possibilities in the future.

@akosyakov what do you think? Would it be fine to break the compatibility like @federicobozzini suggested?

@akosyakov
Copy link
Member

I'm fine with breaking. But here i again come to this issue: #4797 We should test and align it with VS Code. I wish someone already creates such extension.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug bugs found in the application dialogs issues related to dialogs file dialog issues related to the file dialog help wanted issues meant to be picked up, require help
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants