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

Provide API to filter unwanted contributions #17

Closed
wants to merge 55 commits into from
Closed

Conversation

tortmayr
Copy link

@tortmayr tortmayr commented Mar 22, 2021

What it does

Adds the ContributionFilterRegistry which can be used to filter contributions of Theia extensions before they are bound.

This mechanism can be used by application developers to specifically filter individual contributions from Theia extensions they have no control over, i.e. core or 3rd party extensions.

Resolves eclipse-theia#9069

Contributed on behalf of STMicroelectronics
Signed-off-by: Tobias Ortmayr [email protected]

How to test

  • You can take a look at the included test case, or
  • Contribute a ContributionFilter in an example Theia application

Review checklist

Reminder for reviewers

Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

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

Looks good overall.

Some generic comments:

  • We need to clarify which copyright notice we want to use for new files
  • I would like to see more documentation for each class / function / interface
  • I think the offered filters are good in general, I wonder if we could go with a more generalized approach. What do you think?

packages/core/src/common/contribution-filter/filter.ts Outdated Show resolved Hide resolved
packages/core/src/common/contribution-filter/filter.ts Outdated Show resolved Hide resolved
packages/core/src/common/contribution-filter/filter.ts Outdated Show resolved Hide resolved
packages/core/src/common/contribution-filter/filter.ts Outdated Show resolved Hide resolved
packages/core/src/common/contribution-filter/filter.ts Outdated Show resolved Hide resolved
packages/core/src/common/contribution-filter/filter.ts Outdated Show resolved Hide resolved
packages/core/src/common/contribution-filter/filter.ts Outdated Show resolved Hide resolved
@tortmayr tortmayr force-pushed the filter-api branch 3 times, most recently from b8d8ab2 to ef10221 Compare April 7, 2021 05:51
@tortmayr
Copy link
Author

tortmayr commented Apr 7, 2021

Thanks for the review.
@sdirix I addressed your comments/issues and also added a test for the contribution filter and prepared a changelog update.
While revisiting the FilterAPI I noticed that the IdBasedContributionFilter was actually dead code from the POC that we migrated on accident so I removed it.

@tortmayr tortmayr requested a review from sdirix April 7, 2021 05:53
@tortmayr tortmayr force-pushed the filter-api branch 2 times, most recently from 4ce9258 to 4b66222 Compare April 7, 2021 07:30
CHANGELOG.md Outdated
@@ -1,5 +1,12 @@
# Change Log

## v1.13.0

- [core] add API to filter contributions at runtime #PR Contributed on behalf of STMicroelectronics [#XXXX](https://github.com/eclipse-theia/theia/pull/XXXX)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- [core] add API to filter contributions at runtime #PR Contributed on behalf of STMicroelectronics [#XXXX](https://github.com/eclipse-theia/theia/pull/XXXX)
- [core] add API to filter contributions at runtime [#XXXX](https://github.com/eclipse-theia/theia/pull/XXXX) Contributed on behalf of STMicroelectronics

@@ -15,20 +15,22 @@
********************************************************************************/

import { ContainerModule } from '@theia/core/shared/inversify';
import '../../src/browser/style/branding.css';
Copy link
Member

Choose a reason for hiding this comment

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

We should avoid reordering all the imports

Comment on lines 51 to 52
get(type: ContributionType): ContributionFilter[] {
const genericFilters = this.registry.get(GENERIC_CONTRIBUTION_FILTER_KEY) || [];
const filters = this.registry.get(type) || [];
filters.push(...genericFilters);
return filters;
}
Copy link
Member

Choose a reason for hiding this comment

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

If type === GENERIC_CONTRIBUTION_FILTER then we return them twice, correct?

return filters;
}

