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

files.enableTrash preference #4766

Merged
merged 2 commits into from
Apr 2, 2019
Merged

files.enableTrash preference #4766

merged 2 commits into from
Apr 2, 2019

Conversation

akosyakov
Copy link
Member

fix #4524: add files.enableTrash preference
fix #4334: allow to override default preference values:

@akosyakov
Copy link
Member Author

theia-ide/theia-apps#159 to reconfigure docker image cc @marcdumais-work

@@ -22,8 +22,6 @@ import { PreferenceChangeEvent } from '@theia/core/lib/browser/preferences';
import { FileSystemPreferences, FileSystemConfiguration } from '@theia/filesystem/lib/browser/filesystem-preferences';
import { FileNavigatorPreferences, FileNavigatorConfiguration } from './navigator-preferences';

const FILES_EXCLUDE_PREFERENCE: keyof FileSystemConfiguration = 'files.exclude';
Copy link
Member Author

Choose a reason for hiding this comment

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

fyi: not all strings in TS are just string, some can be used for property access, i.e myObject['myProp']
preference proxies are statically typed, one cannot use other prop names and type is properly determined for a prop, there is NO need for a constant. I had to remove it here because it confused compiler. The type of FILES_EXCLUDE_PREFERENCE now is 'files.exclude' | 'files.enableTrash', so compiler cannot derive any more a proper type where FILES_EXCLUDE_PREFERENCE is used.

cc @svor @kittaakos

@thegecko
Copy link
Member

thegecko commented Apr 1, 2019

Hmm, getting the following error on this branch. Investigating....

Screen Shot 2019-04-01 at 11 53 09

@akosyakov
Copy link
Member Author

@thegecko I've annotated the proxy factory as injectable to allow its subclassing in DI context. It seems to be used in process which does not load reflect metadata package.

@thegecko
Copy link
Member

thegecko commented Apr 1, 2019

Ok, have you tried running this in electron? The js runtimes are quite restrictive

@akosyakov
Copy link
Member Author

@thegecko good point, i will try

@akosyakov
Copy link
Member Author

it's again because of cluster module. It uses proxy factories without installing reflect metadata package. I will move annotating proxy factories to browser module for now to make this PR work and then look into getting rid of cluster module.

@akosyakov
Copy link
Member Author

@thegecko could you try again? It should work now

@akosyakov akosyakov requested a review from AlexTugarev April 2, 2019 04:55
Copy link
Member

@thegecko thegecko left a comment

Choose a reason for hiding this comment

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

Perfect!

I've successfully tested the settings override by using the following to open workspaces in the same window:

...
"preferences": {
    "workspace.preserveWindow": true
}
...

Nice work!

Copy link
Contributor

@AlexTugarev AlexTugarev left a comment

Choose a reason for hiding this comment

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

Works nicely!
Thanks for the preference feature! 🎉

@AlexTugarev
Copy link
Contributor

@akosyakov, what's the issue with DCO check?

@akosyakov
Copy link
Member Author

@AlexTugarev i also wonder what is wrong with it. Eclipse sign-off check is green, so it's good to merge?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants