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.

Sign up for GitHub

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
Closed

9075-Fix fetching of values from workspace config #9080

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d10e680
Fix for getting value from workspace preference
balajiv113 Mar 1, 2021
3a6c4ea
Fix for onDidChange not fired for view/title when change
balajiv113 Mar 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/monaco/src/browser/monaco-frontend-module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: string | undefined = overrides && 'resource' in overrides && !!overrides['resource'] ? overrides['resource'].toString() : undefined;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const proxy = createPreferenceProxy<{ [key: string]: any }>(preferences, preferenceSchemaProvider.getCombinedSchema(), {
resourceUri, overrideIdentifier, style: 'both'
Expand Down
8 changes: 3 additions & 5 deletions packages/plugin-ext/src/main/browser/menus/menus-contribution-handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,14 +123,12 @@ export class MenusContributionPointHandler {
if (!action.command) {
continue;
}
toDispose.push(this.registerTitleAction(location, { ...action, when: undefined }, {
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)),
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?

isVisible: widget => widget instanceof PluginViewWidget &&
this.viewContextKeys.with({ view: widget.options.viewId }, () =>
this.commands.isVisible(action.command!) && this.viewContextKeys.match(action.when))
this.commands.isVisible(action.command!) && this.viewContextKeys.match(action.when)
}));
}
} else if (location === 'view/item/context') {
Expand Down
3 changes: 3 additions & 0 deletions packages/plugin-ext/src/main/browser/view/plugin-view-registry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

this.viewContextKeys.view.set(viewId);
});
// if a view is explicitly hidden then suppress updating visibility based on `when` closure
part.onDidChangeVisibility(() => widget.suppressUpdateViewVisibility = part.isHidden);

Expand Down