applyFilters<T extends Object>(toFilter: T[], type: ContributionType): T[] {
Copy link
Member

Choose a reason for hiding this comment

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

  • Should we make type optional by setting it to GENERIC_CONTRIBUTION_FILTER_KEY by default?
  • We should add documentation that the given type of filters as well as the generic filters will be executed


/**
* Specialized `Filter` that is used by the `ContainerBasedContributionProvider` to
* filter out unwanted contributions that are already bound in the DI container.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* filter out unwanted contributions that are already bound in the DI container.
* filter unwanted contributions that are already bound in the DI container.

Comment on lines 18 to 22
/**
* A `Filter` can be used to test wether a given object
* should be filtered out/removed from a set of objects. The `test` function
* can be applied to an object of matching type and returns true if the object
* should be filtered out.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/**
* A `Filter` can be used to test wether a given object
* should be filtered out/removed from a set of objects. The `test` function
* can be applied to an object of matching type and returns true if the object
* should be filtered out.
*/
/**
* A `Filter` can be used to test whether a given object should be filtered
* from a set of objects. The `test` function can be applied to an object
* of matching type and returns `true` if the object should be filtered out.
*/

Comment on lines 25 to 29
/**
* Evaluates this filter on the given argument
* @param toTest Object that should be tested
* @returns `true` if the object should be filtered out, `false` otherwise
*/
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/**
* Evaluates this filter on the given argument
* @param toTest Object that should be tested
* @returns `true` if the object should be filtered out, `false` otherwise
*/
/**
* Evaluates this filter on the given argument.
* @param toTest Object that should be tested
* @returns `true` if the object should be filtered out, `false` otherwise
*/

Comment on lines 33 to 31
/**
* Applies a set of filters to a set of given objects and returns the set of filtered objects
* @param toFilter Set of objects which should be filtered.
* @param filters Set of filters that should be applied.
* @param negate Negation flag. If set to true the result of all Filter.test methods is negated.
* @returns The set of filtered arguments.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/**
* Applies a set of filters to a set of given objects and returns the set of filtered objects
* @param toFilter Set of objects which should be filtered.
* @param filters Set of filters that should be applied.
* @param negate Negation flag. If set to true the result of all Filter.test methods is negated.
* @returns The set of filtered arguments.
*/
/**
* Applies a set of filters to a set of given objects and returns the set of filtered objects.
* @param toFilter Set of objects which should be filtered
* @param filters Set of filters that should be applied
* @param negate Negation flag. If set to true the result of all `Filter.test` methods is negated
* @returns The set of filtered arguments
*/

abstract toString(toTest: T): string | undefined;

/**
* Tests wether the object should be filtered out based on its string representation.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Tests wether the object should be filtered out based on its string representation.
* Tests whether the object should be filtered out based on its string representation.

export interface ContributionFilter extends Filter<Object> {
/**
* Contribution types for which this filter is applicable. If `undefined` or empty this filter
* will be applied to all contribution types.
Copy link
Member

Choose a reason for hiding this comment

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

We should give an example contribution type as it's hard to know what exactly is expected here.

@sdirix
Copy link
Member

sdirix commented Apr 7, 2021

@tortmayr I applied the code review myself. It's almost only doc changes, however I changed the functionality in the registry getter. Can you recheck that code?

@tortmayr
Copy link
Author

tortmayr commented Apr 8, 2021

Thanks for the fixes. Looks good to me now. 👍

@sdirix sdirix force-pushed the filter-api branch 3 times, most recently from bb1bdd5 to deeaa16 Compare April 8, 2021 09:19
@tortmayr tortmayr changed the title [WIP] Provide API to filter unwanted contributions Provide API to filter unwanted contributions Apr 8, 2021
@tortmayr tortmayr force-pushed the filter-api branch 4 times, most recently from 4ae3ca5 to fd50b21 Compare April 9, 2021 07:36
paul-marechal and others added 8 commits May 4, 2021 10:58
Signed-off-by: Paul Maréchal <[email protected]>
What it does
- Updates styling of `rename-label` to align with VSCode.

Signed-off-by: seantan22 <[email protected]>
The following commit adds support for the preference 'search.smartCase'.
When the preference is true, search is case-insensitive if the pattern
is all lowercase, otherwise search is case-sensitive.

Signed-off-by: Duc Nguyen <[email protected]>
The following pull-request sanitizes the rendered `readme` of
extensions for security purposes. The sanitization is similar to other
areas of the code which make use of `dangerouslySetInnerHTML`.

Signed-off-by: vince-fugnitto <[email protected]>
process.env is not available when in a browser environment. Webpack most
likely included a shim for it.
vince-fugnitto and others added 10 commits May 28, 2021 09:47
The commit updates the `sanitize-html` dependency and typings to address
known moderate vulnerabilities. The updated dependency was verified for
being license compliant.

Signed-off-by: vince-fugnitto <[email protected]>
This changes the default goto line range from position 0 to undefined.

This will make it feasible to differentiate between the actual default
and a user input at location zero.

Signed-off-by: Alvaro Sanchez-Leon <[email protected]>
The commit updates the repository's `yarn.lock` following the `v1.14.0`
release to update dependencies to what downstream applications might
expect the first time they attempt to build a `theia`-based application.

Signed-off-by: vince-fugnitto <[email protected]>
The PluginViewWidget has a property to suppress visibility updates
from a `when` clause in the case a user specifically requests to hide
it. However when the browser got reloaded, the flag was reset to
default and did not keep the hide request from the user.

This change preserves the flag mentioned above in the PluginViewWidget
state, this state is preserved and then available for use in
`restoreState` where the flag is now updated to the state before the
browser reloads.

Signed-off-by: Alvaro Sanchez-Leon <[email protected]>
The BackendApplicationServer is an optional backend contribution that
serves frontend files. When not bound, the generators may bind one.

This component is useful when your application is packaged in such a way
that you need to customize how the frontend is served.
Signed-off-by: Igor Vinokur <[email protected]>
@tortmayr tortmayr force-pushed the filter-api branch 3 times, most recently from 6b65888 to 85a5423 Compare June 9, 2021 07:36
Colin Grant and others added 13 commits June 9, 2021 09:03
Signed-off-by: Colin Grant <[email protected]>
Rework how the different application configurations are defined to allow
partial setting when calling configuration providers.

Use default values when setting a partial configuration through the
configuration providers.
The commit adds support for `prefix` arguments when plugins execute the
`workbench.action.quickOpen` command. Previously `prefix` arguments were
ignored.

Signed-off-by: vince-fugnitto <[email protected]>
Signed-off-by: Kenneth Marut <[email protected]>
)

refactor preference ui + add commonly used section
The commit adds `libsecret` to the `gitpod` dockerfile which was
previously failing to start the application due to the new `keytar`
dependency.

Signed-off-by: vince-fugnitto <[email protected]>
The commit adds support for additional queries:
- `@builtin`
- `@installed`

The changes also include commands to open the `extensions-view` with the
corresponding query.

Signed-off-by: vince-fugnitto <[email protected]>
The commit includes the following:
- adds `dompurify` as a shared dependency from `@theia/core`
- bumps the `dompurify` dependency to the latest version.
- suppresses the sanitized content warnings for content which is known
  to be explicitly handled.

Signed-off-by: vince-fugnitto <[email protected]>
The cursor or the selection is now preserved when we close an editor
and when we use ctrlcmd + p to reopen it. It stays at the location where
we left it and doesn't end up at the beginning of the file like before.
Adds the ContributionFilterRegistry which can be used to filter
contributions of Theia extensions before they are bound.

This mechanism can be used by application developers to specifically
filter individual contributions from Theia extensions they have no
control over, i.e. core or 3rd party extensions.

Resolves eclipse-theia#9069

Contributed on behalf of STMicroelectronics

Signed-off-by: Tobias Ortmayr <[email protected]>
Co-Authored-By: Paul Maréchal <[email protected]>

squash me?

Signed-off-by: Paul Maréchal <[email protected]>
@sdirix
Copy link
Member

sdirix commented Nov 16, 2022

Merged upstream with eclipse-theia#9317

@sdirix sdirix closed this Nov 16, 2022
@sdirix sdirix deleted the filter-api branch May 16, 2024 15:59
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.

Provide API to filter unwanted contributions from frontend extensions