Skip to content

Hide java project setting files by default.#776

Merged
fbricon merged 5 commits into
masterfrom
yaohai_dev
Jan 31, 2019
Merged

Hide java project setting files by default.#776
fbricon merged 5 commits into
masterfrom
yaohai_dev

Conversation

@yaohaizh

@yaohaizh yaohaizh commented Jan 25, 2019

Copy link
Copy Markdown
Collaborator

@fbricon
Fix issue #618 which gives us lots of negative feedback and requested by the users.

Since this PR microsoft/vscode#66451 is closed by VSCode side, provide the vscode java setting to mitigate this issue.

Signed-off-by: Yaohai Zheng <yaozheng@microsoft.com>

@fbricon fbricon left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

  • maybe we should display a warning on the 1st startup of the extension, asking the user to enable that feature
  • if the feature is disabled globally, then you manually enable it, the files are not excluded until the next vscode restart.

Comment thread package.json Outdated
"java.configuration.excludeProjectSettingFiles": {
"type": "boolean",
"default": true,
"description": "Exclude the extension generated project setting files from file explorer.",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

settings from the file explorer

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

need to explain setting it to false won't remove the exclusions

Comment thread src/setting.ts Outdated
import { getJavaConfiguration } from './utils';


const DEFAULT_HIIDEN_FILES: string[] = ['**/.classpath', '**/.project', '**/.settings'];

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

HIDDEN

Comment thread package.json Outdated
"description": "Specifies the severity of the message when the classpath is incomplete for a Java file",
"scope": "window"
},
"java.configuration.excludeProjectSettingFiles": {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

hideProjectSettingsFiles

Comment thread src/setting.ts Outdated
});
}

export function excludeProjectSettingFiles(workspaceUri: Uri) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

excludeProjectSettingsFiles

@fbricon

fbricon commented Jan 26, 2019

Copy link
Copy Markdown
Collaborator
	"files.exclude": {
        "**/.git": true,
        "**/.svn": true,
        "**/.hg": true,
        "**/CVS": true,
        "**/.DS_Store": true,
        "**/.classpath": true,
        "**/.project": true,
        "**/.settings": true
    }

is added to the local settings.json, even if the java exclusions are already defined at the user settings level. That's a big no no

@yaohaizh

yaohaizh commented Jan 30, 2019

Copy link
Copy Markdown
Collaborator Author

@fbricon Addressed the review feedback.

Signed-off-by: Yaohai Zheng <yaozheng@microsoft.com>

@fbricon fbricon left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

s/setting/settings/g

Comment thread src/setting.ts Outdated
@@ -0,0 +1,69 @@
'use strict';

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

settings.ts

@fbricon

fbricon commented Jan 30, 2019

Copy link
Copy Markdown
Collaborator

you also need to hide .factorypath

@fbricon

fbricon commented Jan 30, 2019

Copy link
Copy Markdown
Collaborator

if your settings contains

    "files.exclude": {
        "**/.classpath": true,
        "**/.settings": true
    }

agreeing to hide the project settings will change the settings to

    "files.exclude": {
        "**/.project": true
    }

@yaohaizh

yaohaizh commented Jan 30, 2019

Copy link
Copy Markdown
Collaborator Author

For setting generated, if user choose the workspace or global, the settings is split. Should we always keep them the same place? I would let the user to make the decision here.

Signed-off-by: vscjava <vscjava@vscjavas-MacBook-Pro.local>
@yaohaizh

Copy link
Copy Markdown
Collaborator Author

@fbricon Updated

@fbricon

fbricon commented Jan 30, 2019

Copy link
Copy Markdown
Collaborator

How about we change the choices to

  • Exclude globally
  • Exclude in workspace
  • Never

If files exclusions exists in the workspace preferences, then we don't propose to "Exclude globally"

Comment thread src/settings.ts Outdated
let needUpdate = false;
for (const hiddenFiles of DEFAULT_HIDDEN_FILES) {
if (!excludedValue.hasOwnProperty(hiddenFiles)) {
needExcludeFiles[hiddenFiles] = true;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

no, this will always store a subset of the files, overriding existing settings.
We do want to merge excludedValue + DEFAULT_HIDDEN_FILES in config.get('exclude')

Comment thread src/settings.ts
if (!params.affectsConfiguration('java')) {
return;
}
let newConfig = getJavaConfiguration();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

we want to check that if configuration.excludeProjectSettingsFiles is true, but it was false before, then excludeProjectSettingsFiles should be called.

Be aware that oldConfig is only changed if the "java configuration" changed, so it makes things a bit trickier.

@yaohaizh

yaohaizh commented Jan 31, 2019

Copy link
Copy Markdown
Collaborator Author

How about we change the choices to

  • Exclude globally
  • Exclude in workspace
  • Never

If files exclusions exists in the workspace preferences, then we don't propose to "Exclude globally"

Would keep the term simple and consistent with other messages in the similar situation.

@fbricon

fbricon commented Jan 31, 2019

Copy link
Copy Markdown
Collaborator

I'm fine with having better labels, but they need to be explicit

@yaohaizh

Copy link
Copy Markdown
Collaborator Author

@fbricon Updated PR.

@fbricon

fbricon commented Jan 31, 2019

Copy link
Copy Markdown
Collaborator

@yaohaizh I've renamed the new preference to better reflect its role:
java.configuration.checkProjectSettingsExclusions

Signed-off-by: Yaohai Zheng <yaozheng@microsoft.com>
@fbricon fbricon merged commit cffba7e into master Jan 31, 2019
@yaohaizh yaohaizh deleted the yaohai_dev branch March 19, 2019 02:51
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.

2 participants