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

9075-Fix fetching of values from workspace config #9080

Closed
wants to merge 2 commits into from

Conversation

balajiv113
Copy link
Contributor

@balajiv113 balajiv113 commented Feb 17, 2021

What it does

The changes fixes #9075.
The root cause of the issue is resourceUri is set as false if resource is not present in overrides.

const resourceUri = overrides && 'resource' in overrides && !!overrides['resource'] && overrides['resource'].toString();

Due to this preference proxy is returning the default value which is flat.

How to test

Setup
Add the change from #8967 for reloading toolbar based on ToolbarItem.onDidChange.

Test 1 - To test if toolbar items are reloaded on configuration change
Follow the steps mentioned under the issue #9075

Test 2 - To test if toolbar items are reloaded on context change
Follow the steps mentioned under the issue #8757

Review checklist

  • as an author, I have thoroughly tested my changes and carefully followed the review guidelines
  • Update workspace config using preference UI and notice icon is getting updated
  • Clear maven.view from workspace config and update maven.view in user config and notice the behaviour is working fine

Reminder for reviewers

@balajiv113 balajiv113 mentioned this pull request Feb 17, 2021
1 task
@colin-grant-work
Copy link
Contributor

colin-grant-work commented Feb 23, 2021

I confirm that this fixes the problem described.

Some details for testing: The problem occurs specifically in a single-root workspace. Values from workspace (= folder) scope are not correctly evaluated in this case. In a multi-root workspace, it appears that master already shows correct behavior in workspace scope (and this PR does not change that).

