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

Substitute ~ for home directory in file dialog #9416

Conversation

kenneth-marut-work
Copy link
Contributor

@kenneth-marut-work kenneth-marut-work commented Apr 29, 2021

What it does

Fixes #9142 by adding support for '~' character in text input field of browser's file dialog. This MR also introduces some small refactors including factoritizing LocationRenderer and making it injectable to allow for cleaner dependency management and fixes a small issue to prevent autosuggest when path does not begin with / (or ~/)

How to test

  • Build this branch for browser and run
  • Open a file dialog (either save/open) and click on the 'folder' icon to switch to text input mode
  • Begin typing a path that starts with ~, observe that autocompletion works as expected
  • Navigate to a valid path that begins with ~ by pressing enter, observe it works as expected and there are no regressions

Review checklist

Reminder for reviewers

tilde

@kenneth-marut-work kenneth-marut-work added the file dialog issues related to the file dialog label Apr 29, 2021
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@kenneth-marut-work I like the idea, I just had a couple of questions:

1: we have tildify, I wonder if it can be used in such a case (or perhaps it can be refactored to be simpler like you suggested but it does handle other operating systems such as windows):

describe('Linux', () => {
it('should shorten path on Linux, path starting with home', async () => {
const path = `${linuxHome}/a/b/theia`;
const expected = '~/a/b/theia';
expect(Path.tildify(path, linuxHome)).eq(expected);
});

2: what happens in the windows use-case, I believe the substitution should not occur in such a case?

@kenneth-marut-work
Copy link
Contributor Author

@kenneth-marut-work I like the idea, I just had a couple of questions:

1: we have tildify, I wonder if it can be used in such a case (or perhaps it can be refactored to be simpler like you suggested but it does handle other operating systems such as windows):

describe('Linux', () => {
it('should shorten path on Linux, path starting with home', async () => {
const path = `${linuxHome}/a/b/theia`;
const expected = '~/a/b/theia';
expect(Path.tildify(path, linuxHome)).eq(expected);
});

2: what happens in the windows use-case, I believe the substitution should not occur in such a case?

I didn't know about tildify. I'll look into it, thanks for the tip 👍

@kenneth-marut-work
Copy link
Contributor Author

@vince-fugnitto I've made use of the tildify method and also added an 'untildify' which does the reverse operation (along with tests) and also ensured that we skip the replacement for windows. I've built and tested on both Linux and Windows and it seems to be behaving as expected.

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I confirmed that the changes work as expected 👍

@kenneth-marut-work kenneth-marut-work force-pushed the feature/substitute-home branch from 5a52435 to d28fc52 Compare May 3, 2021 13:30
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I confirmed that the behavior works as intended on Linux 👍
It might be worthwhile to also verify on a Windows machine.

@kenneth-marut-work
Copy link
Contributor Author

I confirmed that the behavior works as intended on Linux 👍
It might be worthwhile to also verify on a Windows machine.

I did build and test on Windows and am getting the expected behavior of no substitution

@kenneth-marut-work kenneth-marut-work force-pushed the feature/substitute-home branch from d28fc52 to c08ff72 Compare May 12, 2021 17:03
@kenneth-marut-work
Copy link
Contributor Author

I've pushed a small change to make the FileDialogTreeFiltersRenderer injectable since it seemed appropriate to update alongside the LocationListRenderer

@@ -126,7 +126,8 @@ export abstract class FileDialog<T> extends AbstractDialog<T> {
constructor(
@inject(FileDialogProps) readonly props: FileDialogProps,
@inject(FileDialogWidget) readonly widget: FileDialogWidget,
@inject(LocationListRendererFactory) readonly locationListFactory: LocationListRendererFactory
@inject(LocationListRendererFactory) readonly locationListFactory: LocationListRendererFactory,
@inject(FileDialogTreeFiltersRendererFactory) readonly treeFiltersFactory: FileDialogTreeFiltersRendererFactory
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a new injection, maybe use property injection instead of constructor injection? Same below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've refactored this a bit to utilize property injection for everything except for props and moved the remaining initialization to a postConstruct. I've also updated this for SaveFileDialog and OpenFileDialog. Let me know what you think 👍

@vince-fugnitto
Copy link
Member

@kenneth-marut-work do we plan on merging for the release?

@kenneth-marut-work
Copy link
Contributor Author

@kenneth-marut-work do we plan on merging for the release?

@vince-fugnitto yes that would be ideal. We'll take care of it this morning if everything looks good 👍

@kenneth-marut-work kenneth-marut-work force-pushed the feature/substitute-home branch from 26cdef5 to 882f673 Compare May 26, 2021 15:05
@colin-grant-work
Copy link
Contributor

This looks good to me if there are no objections to the breaking changes. 👍

Signed-off-by: Kenneth Marut <kenneth.marut@ericsson.com>
@kenneth-marut-work kenneth-marut-work force-pushed the feature/substitute-home branch from 882f673 to d88f5c9 Compare May 26, 2021 15:43
@kenneth-marut-work kenneth-marut-work merged commit 8343cb8 into eclipse-theia:master May 26, 2021
@vince-fugnitto vince-fugnitto added this to the 1.14.0 milestone May 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
file dialog issues related to the file dialog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support ~ as replacement for HOME directory in file dialog text-input
3 participants