Skip to content

fix: library search boosting #1866

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

Merged
merged 1 commit into from
Feb 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 arduino-ide-extension/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
"build": "tsc && ncp ./src/node/cli-protocol/ ./lib/node/cli-protocol/ && yarn lint",
"watch": "tsc -w",
"test": "mocha \"./lib/test/**/*.test.js\"",
"test:slow": "mocha \"./lib/test/**/*.slow-test.js\"",
"test:slow": "mocha \"./lib/test/**/*.slow-test.js\" --slow 5000",
"test:watch": "mocha --watch --watch-files lib \"./lib/test/**/*.test.js\""
},
"dependencies": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ export class BoardsListWidget extends ListWidget<BoardsPackage, BoardSearch> {
searchable: service,
installable: service,
itemLabel: (item: BoardsPackage) => item.name,
itemDeprecated: (item: BoardsPackage) => item.deprecated,
itemRenderer,
filterRenderer,
defaultSearchOptions: { query: '', type: 'All' },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ export class LibraryListWidget extends ListWidget<
searchable: service,
installable: service,
itemLabel: (item: LibraryPackage) => item.name,
itemDeprecated: (item: LibraryPackage) => item.deprecated,
itemRenderer,
filterRenderer,
defaultSearchOptions: { query: '', type: 'All', topic: 'All' },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,6 @@ export namespace ComponentList {
export interface Props<T extends ArduinoComponent> {
readonly items: T[];
readonly itemLabel: (item: T) => string;
readonly itemDeprecated: (item: T) => boolean;
readonly itemRenderer: ListItemRenderer<T>;
readonly install: (item: T, version?: Installable.Version) => Promise<void>;
readonly uninstall: (item: T) => Promise<void>;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,12 +82,11 @@ export class FilterableListContainer<
}

protected renderComponentList(): React.ReactNode {
const { itemLabel, itemDeprecated, itemRenderer } = this.props;
const { itemLabel, itemRenderer } = this.props;
return (
<ComponentList<T>
items={this.state.items}
itemLabel={itemLabel}
itemDeprecated={itemDeprecated}
itemRenderer={itemRenderer}
install={this.install.bind(this)}
uninstall={this.uninstall.bind(this)}
Expand All @@ -109,9 +108,7 @@ export class FilterableListContainer<

protected search(searchOptions: S): void {
const { searchable } = this.props;
searchable
.search(searchOptions)
.then((items) => this.setState({ items: this.props.sort(items) }));
searchable.search(searchOptions).then((items) => this.setState({ items }));
}

protected async install(
Expand All @@ -127,7 +124,7 @@ export class FilterableListContainer<
run: ({ progressId }) => install({ item, progressId, version }),
});
const items = await searchable.search(this.state.searchOptions);
this.setState({ items: this.props.sort(items) });
this.setState({ items });
}

protected async uninstall(item: T): Promise<void> {
Expand Down Expand Up @@ -155,7 +152,7 @@ export class FilterableListContainer<
run: ({ progressId }) => uninstall({ item, progressId }),
});
const items = await searchable.search(this.state.searchOptions);
this.setState({ items: this.props.sort(items) });
this.setState({ items });
}
}

Expand All @@ -168,7 +165,6 @@ export namespace FilterableListContainer {
readonly container: ListWidget<T, S>;
readonly searchable: Searchable<T, S>;
readonly itemLabel: (item: T) => string;
readonly itemDeprecated: (item: T) => boolean;
readonly itemRenderer: ListItemRenderer<T>;
readonly filterRenderer: FilterRenderer<S>;
readonly resolveFocus: (element: HTMLElement | undefined) => void;
Expand All @@ -192,7 +188,6 @@ export namespace FilterableListContainer {
progressId: string;
}) => Promise<void>;
readonly commandService: CommandService;
readonly sort: (items: T[]) => T[];
}

export interface State<T, S extends Searchable.Options> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,9 @@ export abstract class ListWidget<
*/
protected firstActivate = true;

protected readonly defaultSortComparator: (left: T, right: T) => number;

constructor(protected options: ListWidget.Options<T, S>) {
super();
const { id, label, iconClass, itemDeprecated, itemLabel } = options;
const { id, label, iconClass } = options;
this.id = id;
this.title.label = label;
this.title.caption = label;
Expand All @@ -67,15 +65,6 @@ export abstract class ListWidget<
this.node.tabIndex = 0; // To be able to set the focus on the widget.
this.scrollOptions = undefined;
this.toDispose.push(this.searchOptionsChangeEmitter);

this.defaultSortComparator = (left, right): number => {
// always put deprecated items at the bottom of the list
if (itemDeprecated(left)) {
return 1;
}

return itemLabel(left).localeCompare(itemLabel(right));
};
}

@postConstruct()
Expand Down Expand Up @@ -144,30 +133,6 @@ export abstract class ListWidget<
return this.options.installable.uninstall({ item, progressId });
}

protected filterableListSort = (items: T[]): T[] => {
const isArduinoTypeComparator = (left: T, right: T) => {
const aIsArduinoType = left.types.includes('Arduino');
const bIsArduinoType = right.types.includes('Arduino');

if (aIsArduinoType && !bIsArduinoType && !left.deprecated) {
return -1;
}

if (!aIsArduinoType && bIsArduinoType && !right.deprecated) {
return 1;
}

return 0;
};

return items.sort((left, right) => {
return (
isArduinoTypeComparator(left, right) ||
this.defaultSortComparator(left, right)
);
});
};

render(): React.ReactNode {
return (
<FilterableListContainer<T, S>
Expand All @@ -178,14 +143,12 @@ export abstract class ListWidget<
install={this.install.bind(this)}
uninstall={this.uninstall.bind(this)}
itemLabel={this.options.itemLabel}
itemDeprecated={this.options.itemDeprecated}
itemRenderer={this.options.itemRenderer}
filterRenderer={this.options.filterRenderer}
searchOptionsDidChange={this.searchOptionsChangeEmitter.event}
messageService={this.messageService}
commandService={this.commandService}
responseService={this.responseService}
sort={this.filterableListSort}
/>
);
}
Expand Down Expand Up @@ -218,7 +181,6 @@ export namespace ListWidget {
readonly installable: Installable<T>;
readonly searchable: Searchable<T, S>;
readonly itemLabel: (item: T) => string;
readonly itemDeprecated: (item: T) => boolean;
readonly itemRenderer: ListItemRenderer<T>;
readonly filterRenderer: FilterRenderer<S>;
readonly defaultSearchOptions: S;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { Installable } from './installable';

export interface ArduinoComponent {
readonly name: string;
readonly deprecated: boolean;
readonly deprecated?: boolean;
readonly author: string;
readonly summary: string;
readonly description: string;
Expand Down
29 changes: 29 additions & 0 deletions arduino-ide-extension/src/common/protocol/searchable.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import URI from '@theia/core/lib/common/uri';
import type { ArduinoComponent } from './arduino-component';

export interface Searchable<T, O extends Searchable.Options> {
search(options: O): Promise<T[]>;
Expand Down Expand Up @@ -31,3 +32,31 @@ export namespace Searchable {
}
}
}

// IDE2 must keep the library search order from the CLI but do additional boosting
// https://github.com/arduino/arduino-ide/issues/1106
// This additional search result boosting considers the following groups: 'Arduino', '', 'Arduino-Retired', and 'Retired'.
// If two libraries fall into the same group, the original index is the tiebreaker.
export type SortGroup = 'Arduino' | '' | 'Arduino-Retired' | 'Retired';
const sortGroupOrder: Record<SortGroup, number> = {
Arduino: 0,
'': 1,
'Arduino-Retired': 2,
Retired: 3,
};

export function sortComponents<T extends ArduinoComponent>(
components: T[],
group: (component: T) => SortGroup
): T[] {
return components
.map((component, index) => ({ ...component, index }))
.sort((left, right) => {
const leftGroup = group(left);
const rightGroup = group(right);
if (leftGroup === rightGroup) {
return left.index - right.index;
}
return sortGroupOrder[leftGroup] - sortGroupOrder[rightGroup];
});
}
16 changes: 15 additions & 1 deletion arduino-ide-extension/src/node/boards-service-impl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ import {
BoardWithPackage,
BoardUserField,
BoardSearch,
sortComponents,
SortGroup,
} from '../common/protocol';
import {
PlatformInstallRequest,
Expand Down Expand Up @@ -405,7 +407,8 @@ export class BoardsServiceImpl
}

const filter = this.typePredicate(options);
return [...packages.values()].filter(filter);
const boardsPackages = [...packages.values()].filter(filter);
return sortComponents(boardsPackages, boardsPackageSortGroup);
}

