Fix registration for the TreeView containers

[olexii4]

What does this PR do?

Fix registration for the TreeView containers.

What issues does this PR fix or reference?

#3907

Signed-off-by: Oleksii Orel oorel@redhat.com

[vitaliy-guliy]

@olexii4 as we discussed earlier, there is a pretty good solution using promise.
It helps to cover two cases described in #3907 and gives the ability to register TreeDataProvider and open TreeView at any time.

I don't remember exactly (need to test again), but it seems issues with ShellLayoutRestorer made me to add handling the ready state like https://github.com/theia-ide/theia/blob/master/packages/plugin-ext/src/main/browser/view/view-registry.ts#L48.

WDYT about this https://github.com/theia-ide/theia/compare/fix-tree-view ?

[olexii4]

@vitaliy-guliy
I don't think so. I did everything that was described in the description of the issue. Your solution still depends on application status ready.
In my solution, I improved behavior with activateWidget after adding. Add the possibility to remember state. Remove some timers which were duplicated.

[mmorhun]

I have small remarks. If I am not mistaken, view container (or tree view itself not sure which one) is shown automatically. This could be pretty annoying (like in case with containers plug-in). So if it is not hard and not time consuming, will be nice to have this configurable (to show by default or not).
Also we may close view container and there is no way (except page reload) to restore view container. Maybe we should add each view container into View menu? What do you thinks? (it could be another issue).
cc @evidolob

[vitaliy-guliy]

@mmorhun it's another issue. And it looks like improvement. You can create issue for that.

[sunix]

@vitaliy-guliy @olexii4 Could you describe both proposals with pros and cons ?

[vitaliy-guliy]

@sunix Oleksii will split fix and refactoring on two PRs.

[olexii4]

@vitaliy-guliy I've added only containers registration changes. WDYT?

[vitaliy-guliy]

I think we can delete pendingReady() method and use this.applicationStateService.reachedState('ready') directly.
When the state is already ready, it will return resolved promise. If the state still not ready, it will return a promise, which will be resolved when the state become ready.
So, the code become shorter.

        if (!this.pending) {
            this.pending = this.applicationStateService.reachedState('ready');
        }

[olexii4]

ok. Done

[sunix]

To me these lines should be refactored a bit. From an external point of view it is hard to understand what is this.pending. Maybe rename it ... but just pending doesn't mean anything to me.
Also, what if state 'ready' is already reached the first time ?

[olexii4]

Frontend application startup consists of the following steps:

• start frontend contributions
• attach the application shell to the host element
• initialize the application shell layout
• reveal the application shell if it was hidden by a startup indicator (state 'ready')

If state 'ready' is already reached the first time will be returned a new promise (deferred.promise)
https://github.com/theia-ide/theia/blob/master/packages/core/src/browser/frontend-application-state.ts#L61
https://github.com/theia-ide/theia/blob/master/packages/core/src/common/promise-util.ts#L26

[olexii4]

Done

[vitaliy-guliy]

Looks good. Thanks!

[sunix]

I approve it because it is working and fix the issue but I think this part deserve a deeper refactoring. I have some doubt about the behaviour if multiple TreeView get registered.

[olexii4]

Done

[olexii4]

Done

[sunix]

Thanks a lot !