From db5a6408e63686054768eaf5bfa000f80fc5af86 Mon Sep 17 00:00:00 2001 From: Eric Amodio Date: Thu, 22 Sep 2022 23:33:27 -0400 Subject: [PATCH] Fixes row selection jumping on page loads & search Adds search cancellation to avoid needless calls Adds search limit setting for the Graph --- package.json | 7 +++ src/config.ts | 1 + src/env/node/git/git.ts | 13 ++++- src/env/node/git/localGitProvider.ts | 21 +++++-- src/git/gitProvider.ts | 4 +- src/git/gitProviderService.ts | 3 +- src/git/models/repository.ts | 4 +- src/git/search.ts | 2 +- src/plus/github/githubGitProvider.ts | 3 +- src/plus/webviews/graph/graphWebview.ts | 82 ++++++++++++++++++--------- src/plus/webviews/graph/protocol.ts | 38 +++++++------ src/webviews/apps/plus/graph/GraphWrapper.tsx | 8 ++- src/webviews/apps/plus/graph/graph.tsx | 12 +++- 13 files changed, 134 insertions(+), 64 deletions(-) diff --git a/package.json b/package.json index 2faea47..818e38b 100644 --- a/package.json +++ b/package.json @@ -2120,6 +2120,13 @@ "scope": "window", "order": 21 }, + "gitlens.graph.searchItemLimit": { + "type": "number", + "default": 100, + "markdownDescription": "Specifies the number of results to gather when searching in the _Commit Graph_. Use 0 to specify no limit", + "scope": "window", + "order": 22 + }, "gitlens.graph.commitOrdering": { "type": "string", "default": "date", diff --git a/src/config.ts b/src/config.ts index 00b6045..a6810f5 100644 --- a/src/config.ts +++ b/src/config.ts @@ -386,6 +386,7 @@ export interface GraphConfig { defaultItemLimit: number; highlightRowsOnRefHover: boolean; pageItemLimit: number; + searchItemLimit: number; statusBar: { enabled: boolean; }; diff --git a/src/env/node/git/git.ts b/src/env/node/git/git.ts index 5780063..fcf925d 100644 --- a/src/env/node/git/git.ts +++ b/src/env/node/git/git.ts @@ -856,7 +856,11 @@ export class Git { return this.git({ cwd: repoPath, configs: gitLogDefaultConfigsWithFiles }, ...params, '--'); } - log2(repoPath: string, options?: { configs?: readonly string[]; ref?: string; stdin?: string }, ...args: string[]) { + log2( + repoPath: string, + options?: { cancellation?: CancellationToken; configs?: readonly string[]; ref?: string; stdin?: string }, + ...args: string[] + ) { const params = ['log']; if (options?.stdin) { params.push('--stdin'); @@ -872,7 +876,12 @@ export class Git { } return this.git( - { cwd: repoPath, configs: options?.configs ?? gitLogDefaultConfigs, stdin: options?.stdin }, + { + cwd: repoPath, + cancellation: options?.cancellation, + configs: options?.configs ?? gitLogDefaultConfigs, + stdin: options?.stdin, + }, ...params, ); } diff --git a/src/env/node/git/localGitProvider.ts b/src/env/node/git/localGitProvider.ts index 83f2b2a..e281c81 100644 --- a/src/env/node/git/localGitProvider.ts +++ b/src/env/node/git/localGitProvider.ts @@ -2,7 +2,7 @@ import { readdir, realpath } from 'fs'; import { homedir, hostname, userInfo } from 'os'; import { resolve as resolvePath } from 'path'; import { env as process_env } from 'process'; -import type { Event, TextDocument, WorkspaceFolder } from 'vscode'; +import type { CancellationToken, Event, TextDocument, WorkspaceFolder } from 'vscode'; import { Disposable, env, EventEmitter, extensions, FileType, Range, Uri, window, workspace } from 'vscode'; import { fetch, getProxyAgent } from '@env/fetch'; import { hrtime } from '@env/hrtime'; @@ -2641,7 +2641,7 @@ export class LocalGitProvider implements GitProvider, Disposable { async searchForCommitsSimple( repoPath: string, search: SearchPattern, - options?: { limit?: number; ordering?: 'date' | 'author-date' | 'topo' }, + options?: { cancellation?: CancellationToken; limit?: number; ordering?: 'date' | 'author-date' | 'topo' }, ): Promise { search = { matchAll: false, matchCase: false, matchRegex: true, ...search }; @@ -2676,15 +2676,26 @@ export class LocalGitProvider implements GitProvider, Disposable { limit: number, cursor?: { sha: string; skip: number }, ): Promise { + if (options?.cancellation?.isCancellationRequested) { + // TODO@eamodio: Should we throw an error here? + return { repoPath: repoPath, pattern: search, results: [] }; + } + const data = await this.git.log2( repoPath, - undefined, + { cancellation: options?.cancellation }, ...args, ...(cursor?.skip ? [`--skip=${cursor.skip}`] : []), ...searchArgs, '--', ...files, ); + + if (options?.cancellation?.isCancellationRequested) { + // TODO@eamodio: Should we throw an error here? + return { repoPath: repoPath, pattern: search, results: [] }; + } + const results = [...refParser.parse(data)]; const last = results[results.length - 1]; @@ -2708,13 +2719,13 @@ export class LocalGitProvider implements GitProvider, Disposable { more: true, } : undefined, - more: async (limit: number): Promise => - searchForCommitsCore.call(this, limit, cursor), + more: async (limit: number): Promise => searchForCommitsCore.call(this, limit, cursor), }; } return searchForCommitsCore.call(this, limit); } catch (ex) { + // TODO@eamodio: Should we throw an error here? // TODO@eamodio handle error reporting -- just invalid patterns? or more detailed? return { repoPath: repoPath, diff --git a/src/git/gitProvider.ts b/src/git/gitProvider.ts index 66da6bd..bd3578b 100644 --- a/src/git/gitProvider.ts +++ b/src/git/gitProvider.ts @@ -1,4 +1,4 @@ -import type { Disposable, Event, Range, TextDocument, Uri, WorkspaceFolder } from 'vscode'; +import type { CancellationToken, Disposable, Event, Range, TextDocument, Uri, WorkspaceFolder } from 'vscode'; import type { Commit, InputBox } from '../@types/vscode.git'; import type { ForcePushMode } from '../@types/vscode.git.enums'; import type { Features } from '../features'; @@ -299,7 +299,7 @@ export interface GitProvider extends Disposable { searchForCommitsSimple( repoPath: string | Uri, search: SearchPattern, - options?: { limit?: number; ordering?: 'date' | 'author-date' | 'topo' }, + options?: { cancellation?: CancellationToken; limit?: number; ordering?: 'date' | 'author-date' | 'topo' }, ): Promise; getLogForSearch( repoPath: string, diff --git a/src/git/gitProviderService.ts b/src/git/gitProviderService.ts index 153b22b..c9645fb 100644 --- a/src/git/gitProviderService.ts +++ b/src/git/gitProviderService.ts @@ -1,5 +1,6 @@ import { encodingExists } from 'iconv-lite'; import type { + CancellationToken, ConfigurationChangeEvent, Event, Range, @@ -1468,7 +1469,7 @@ export class GitProviderService implements Disposable { searchForCommitsSimple( repoPath: string | Uri, search: SearchPattern, - options?: { limit?: number; ordering?: 'date' | 'author-date' | 'topo' }, + options?: { cancellation?: CancellationToken; limit?: number; ordering?: 'date' | 'author-date' | 'topo' }, ): Promise { const { provider, path } = this.getProvider(repoPath); return provider.searchForCommitsSimple(path, search, options); diff --git a/src/git/models/repository.ts b/src/git/models/repository.ts index d43a4a1..b89aef8 100644 --- a/src/git/models/repository.ts +++ b/src/git/models/repository.ts @@ -1,4 +1,4 @@ -import type { ConfigurationChangeEvent, Event, WorkspaceFolder } from 'vscode'; +import type { CancellationToken, ConfigurationChangeEvent, Event, WorkspaceFolder } from 'vscode'; import { Disposable, EventEmitter, ProgressLocation, RelativePattern, Uri, window, workspace } from 'vscode'; import { ForcePushMode } from '../../@types/vscode.git.enums'; import type { CreatePullRequestActionContext } from '../../api/gitlens'; @@ -861,7 +861,7 @@ export class Repository implements Disposable { searchForCommitsSimple( search: SearchPattern, - options?: { limit?: number; ordering?: 'date' | 'author-date' | 'topo' }, + options?: { cancellation?: CancellationToken; limit?: number; ordering?: 'date' | 'author-date' | 'topo' }, ): Promise { return this.container.git.searchForCommitsSimple(this.path, search, options); } diff --git a/src/git/search.ts b/src/git/search.ts index 6b681e6..8b98fa4 100644 --- a/src/git/search.ts +++ b/src/git/search.ts @@ -46,7 +46,7 @@ export interface GitSearch { readonly more: boolean; }; - more?(limit: number): Promise; + more?(limit: number): Promise; } export function getKeyForSearchPattern(search: SearchPattern) { diff --git a/src/plus/github/githubGitProvider.ts b/src/plus/github/githubGitProvider.ts index 47f9cd2..c22c522 100644 --- a/src/plus/github/githubGitProvider.ts +++ b/src/plus/github/githubGitProvider.ts @@ -2,6 +2,7 @@ import type { AuthenticationSession, AuthenticationSessionsChangeEvent, + CancellationToken, Disposable, Event, Range, @@ -1584,7 +1585,7 @@ export class GitHubGitProvider implements GitProvider, Disposable { async searchForCommitsSimple( repoPath: string, search: SearchPattern, - _options?: { limit?: number; ordering?: 'date' | 'author-date' | 'topo' }, + _options?: { cancellation?: CancellationToken; limit?: number; ordering?: 'date' | 'author-date' | 'topo' }, ): Promise { search = { matchAll: false, matchCase: false, matchRegex: true, ...search }; return { diff --git a/src/plus/webviews/graph/graphWebview.ts b/src/plus/webviews/graph/graphWebview.ts index e0771e7..c118d85 100644 --- a/src/plus/webviews/graph/graphWebview.ts +++ b/src/plus/webviews/graph/graphWebview.ts @@ -1,5 +1,5 @@ import type { ColorTheme, ConfigurationChangeEvent, Disposable, Event, StatusBarItem } from 'vscode'; -import { EventEmitter, MarkdownString, ProgressLocation, StatusBarAlignment, ViewColumn, window } from 'vscode'; +import { CancellationTokenSource, EventEmitter, MarkdownString, StatusBarAlignment, ViewColumn, window } from 'vscode'; import { getAvatarUri } from '../../../avatars'; import { parseCommandContext } from '../../../commands/base'; import { GitActions } from '../../../commands/gitCommands.actions'; @@ -14,7 +14,6 @@ import { GitGraphRowType } from '../../../git/models/graph'; import type { GitGraph } from '../../../git/models/graph'; import type { Repository, RepositoryChangeEvent } from '../../../git/models/repository'; import { RepositoryChange, RepositoryChangeComparisonMode } from '../../../git/models/repository'; -import type { SearchPattern } from '../../../git/search'; import { registerCommand } from '../../../system/command'; import { gate } from '../../../system/decorators/gate'; import { debug } from '../../../system/decorators/log'; @@ -28,7 +27,14 @@ import { onIpc } from '../../../webviews/protocol'; import { WebviewBase } from '../../../webviews/webviewBase'; import type { SubscriptionChangeEvent } from '../../subscription/subscriptionService'; import { ensurePlusFeaturesEnabled } from '../../subscription/utils'; -import type { DismissBannerParams, GraphComponentConfig, GraphRepository, State } from './protocol'; +import type { + DismissBannerParams, + EnsureCommitParams, + GraphComponentConfig, + GraphRepository, + SearchCommitsParams, + State, +} from './protocol'; import { DidChangeAvatarsNotificationType, DidChangeCommitsNotificationType, @@ -195,6 +201,9 @@ export class GraphWebview extends WebviewBase { case DismissBannerCommandType.method: onIpc(DismissBannerCommandType, e, params => this.dismissBanner(params.key)); break; + case EnsureCommitCommandType.method: + onIpc(EnsureCommitCommandType, e, params => this.onEnsureCommit(params, e.completionId)); + break; case GetMissingAvatarsCommandType.method: onIpc(GetMissingAvatarsCommandType, e, params => this.onGetMissingAvatars(params.emails)); break; @@ -202,7 +211,7 @@ export class GraphWebview extends WebviewBase { onIpc(GetMoreCommitsCommandType, e, params => this.onGetMoreCommits(params.sha, e.id)); break; case SearchCommitsCommandType.method: - onIpc(SearchCommitsCommandType, e, params => this.onSearchCommits(params.search, e.id)); + onIpc(SearchCommitsCommandType, e, params => this.onSearchCommits(params, e.id)); break; case UpdateColumnCommandType.method: onIpc(UpdateColumnCommandType, e, params => this.onColumnUpdated(params.name, params.config)); @@ -213,9 +222,6 @@ export class GraphWebview extends WebviewBase { case UpdateSelectionCommandType.method: onIpc(UpdateSelectionCommandType, e, params => this.onSelectionChanged(params.selection)); break; - case EnsureCommitCommandType.method: - onIpc(EnsureCommitCommandType, e, params => this.onEnsureCommit(params.id, e.completionId)); - break; } } @@ -349,22 +355,31 @@ export class GraphWebview extends WebviewBase { void this.notifyDidChangeGraphConfiguration(); } - private async onEnsureCommit(id: string, completionId?: string) { + @debug() + private async onEnsureCommit(e: EnsureCommitParams, completionId?: string) { if (this._graph?.more == null) return; - if (!this._graph.ids.has(id)) { + let selected: boolean | undefined; + if (!this._graph.ids.has(e.id)) { const { defaultItemLimit, pageItemLimit } = configuration.get('graph'); - const newGraph = await this._graph.more(pageItemLimit ?? defaultItemLimit, id); + const newGraph = await this._graph.more(pageItemLimit ?? defaultItemLimit, e.id); if (newGraph != null) { this.setGraph(newGraph); } else { debugger; } + if (e.select && this._graph.ids.has(e.id)) { + selected = true; + this.setSelectedRows(e.id); + } void this.notifyDidChangeCommits(); + } else if (e.select) { + selected = true; + this.setSelectedRows(e.id); } - void this.notify(DidEnsureCommitNotificationType, { id: id }, completionId); + void this.notify(DidEnsureCommitNotificationType, { id: e.id, selected: selected }, completionId); } private async onGetMissingAvatars(emails: { [email: string]: string }) { @@ -392,6 +407,7 @@ export class GraphWebview extends WebviewBase { } @gate() + @debug() private async onGetMoreCommits(sha?: string, completionId?: string) { if (this._graph?.more == null || this._repository?.etag !== this._etagRepository) { this.updateState(true); @@ -410,21 +426,37 @@ export class GraphWebview extends WebviewBase { void this.notifyDidChangeCommits(completionId); } - @gate() - private async onSearchCommits(searchPattern: SearchPattern, completionId?: string) { - // if (this._repository?.etag !== this._etagRepository) { - // this.updateState(true); - - // return; - // } + private _searchCancellation: CancellationTokenSource | undefined; + @debug() + private async onSearchCommits(e: SearchCommitsParams, completionId?: string) { if (this._repository == null) return; - const search = await this._repository.searchForCommitsSimple(searchPattern, { - limit: 100, + if (this._repository.etag !== this._etagRepository) { + this.updateState(true); + } + + if (this._searchCancellation != null) { + this._searchCancellation.cancel(); + this._searchCancellation.dispose(); + } + + const cancellation = new CancellationTokenSource(); + this._searchCancellation = cancellation; + + const search = await this._repository.searchForCommitsSimple(e.search, { + limit: configuration.get('graph.searchItemLimit') ?? 100, ordering: configuration.get('graph.commitOrdering'), + cancellation: cancellation.token, }); + if (cancellation.token.isCancellationRequested) { + if (completionId != null) { + void this.notify(DidSearchCommitsNotificationType, { results: undefined }, completionId); + } + return; + } + if (search.results.length > 0) { this.setSelectedRows(search.results[0]); } @@ -432,7 +464,7 @@ export class GraphWebview extends WebviewBase { void this.notify( DidSearchCommitsNotificationType, { - searchResults: { + results: { ids: search.results, paging: { startingCursor: search.paging?.startingCursor, @@ -451,7 +483,7 @@ export class GraphWebview extends WebviewBase { private async onSelectionChanged(selection: { id: string; type: GitGraphRowType }[]) { const item = selection[0]; - this._selectedSha = item?.id; + this.setSelectedRows(item?.id); let commits: GitCommit[] | undefined; if (item?.id != null) { @@ -515,11 +547,7 @@ export class GraphWebview extends WebviewBase { private async notifyDidChangeState() { if (!this.isReady || !this.visible) return false; - return window.withProgress({ location: ProgressLocation.Window, title: 'Loading Commit Graph...' }, async () => - this.notify(DidChangeNotificationType, { - state: await this.getState(), - }), - ); + return this.notify(DidChangeNotificationType, { state: await this.getState() }); } @debug() diff --git a/src/plus/webviews/graph/protocol.ts b/src/plus/webviews/graph/protocol.ts index 84ed35a..487867b 100644 --- a/src/plus/webviews/graph/protocol.ts +++ b/src/plus/webviews/graph/protocol.ts @@ -25,7 +25,7 @@ export interface State { // Props below are computed in the webview (not passed) mixedColumnColors?: Record; - searchResults?: DidSearchCommitsParams['searchResults']; + searchResults?: DidSearchCommitsParams['results']; } export interface GraphPaging { @@ -79,6 +79,12 @@ export interface DismissBannerParams { } export const DismissBannerCommandType = new IpcCommandType('graph/dismissBanner'); +export interface EnsureCommitParams { + id: string; + select?: boolean; +} +export const EnsureCommitCommandType = new IpcCommandType('graph/ensureCommit'); + export interface GetMissingAvatarsParams { emails: { [email: string]: string }; } @@ -112,11 +118,6 @@ export interface UpdateSelectionParams { } export const UpdateSelectionCommandType = new IpcCommandType('graph/update/selection'); -export interface EnsureCommitParams { - id: string; -} -export const EnsureCommitCommandType = new IpcCommandType('graph/ensureCommit'); - // Notifications export interface DidChangeParams { state: State; @@ -162,20 +163,23 @@ export const DidChangeSelectionNotificationType = new IpcNotificationType( - 'graph/commits/didSearch', -); - export interface DidEnsureCommitParams { id?: string; + selected?: boolean; } export const DidEnsureCommitNotificationType = new IpcNotificationType( 'graph/commits/didEnsureCommit', ); + +export interface DidSearchCommitsParams { + results: + | { + ids: string[]; + paging?: GraphPaging; + } + | undefined; + selectedRows?: { [id: string]: true }; +} +export const DidSearchCommitsNotificationType = new IpcNotificationType( + 'graph/commits/didSearch', +); diff --git a/src/webviews/apps/plus/graph/GraphWrapper.tsx b/src/webviews/apps/plus/graph/GraphWrapper.tsx index e9bd18d..cb4894b 100644 --- a/src/webviews/apps/plus/graph/GraphWrapper.tsx +++ b/src/webviews/apps/plus/graph/GraphWrapper.tsx @@ -40,7 +40,7 @@ export interface GraphWrapperProps extends State { onSearchCommits?: (search: SearchPattern) => void; //Promise; onDismissBanner?: (key: DismissBannerParams['key']) => void; onSelectionChange?: (selection: { id: string; type: GitGraphRowType }[]) => void; - onEnsureCommit?: (id: string) => Promise; + onEnsureCommit?: (id: string, select: boolean) => Promise; } const getStyleProps = ( @@ -299,7 +299,7 @@ export function GraphWrapper({ timeout = undefined; setIsLoading(true); }, 250); - onEnsureCommit(nextSha).finally(() => { + onEnsureCommit(nextSha, true).finally(() => { if (timeout == null) { setIsLoading(false); } else { @@ -347,7 +347,9 @@ export function GraphWrapper({ setAvatars(state.avatars ?? {}); setReposList(state.repositories ?? []); setCurrentRepository(reposList.find(item => item.path === state.selectedRepository)); - setSelectedRows(state.selectedRows); + if (JSON.stringify(graphSelectedRows) !== JSON.stringify(state.selectedRows)) { + setSelectedRows(state.selectedRows); + } setGraphConfig(state.config); // setGraphDateFormatter(getGraphDateFormatter(config)); setGraphColSettings(getGraphColSettingsModel(state.config)); diff --git a/src/webviews/apps/plus/graph/graph.tsx b/src/webviews/apps/plus/graph/graph.tsx index e8c396d..ed76a52 100644 --- a/src/webviews/apps/plus/graph/graph.tsx +++ b/src/webviews/apps/plus/graph/graph.tsx @@ -192,9 +192,11 @@ export class GraphApp extends App { case DidSearchCommitsNotificationType.method: onIpc(DidSearchCommitsNotificationType, msg, params => { + if (params.results == null && params.selectedRows == null) return; + this.setState({ ...this.state, - searchResults: params.searchResults, + searchResults: params.results, selectedRows: params.selectedRows, }); this.refresh(this.state); @@ -316,8 +318,12 @@ export class GraphApp extends App { return this.sendCommand(SearchCommitsCommandType, { search: search }); } - private onEnsureCommit(id: string) { - return this.sendCommandWithCompletion(EnsureCommitCommandType, { id: id }, DidEnsureCommitNotificationType); + private onEnsureCommit(id: string, select: boolean) { + return this.sendCommandWithCompletion( + EnsureCommitCommandType, + { id: id, select: select }, + DidEnsureCommitNotificationType, + ); } private onSelectionChanged(selection: { id: string; type: GitGraphRowType }[]) {