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

[plug-in][java][preferences] Add files.exclude preference #4274

Merged
merged 1 commit into from
Feb 11, 2019
Merged

[plug-in][java][preferences] Add files.exclude preference #4274

merged 1 commit into from
Feb 11, 2019

Conversation

svor
Copy link
Contributor

@svor svor commented Feb 7, 2019

Signed-off-by: Valeriy Svydenko vsvydenk@redhat.com

The problem
VS Code has files.exclude preference and some of VS Code extensions want to change value of this preference. In my case it is java-vscode extension. The last version of java-vscode extension (0.38.0) asks user to exclude some types of files (.project, .classpath) to hide them in Files view. To do that the extension wants to extend files.exclude preference, but Theia doesn't have such preference.

Changes
This PR offers to add files.exclude and makes it possible to hide excluded files/folders in Files view.

Related issue
#4254

Short demo
files_exclude

@svor svor added preferences issues related to preferences java issues related to the java language plug-in system issues related to the plug-in system vscode issues related to VSCode compatibility Team: Che-Languages issues regarding the che-languages team labels Feb 7, 2019
@svor svor self-assigned this Feb 7, 2019
@svor svor requested review from benoitf and evidolob as code owners February 7, 2019 10:24
Copy link
Contributor

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

fine for me for plugin-related code

waiting other feedback for preferences part

@kittaakos
Copy link
Contributor

This PR offers to add files.exclude and makes it possible to hide excluded files/folders in Files view.

I do not know much about the VS Code extensions but why do not we have one preference for this in Theia instead of having both the navigator.exclude and the new files.exclude?

@svor
Copy link
Contributor Author

svor commented Feb 7, 2019

@kittaakos for me it makes sense to remove navigator.exclude and just use files.exclude, but I wasn't sure of it, maybe some of Theia extensions or plugins already use navigator.exclude preference.

@kittaakos
Copy link
Contributor

but I wasn't sure of it, maybe some of Theia extensions or plugins already use navigator.exclude preference.

Figuring this out should be part of this PR, I think. If possible, the two preferences should be merged. Otherwise, it could be confusing later why do we have two.

// cc @akosyakov

@akosyakov
Copy link
Member

akosyakov commented Feb 8, 2019

@svor @kittaakos If navigator.exclude and files.exclude is the same thing, then let's break and rename it. We do next release as major, please update the change log about it.

@svor
Copy link
Contributor Author

svor commented Feb 8, 2019

@akosyakov @kittaakos I've made changes to use just files.exclude preference and deleted navigator.eclude. Also I've added an info about this into change log.
The PR is ready to review, please take another look at it.

@tsmaeder
Copy link
Contributor

tsmaeder commented Feb 8, 2019

There is a list of default settings here: https://code.visualstudio.com/docs/getstarted/settings

@svor
Copy link
Contributor Author

svor commented Feb 11, 2019

@akosyakov @kittaakos @svenefftinge
Can someone review this PR, please

@kittaakos
Copy link
Contributor

Doing the review...

Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

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

Other than my comment on the ctor, it works as expected. Thank you!

protected showHiddenFiles: boolean;

constructor(
@inject(FileNavigatorPreferences) protected readonly preferences: FileNavigatorPreferences
@inject(FileSystemPreferences) protected readonly filesPreferences: FileSystemPreferences
Copy link
Contributor

Choose a reason for hiding this comment

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

You should leave the constructor as it was

@inject(FileNavigatorPreferences) protected readonly preferences: FileNavigatorPreferences

and use property injection for the FileSystemPreferences. Otherwise, you break another API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kittaakos Thank you for your review. Can you explain more which API it can break?
I don't see any reason to inject FileNavigatorPreferences here it is never used in this class

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain more which API it can break?

Sure. Subclasses must call super in the ctors. If there is any subclass of the FileNavigatorFilter, you will break client code if we publish a new latest version. If clear, please adjust , if no, or I have overlooked something, let me know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kittaakos Thank you for the explanation, it is clear! I've fixed that.

@kittaakos
Copy link
Contributor

I'll give it a try once more, but it should be fine...

@@ -60,14 +64,10 @@ export class FileNavigatorFilter {
this.emitter.fire(undefined);
}

protected onPreferenceChanged(event: PreferenceChangeEvent<FileNavigatorConfiguration>): void {
let hasChanged = false;
protected onFilesPreferenceChanged(event: PreferenceChangeEvent<FileSystemConfiguration>): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

This cannot be renamed. Please restore the original method name. Sorry for not mentioning it before :(

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 left this method as it is, and added
protected onPreferenceChanged(event: PreferenceChangeEvent<FileNavigatorConfiguration>): void with empty body. Please take another look at it :)

…avigator.exclude

Signed-off-by: Valeriy Svydenko <vsvydenk@redhat.com>
Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

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

It works nicely, thank you for the patch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
java issues related to the java language plug-in system issues related to the plug-in system preferences issues related to preferences Team: Che-Languages issues regarding the che-languages team vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants