Address review comments

- Add documentation
- Generialize Stringbased filter by using functions + Utility functions
- Remove IdBasedFilter (which cannot be used for contributions)
- Fix typos
- Add api test

examples/api-samples/src/browser/api-samples-frontend-module.ts

@@ -14,28 +14,23 @@
  * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
  ********************************************************************************/
 
-import { ContributionFilter, NameBasedContributionFilter } from '@theia/core/lib/common';
 import { ContainerModule } from '@theia/core/shared/inversify';
 import '../../src/browser/style/branding.css';
+import { bindSampleFilteredCommandContribution } from './contribution-filter/sample-filtered-command-contribution';
 import { bindSampleFileWatching } from './file-watching/sample-file-watching-contribution';
 import { bindDynamicLabelProvider } from './label/sample-dynamic-label-provider-command-contribution';
 import { bindSampleMenu } from './menu/sample-menu-contribution';
 import { bindSampleOutputChannelWithSeverity } from './output/sample-output-channel-with-severity';
 import { bindSampleUnclosableView } from './view/sample-unclosable-view-contribution';
 import { bindVSXCommand } from './vsx/sample-vsx-command-contribution';
 
-class TestContributionFilter extends NameBasedContributionFilter {
-    patterns = ['TerminalActiveContext', 'TerminalSearchVisibleContext', 'TerminalQuickOpenContribution', 'TerminalFrontendContribution'];
-    patterMatching = 'equals';
-}
-
 export default new ContainerModule(bind => {
     bindDynamicLabelProvider(bind);
     bindSampleUnclosableView(bind);
     bindSampleOutputChannelWithSeverity(bind);
     bindSampleMenu(bind);
     bindSampleFileWatching(bind);
     bindVSXCommand(bind);
-    bind(ContributionFilter).to(TestContributionFilter);
+    bindSampleFilteredCommandContribution(bind);
 });
 

examples/api-samples/src/browser/contribution-filter/sample-filtered-command-contribution.ts

@@ -0,0 +1,50 @@
+/********************************************************************************
+ * Copyright (C) 2021 STMicroelectronics and others.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License v. 2.0 which is available at
+ * http://www.eclipse.org/legal/epl-2.0.
+ *
+ * This Source Code may also be made available under the following Secondary
+ * Licenses when the conditions for such availability set forth in the Eclipse
+ * Public License v. 2.0 are satisfied: GNU General Public License, version 2
+ * with the GNU Classpath Exception which is available at
+ * https://www.gnu.org/software/classpath/license.html.
+ *
+ * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
+ ********************************************************************************/
+import { Command, CommandContribution, CommandRegistry, ContributionFilter, equals, NameBasedContributionFilter } from '@theia/core/lib/common';
+import { injectable, interfaces } from 'inversify';
+export namespace SampleFilteredCommand {
+    const EXAMPLE_CATEGORY = 'Examples';
+    export const FILTERED: Command = {
+        id: 'example_command.filtered',
+        category: EXAMPLE_CATEGORY,
+        label: 'This command should be filtered out'
+    };
+}
+
+@injectable()
+/**
+ * This sample command is used to test the runtime filtering of already bound contributions.
+ */
+export class SampleFilteredCommandContribution implements CommandContribution {
+
+    registerCommands(commands: CommandRegistry): void {
+        commands.registerCommand(SampleFilteredCommand.FILTERED, { execute: () => { } });
+    }
+}
+
+@injectable()
+export class SampleFilteredCommandContributionFilter extends NameBasedContributionFilter {
+
+    contributions = [CommandContribution];
+    doTest(toTest: string): boolean {
+        return equals(toTest, false, 'SampleFilteredCommandContribution');
+    }
+}
+
+export const bindSampleFilteredCommandContribution = (bind: interfaces.Bind) => {
+    bind(CommandContribution).to(SampleFilteredCommandContribution);
+    bind(ContributionFilter).to(SampleFilteredCommandContributionFilter);
+};

examples/api-tests/src/contribution-filter.spec.js

@@ -0,0 +1,36 @@
+/********************************************************************************
+ * Copyright (C) 2021 TypeFox and others.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License v. 2.0 which is available at
+ * http://www.eclipse.org/legal/epl-2.0.
+ *
+ * This Source Code may also be made available under the following Secondary
+ * Licenses when the conditions for such availability set forth in the Eclipse
+ * Public License v. 2.0 are satisfied: GNU General Public License, version 2
+ * with the GNU Classpath Exception which is available at
+ * https://www.gnu.org/software/classpath/license.html.
+ *
+ * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
+ ********************************************************************************/
+
+// @ts-check
+describe('Contribution filter', function () {
+    this.timeout(5000);
+    const { assert } = chai;
+
+    const { CommandRegistry, CommandContribution } = require('@theia/core/lib/common/command');
+    const { SampleFilteredCommandContribution, SampleFilteredCommand } = require('@theia/api-samples/lib/browser/contribution-filter/sample-filtered-command-contribution');
+
+    const container = window.theia.container;
+    const commands = container.get(CommandRegistry);
+
+    it('filtered command in container but not in registry', async function () {
+        const allCommands = container.getAll(CommandContribution);
+        assert.isDefined(allCommands.find(contribution => contribution instanceof SampleFilteredCommandContribution),
+            'SampleFilteredCommandContribution is not bound in container');
+        const filteredCommand = commands.getCommand(SampleFilteredCommand.FILTERED.id);
+        assert.isUndefined(filteredCommand, 'SampleFilteredCommandContribution should be filtered out but is present in "CommandRegistry"');
+    });
+
+});