@@ -155,7 +155,7 @@ export function createMonacoConfigurationService(container: interfaces.Container

_configuration.getValue = (section, overrides) => {
const overrideIdentifier = overrides && 'overrideIdentifier' in overrides && overrides['overrideIdentifier'] as string || undefined;
const resourceUri = overrides && 'resource' in overrides && !!overrides['resource'] && overrides['resource'].toString();
const resourceUri = overrides && 'resource' in overrides && !!overrides['resource'] ? overrides['resource'].toString() : undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a declaration on resourceUri here specifying that it should be string | undefined so that we don't get caught assigning a different type (false) to it in the future?

@balajiv113
Copy link
Contributor Author

@colin-grant-work - I have provided the fix for firing toolbar item onDidChange when command registered under view/title. This in combination with your PR #8967 will fix the reload issues in toolbar.

Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

The code looks fine. I have a couple of questions just so I understand better what's going on. Could you update the testing instructions to outline the scenarios you're handling so that it's easier to confirm that those scenarios are handled and think about whether there are any that are unhandled?

@@ -465,6 +465,9 @@ export class PluginViewRegistry implements FrontendApplicationContribution {
}
const part = containerWidget.getPartFor(widget);
if (part) {
part.node.addEventListener('mouseenter', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain the reasoning for setting this value on mouseenter? mouseenter doesn't generally correspond to other definitions of widget focus. What scenario does this account for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is mainly to resolve the issue mentioned in below comment. As mentioned, the viewId now will not be updated as part of isVisible. So instead we would need a place to update viewContextKeys on view hover (toolbar items are shown on hover).

Copy link
Contributor

Choose a reason for hiding this comment

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

I see what you mean, thanks for the explanation. Should this stay here and only apply to plugins, or could this also be a problem for 'native' view containers, so that it should be part of the code for the ViewContainer or ViewContainerPart classes? Are there any side effects to setting the viewID in this way - for example will the setting persist longer than intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the isVisible clearly bound to PluginViewWidget (this was the place previously view context been updated) that's the reason i added specific to plugins. If needed i can look into making generic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding side effects will do a further in-depth round of testing. Till now i haven't found any.

execute: widget => widget instanceof PluginViewWidget && this.commands.executeCommand(action.command!),
isEnabled: widget => widget instanceof PluginViewWidget &&
this.viewContextKeys.with({ view: widget.options.viewId }, () =>
this.commands.isEnabled(action.command!) && this.viewContextKeys.match(action.when)),
this.commands.isEnabled(action.command!) && this.viewContextKeys.match(action.when),
Copy link
Contributor

Choose a reason for hiding this comment

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

These lines seem almost equivalent. What was going wrong before with the .with check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After removing the when: undefined. On contextKey change (eg: view change) now toolBarItem.onDidChange will be fired (This will not happen before).

Due to this there will be lot of updateToolbar being called. isVisible will be called for every command on focus.
This will in-turn update the context key and fire toolbar update.

Also the final UI was not correct, the toolbar items were not visible on focus, rather was available only after entering into the view.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have experimented with modifying these lines to look like this:

                    toDispose.push(this.registerTitleAction(location, action, {
                        execute: widget => widget instanceof PluginViewWidget && this.commands.executeCommand(action.command!),
                        isEnabled: widget => widget instanceof PluginViewWidget &&
                            this.viewContextKeys.with({ view: widget.options.viewId }, () =>
                                this.commands.isEnabled(action.command!) && this.viewContextKeys.match(action.when)),
                        isVisible: widget => widget instanceof PluginViewWidget &&
                            this.viewContextKeys.with({ view: widget.options.viewId }, () =>
                                this.commands.isVisible(action.command!) && this.viewContextKeys.match(action.when))
                    }));

and that seems to work - the scenarios described for testing have the correct behavior - and there are no shadows. It seems that what this achieves is that the widget used to check visibility is always the correct widget, and the context-key change onmouseenter happens in the background to ensure that the toolbars update appropriately. Does that seem correct to you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes looks correct. My only concern is on the recursive events fired due to setting of context keys on isVisible or isEnabled.

Consider the following scenario like,
Commands available - npm reload (view: npm), maven flat (view: maven)
This isVisible or isEnabled is going to be called for both these commands.
Previously we didn't fire onDidChange event so there was no issues in rendering.
But now since we have onDidChange first a toolbar update will be fired for setting npm view and then for maven view. This will cause multiple re-renders and will increase with number of commands available.

From debugging, this is what i found. Do correct me if my understanding is wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm... it seems to me that we shouldn't fire an event or set a context key because an event just because a particular check (isVisible or isEnabled) is run. Thing should run like this

Context changes in the application
  causes: context key changes
    causes: context key service fires event
      causes: TabbarToolbarRegistry recalculates visibility given new context
        causes: TabbarToolbarRegistry fires didChange
          causes: Toolbars update

And the flow should stop there. The tabbar toolbar registry updating shouldn't cause a context key change event, and the toolbars updating shouldn't cause a toolbar registry change event. If we have a situation where these events are looping into one another, we probably need to do a deeper adjustment of the logic. Have you been able to track down what causes the looping behavior you describe?

@balajiv113
Copy link
Contributor Author

@colin-grant-work - Updated the description with proper test instructions and scenarios that are being covered as part of this PR.

@colin-grant-work
Copy link
Contributor

colin-grant-work commented Mar 2, 2021

This appears to be working more or less as intended. I am seeing occasional shadows where moving the mouse from one view part (e.g. the Maven widget) to another (e.g. tal-sapan's demo plugin view) results in the icons for Maven briefly showing up on the toolbar for the Test View, presumably before the onmouseenter context key change propagates to the toolbar. It isn't consistent, and it's very brief, but it might be good to think about alternatives - the shadows never occur on master. I will see if I can find a solution (here's my suggestion). In the meantime, please also squash your commits.

@EstherPerelman
Copy link
Contributor

@balajiv113 do you mind finish this PR? Is there anything that stopping you?

@colin-grant-work
Copy link
Contributor

@balajiv113, if you're not interested in finishing or are unable to take it on, I'm happy to finish up the PR for you. Let me know what you prefer.

cc. @EstherPerelman

@balajiv113
Copy link
Contributor Author

Hi @colin-grant-work , @EstherPerelman
Apologies, Unfortunately i won't be able to complete as of now. Please feel free to take it up if critical.

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.

Icons (and Their Matching Commands ) of view/title Do not Update Upon Changes in Workspace settings.json
3 participants