[workspace] Improved Recently Used Workspace Handling #809
Conversation
|
I am starting the review now... |
| export class WorkspaceService { | ||
| export class WorkspaceService implements FrontendApplicationContribution { | ||
|
|
||
| root: FileStat | undefined; |
There was a problem hiding this comment.
I do not understand why do we have a public property here and another public property getter, tryRoot too. Perhaps, you can explain this to me. The tryRoot at least validates whether the root FileStat (if set) is valid or not.
There was a problem hiding this comment.
it should not be public
| } | ||
|
|
||
| /** | ||
| * Unlike [root](WorkspaceService.root), this method gets resolved even if the workspace root URI is not yet set on the backend or if it | ||
| * is invalid. In such cases, it will resolve to `undefined`. | ||
| */ | ||
| get tryRoot(): Promise<FileStat | undefined> { | ||
| if (this.root) { | ||
| return Promise.resolve(this.root); | ||
| } |
There was a problem hiding this comment.
Here, the rest of this property getter never sets the root property which is good because it does not have a side-effect, but then the this.root always remains undefined.
Could you please clean up the documentation, I assume it is not valid anymore.
There was a problem hiding this comment.
It is set in the constructor, but your question tells me that I should move it here :)
There was a problem hiding this comment.
Thanks! I believe that would be cleaner. Otherwise, the rest of the code in this getter is just unused.
|
Another thing that might be interesting; if I have two completely different bundled application, for instance, Yangster and something else. They will override each others last WS configuration in the user's home folder. Perhaps, we do not have to provide an exact solution for that in the context of this task. |
Good point. Could you create a ticket for that? |
|
svenefftinge
force-pushed
the
workspace_improvements
branch
from
de0c648 to
57e4db4
Compare
|
@kittaakos I addressed your feedback and tested again. Good to merge? |
|
Please fix these too: |
Signed-off-by: Sven Efftinge <sven.efftinge@typefox.io>
Signed-off-by: Sven Efftinge <sven.efftinge@typefox.io>
svenefftinge
force-pushed
the
workspace_improvements
branch
from
57e4db4 to
c6adddd
Compare
… home directory and use it when the backend is started without an explicit root argument. Signed-off-by: Sven Efftinge <sven.efftinge@typefox.io>
svenefftinge
force-pushed
the
workspace_improvements
branch
from
c6adddd to
c3152ca
Compare
With this PR, the recently used workspace root is updated when a frontend unloads, such that the next opened frontend app will use that again.
Also the most recently used workspace root is persisted on the backend's file system (user home), wuch that it can be used, in case no argument is provided on start.