packages/core/src/common/contribution-filter/contribution-filter-registry.ts

@@ -25,7 +25,7 @@ export class ContributionFilterRegistry {
     constructor(@multiInject(ContributionFilter) @optional() contributionFilters: ContributionFilter[] = []) {
         this.registry = new Map();
         contributionFilters.forEach(filter => {
-            if (!filter.contributions) {
+            if (!filter.contributions || filter.contributions.length === 0) {
                 this.addFilter(GENERIC_CONTRIBUTION_FILTER_KEY, filter);
             } else {
                 filter.contributions.forEach(type => {

packages/core/src/common/contribution-filter/contribution-filter.ts

@@ -14,22 +14,27 @@
  * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
  ********************************************************************************/
 
-import { Filter, IdBasedFilter, NameBasedFilter } from './filter';
 import { interfaces } from 'inversify';
+import { Filter } from './filter';
+import { NameBasedFilter } from './string-based-filter';
 export type ContributionType = interfaces.ServiceIdentifier<unknown>;
 
 export const ContributionFilter = Symbol('ContributionFilter');
+
+/**
+ * Specialized `Filter` that is used by the `ContainerBasedContributionProvider` to
+ * filter out unwanted contributions that are already bound in the DI container.
+ */
 export interface ContributionFilter extends Filter<Object> {
     /**
-     * contribution types for which this filter is applicable
+     * Contribution types for which this filter is applicable. If `undefined` or empty this filter
+     * will be applied to all contribution types.
      */
     contributions?: ContributionType[];
 }
 
-export abstract class IdBasedContributionFilter extends IdBasedFilter<Object> implements ContributionFilter {
-    contributions?: ContributionType[];
-}
-
+/**
+ * Specialized `ContributionFilter` that can be used to filter contributions based on their constructor name.
+ */
 export abstract class NameBasedContributionFilter extends NameBasedFilter<Object> implements ContributionFilter {
-    contributions?: ContributionType[];
 }

packages/core/src/common/contribution-filter/filter.ts

@@ -13,19 +13,30 @@
  *
  * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
  ********************************************************************************/
-import { injectable } from 'inversify';
 
 export const Filter = Symbol('Filter');
-
+/**
+ * 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.
+ */
 export interface Filter<T extends Object> {
     /**
      * 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
+     * @returns `true` if the object should be filtered out, `false` otherwise
      */
     test(toTest: T): Boolean;
 }
 
+/**
+ * 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.
+ */
 export function applyFilters<T extends Object>(toFilter: T[], filters: Filter<T>[], negate: boolean = false): T[] {
     if (filters.length === 0) {
         return toFilter;
@@ -36,65 +47,3 @@ export function applyFilters<T extends Object>(toFilter: T[], filters: Filter<T>
     });
 }
 
-export type PatternMatchingType = 'equals' | 'includes' | 'regex';
-
-@injectable()
-export abstract class StringBasedFilter<T extends Object> implements Filter<T> {
-
-    abstract patterns: string[];
-    patterMatchingType: PatternMatchingType = 'equals';
-    ignoreCase = false;
-
-    abstract toFilterString(toFilter: T): string | undefined;
-
-    protected doTest(filterStr: string): boolean {
-        const patterns = this.ignoreCase ? this.patterns.map(pattern => pattern.toLowerCase()) : this.patterns;
-        if (this.patterMatchingType === 'includes') {
-            return patterns.find(pattern => filterStr.includes(pattern)) !== undefined;
-        } else if (this.patterMatchingType === 'equals') {
-            return patterns.find(pattern => filterStr === pattern) !== undefined;
-        } else {
-            // eslint-disable-next-line no-null/no-null
-            return patterns.find(pattern => filterStr.match(new RegExp(pattern)) !== null) !== undefined;
-        }
-    }
-
-    test(contribution: T): boolean {
-        let filterStr = this.toFilterString(contribution);
-        if (!filterStr) {
-            return false;
-        }
-        if (this.ignoreCase) {
-            filterStr = filterStr.toLowerCase();
-        }
-        return this.doTest(filterStr);
-    }
-}
-
-@injectable()
-export abstract class NameBasedFilter<T extends Object> extends StringBasedFilter<T> {
-    toFilterString(toTest: T): string {
-        return toTest.constructor.name;
-    }
-}
-
-@injectable()
-export abstract class IdBasedFilter<T extends Object> extends StringBasedFilter<T> {
-    toFilterString(toTest: T): string | undefined {
-        if (Identifable.is(toTest)) {
-            return toTest.id;
-        }
-        return undefined;
-    }
-}
-
-export interface Identifable {
-    id: string;
-}
-
-export namespace Identifable {
-    // eslint-disable-next-line @typescript-eslint/no-explicit-any
-    export function is(object: any): object is Identifable {
-        return 'id' in object && typeof object['id'] === 'string';
-    }
-}

packages/core/src/common/contribution-filter/index.ts

@@ -17,3 +17,4 @@
 export * from './contribution-filter';
 export * from './contribution-filter-registry';
 export * from './filter';
+export * from './string-based-filter';

packages/core/src/common/contribution-filter/string-based-filter.ts

@@ -0,0 +1,106 @@
+/********************************************************************************
+ * Copyright (C) 2021 STMicroelectronics and others.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License v. 2.0 which is available at
+ * http://www.eclipse.org/legal/epl-2.0.
+ *
+ * This Source Code may also be made available under the following Secondary
+ * Licenses when the conditions for such availability set forth in the Eclipse
+ * Public License v. 2.0 are satisfied: GNU General Public License, version 2
+ * with the GNU Classpath Exception which is available at
+ * https://www.gnu.org/software/classpath/license.html.
+ *
+ * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
+ ********************************************************************************/
+
+import { injectable } from 'inversify';
+import { Filter } from './filter';
+
+/**
+ * Specialized `Filter` class for filters based on string level. Test objects
+ * are converted to a corresponding string representation with the help of the `toString()`-method
+ * which enable to implement a `doTest()`-method based on string comparison.
+ */
+@injectable()
+export abstract class StringBasedFilter<T extends Object> implements Filter<T> {
+
+    /**
+     * Converts the test object to a corresponding string representation
+     * that is then used in the `doTest()` method.
+     * @param toTest Test object that should be converted to string.
+     */
+    abstract toString(toTest: T): string | undefined;
+
+    /**
+     * Tests wether the object should be filtered out based on its string representation.
+     * @param testStr String representation of the test object.
+     * @returns `true` if the object should be filtered out, `false` otherwise
+     */
+    abstract doTest(testStr: string): boolean;
+
+    test(contribution: T): boolean {
+        const testStr = this.toString(contribution);
+        if (typeof testStr !== 'string') {
+            return false;
+        }
+
+        return this.doTest(testStr);
+    }
+}
+/**
+ * Utility function to test wether any of the given string patterns is included in the
+ * tested string.
+ * @param testStr String that should be tested.
+ * @param ignoreCase Flag, to indicate wether the test should be case-sensitive or not.
+ * @param patterns  The set of patterns that should be tested for inclusion
+ * @returns `true` if any of the given patterns is included in the test string, `false` otherwise
+ */
+export function includes(testStr: string, ignoreCase: boolean, ...patterns: string[]): boolean {
+    if (ignoreCase) {
+        testStr = testStr.toLowerCase();
+        patterns = patterns.map(pattern => pattern.toLowerCase());
+    }
+    return patterns.find(pattern => testStr.includes(pattern)) !== undefined;
+}
+
+/**
+ * Utility function to test wether any of the given string patterns is equal to the
+ * tested string.
+ * @param testStr String that should be tested.
+ * @param ignoreCase Flag, to indicate wether the test should be case-sensitive or not.
+ * @param patterns  The set of patterns that should be tested for equality
+ * @returns `true` if any of the given patterns is equal to the test string, `false` otherwise
+ */
+export function equals(testStr: string, ignoreCase: boolean, ...patterns: string[]): boolean {
+    if (ignoreCase) {
+        testStr = testStr.toLowerCase();
+        patterns = patterns.map(pattern => pattern.toLowerCase());
+    }
+    return patterns.find(pattern => testStr === pattern) !== undefined;
+
+}
+
+/**
+ * Utility function to test wether a string matches any of the given regular expression patterns.
+ * @param testStr String that should be tested.
+ * @param ignoreCase Flag, to indicate wether the test should be case-sensitive or not.
+ * @param patterns The set of regular expressions that should be matched.
+ * @returns `true` if the test string matches any of the given regular expressions, `false` otherwise
+ */
+export function matches(testStr: string, ignoreCase: boolean, ...patterns: (RegExp | string)[]): boolean {
+    const flags = ignoreCase ? 'i' : undefined;
+    const regexps = patterns.map(pattern => new RegExp(pattern, flags));
+    return regexps.find(regexp => regexp.test(testStr)) !== undefined;
+}
+
+/**
+ * Specialized `StringBasedFilter` that can be used to filter objects based on their constructor name.
+ */
+@injectable()
+export abstract class NameBasedFilter<T extends Object> extends StringBasedFilter<T> {
+    toString(toTest: T): string {
+        return toTest.constructor.name;
+    }
+}
+