[preferences] use the editor manager instead of reimplementing it #2615
Conversation
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
akosyakov
force-pushed
the
ak/preferences_editor
branch
from
7086923
to
2d852fd
Compare
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
akosyakov
changed the title
|
@elaihau I've refactored the workspace service since it was bogusly mutating @marechal-p it should fix gitpod-io/gitpod#21 |
| this.rootWatchers[validRoot.uri] = watcher; | ||
| protected readonly watchers = new Map<string, Disposable>(); | ||
|
|
||
| protected async watchRoots(): Promise<void> { |
There was a problem hiding this comment.
does this function stop all the watchers, and start a subset of them over again, when users add/remove roots? do we want to only deal with the diff ?
There was a problem hiding this comment.
does this function stop all the watchers, and start a subset of them over again, when users add/remove roots?
it stops a watcher only if a root is removed
| return { stat, roots: this._workspaceFolder && roots.length === 0 ? [this._workspaceFolder.uri] : roots }; | ||
| // FIXME use jsonc + ajx to parse & validate content | ||
| const roots: string[] = JSON.parse(content).roots || []; | ||
| return { stat, roots }; |
There was a problem hiding this comment.
in the case that roots == [ ], all watchers are disposed - at this point if the user adds a new root to the workspace, s/he wouldn't see that new root, because the workspace config file is not being watched anymore.
but it is ok - my PR in review makes the config file independent from the root folders, so it is not going to be an issue.
There was a problem hiding this comment.
- a config file is watched always independently: https://github.com/theia-ide/theia/pull/2615/files/3dc813afbc0133fe3099c4eb4b60d2fa23dff3f9#diff-70a982fe47d80b1a0ac76c9c84fd5a4bR105
- if roots
=== [], then a workspace folder is used as a root: https://github.com/theia-ide/theia/pull/2615/files/3dc813afbc0133fe3099c4eb4b60d2fa23dff3f9#diff-70a982fe47d80b1a0ac76c9c84fd5a4bR119
| this._roots = await this.computeRoots(); | ||
| this.deferredRoots.resolve(this._roots); // in order to resolve first | ||
| this.deferredRoots = new Deferred<FileStat[]>(); | ||
| this.deferredRoots.resolve(this._roots); |
There was a problem hiding this comment.
could you please explain why we need to resolve it twice ? Thank you !
There was a problem hiding this comment.
The first resolve is only needed to resolve a promise created in the constructor, otherwise, it won't be ever resolved. I don't like it much either.
I don't see it much in other APIs. Usually, I use a promise if a single action should be awaited, but if there is a value which changes over the time, like _roots then synchronous access + an event is usually enough. We could analyze clients to see whether we can get rid of a promise.
The internal contract between the editor manager and the editor was changed, but the preference widget relying on it was not.
This PR refactors the preferences widget to rely on the editor manger instead of its implementation details.