private typePredicate(
Expand Down Expand Up @@ -559,3 +562,14 @@ function isMissingPlatformError(error: unknown): boolean {
}
return false;
}

function boardsPackageSortGroup(boardsPackage: BoardsPackage): SortGroup {
const types: string[] = [];
if (boardsPackage.types.includes('Arduino')) {
types.push('Arduino');
}
if (boardsPackage.deprecated) {
types.push('Retired');
}
return types.join('-') as SortGroup;
}
42 changes: 30 additions & 12 deletions arduino-ide-extension/src/node/library-service-impl.ts
Original file line number Diff line number Diff line change
@@ -1,34 +1,39 @@
import { injectable, inject } from '@theia/core/shared/inversify';
import { ILogger, notEmpty } from '@theia/core';
import { FileUri } from '@theia/core/lib/node';
import { inject, injectable } from '@theia/core/shared/inversify';
import { duration } from '../common/decorators';
import {
NotificationServiceServer,
ResponseService,
sortComponents,
SortGroup,
} from '../common/protocol';
import { Installable } from '../common/protocol/installable';
import {
LibraryDependency,
LibraryLocation,
LibraryPackage,
LibrarySearch,
LibraryService,
} from '../common/protocol/library-service';
import { CoreClientAware } from './core-client-provider';
import { BoardDiscovery } from './board-discovery';
import {
InstalledLibrary,
Library,
LibraryInstallLocation,
LibraryInstallRequest,
LibraryListRequest,
LibraryListResponse,
LibraryLocation as GrpcLibraryLocation,
LibraryRelease,
LibraryResolveDependenciesRequest,
LibraryUninstallRequest,
ZipLibraryInstallRequest,
LibrarySearchRequest,
LibrarySearchResponse,
LibraryInstallLocation,
LibraryUninstallRequest,
ZipLibraryInstallRequest,
} from './cli-protocol/cc/arduino/cli/commands/v1/lib_pb';
import { Installable } from '../common/protocol/installable';
import { ILogger, notEmpty } from '@theia/core';
import { FileUri } from '@theia/core/lib/node';
import { ResponseService, NotificationServiceServer } from '../common/protocol';
import { CoreClientAware } from './core-client-provider';
import { ExecuteWithProgress } from './grpc-progressible';
import { duration } from '../common/decorators';

@injectable()
export class LibraryServiceImpl
Expand Down Expand Up @@ -108,7 +113,10 @@ export class LibraryServiceImpl

const typePredicate = this.typePredicate(options);
const topicPredicate = this.topicPredicate(options);
return items.filter((item) => typePredicate(item) && topicPredicate(item));
const libraries = items.filter(
(item) => typePredicate(item) && topicPredicate(item)
);
return sortComponents(libraries, librarySortGroup);
}

private typePredicate(
Expand Down Expand Up @@ -448,7 +456,6 @@ function toLibrary(
name: '',
exampleUris: [],
installable: false,
deprecated: false,
location: 0,
...pkg,

Expand All @@ -462,3 +469,14 @@ function toLibrary(
types: lib.getTypesList(),
};
}

// Libraries do not have a deprecated property. The deprecated information is inferred if 'Retired' is in 'types'
function librarySortGroup(library: LibraryPackage): SortGroup {
const types: string[] = [];
for (const type of ['Arduino', 'Retired']) {
if (library.types.includes(type)) {
types.push(type);
}
}
return types.join('-') as SortGroup;
}
Loading