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

Update type check for TreeContainerProps #10881

Conversation

colin-grant-work
Copy link
Contributor

@colin-grant-work colin-grant-work commented Mar 14, 2022

What it does

The previous implementation of the check to distinguish TreeProps from TreeContainerProps could be confused by the fact that both had a search key. This PR introduces special handling of that field to ensure that the check returns the correct value.

How to test

  1. Assert that the new test introduced in this PR passes.
  2. use createTreeContainer with TreeProps that include a search field. Observe that the tree view is successfully constructed and functions correctly.

e.g. in outline-view-frontend-module.ts replace this:

    const child = createTreeContainer(parent, {
         props: { expandOnlyOnExpansionToggleClick: true, search: true },
         widget: OutlineViewWidget,
         model: OutlineViewTreeModel,
         decoratorService: OutlineDecoratorService,
     });

with this:

    const child = createTreeContainer(parent, { ...defaultTreeProps, expandOnlyOnExpansionToggleClick: true, search: true });


     child.unbind(TreeWidget);
     child.bind(OutlineViewWidget).toSelf();

     child.unbind(TreeModelImpl);
     child.bind(OutlineViewTreeModel).toSelf();
     child.rebind(TreeModel).toService(OutlineViewTreeModel);

     child.bind(OutlineDecoratorService).toSelf().inSingletonScope();
     child.rebind(TreeDecoratorService).toDynamicValue(ctx => ctx.container.get(OutlineDecoratorService)).inSingletonScope();

Review checklist

Reminder for reviewers

@colin-grant-work colin-grant-work added bug bugs found in the application tree issues related to the tree (ex: tree widget) labels Mar 14, 2022
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I confirmed with the changes that the outline-view can successfully be constructed and viewed (the search does not work, but it does without the changes to the outline-view itself).

packages/core/src/browser/tree/tree-container.spec.ts Outdated Show resolved Hide resolved
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The changes look good to me 👍
I confirmed that the new code works well, the outline-view works including the snippet, and so does searching in the trees.

Comment on lines 15 to 16
// *****************************************************************************
import * as assert from 'assert';
Copy link
Member

Choose a reason for hiding this comment

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

minor: add a newline after the header.

@colin-grant-work colin-grant-work merged commit 817fdee into eclipse-theia:master Mar 17, 2022
@colin-grant-work colin-grant-work deleted the bugfix/better-type-check branch March 17, 2022 20:34
@colin-grant-work colin-grant-work added this to the 1.24.0 milestone Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug bugs found in the application tree issues related to the tree (ex: tree widget)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants