From 616b7fac30fa66ad3b731d75b384d558a8281be2 Mon Sep 17 00:00:00 2001 From: Eric Amodio Date: Sun, 25 Sep 2022 01:39:56 -0400 Subject: [PATCH] Considers selected row in graph search Adds search shortcuts F3/Cmd+G to the graph --- src/env/node/git/git.ts | 88 ++++++++++--------- src/env/node/git/localGitProvider.ts | 47 +++++++--- src/git/parsers/logParser.ts | 26 ++++-- src/git/search.ts | 2 +- src/plus/github/github.ts | 8 +- src/plus/github/githubGitProvider.ts | 24 +++-- src/plus/webviews/graph/graphWebview.ts | 10 +-- src/plus/webviews/graph/protocol.ts | 2 +- src/webviews/apps/plus/graph/GraphWrapper.tsx | 71 ++++++++++++--- .../shared/components/search/search-field.ts | 5 +- .../shared/components/search/search-nav.ts | 44 +++++++++- 11 files changed, 235 insertions(+), 92 deletions(-) diff --git a/src/env/node/git/git.ts b/src/env/node/git/git.ts index fcf925d435fdd..e54b2cb1d2374 100644 --- a/src/env/node/git/git.ts +++ b/src/env/node/git/git.ts @@ -146,7 +146,7 @@ export class Git { 'core.quotepath=false', '-c', 'color.ui=false', - ...(configs !== undefined ? configs : emptyArray), + ...(configs != null ? configs : emptyArray), ); if (process.platform === 'win32') { @@ -861,20 +861,6 @@ export class Git { options?: { cancellation?: CancellationToken; configs?: readonly string[]; ref?: string; stdin?: string }, ...args: string[] ) { - const params = ['log']; - if (options?.stdin) { - params.push('--stdin'); - } - params.push(...args); - - if (options?.ref && !GitRevision.isUncommittedStaged(options.ref)) { - params.push(options?.ref); - } - - if (!params.includes('--')) { - params.push('--'); - } - return this.git( { cwd: repoPath, @@ -882,7 +868,11 @@ export class Git { configs: options?.configs ?? gitLogDefaultConfigs, stdin: options?.stdin, }, - ...params, + 'log', + ...(options?.stdin ? ['--stdin'] : emptyArray), + ...args, + ...(options?.ref && !GitRevision.isUncommittedStaged(options.ref) ? [options.ref] : emptyArray), + ...(!args.includes('--') ? ['--'] : emptyArray), ); } @@ -1185,35 +1175,34 @@ export class Git { log__search( repoPath: string, search: string[] = emptyArray, - { - limit, - ordering, - skip, - useShow, - }: { limit?: number; ordering?: 'date' | 'author-date' | 'topo' | null; skip?: number; useShow?: boolean } = {}, + options?: { + limit?: number; + ordering?: 'date' | 'author-date' | 'topo' | null; + skip?: number; + useShow?: boolean; + }, ) { - const params = [ - useShow ? 'show' : 'log', - '--name-status', - `--format=${GitLogParser.defaultFormat}`, - '--use-mailmap', - ]; - - if (limit && !useShow) { - params.push(`-n${limit + 1}`); - } - - if (skip && !useShow) { - params.push(`--skip=${skip}`); - } - - if (ordering && !useShow) { - params.push(`--${ordering}-order`); + if (options?.useShow) { + return this.git( + { cwd: repoPath }, + 'show', + '-s', + '--name-status', + `--format=${GitLogParser.defaultFormat}`, + '--use-mailmap', + ...search, + ); } return this.git( - { cwd: repoPath, configs: useShow ? undefined : gitLogDefaultConfigs }, - ...params, + { cwd: repoPath, configs: gitLogDefaultConfigs }, + 'log', + '--name-status', + `--format=${GitLogParser.defaultFormat}`, + '--use-mailmap', + ...(options?.limit ? [`-n${options.limit + 1}`] : emptyArray), + ...(options?.skip ? [`--skip=${options.skip}`] : emptyArray), + ...(options?.ordering ? [`--${options.ordering}-order`] : emptyArray), ...search, ); } @@ -1567,6 +1556,23 @@ export class Git { } } + show2( + repoPath: string, + options?: { cancellation?: CancellationToken; configs?: readonly string[] }, + ...args: string[] + ) { + return this.git( + { + cwd: repoPath, + cancellation: options?.cancellation, + configs: options?.configs ?? gitLogDefaultConfigs, + }, + 'show', + ...args, + ...(!args.includes('--') ? ['--'] : emptyArray), + ); + } + show__diff( repoPath: string, fileName: string, diff --git a/src/env/node/git/localGitProvider.ts b/src/env/node/git/localGitProvider.ts index e221430729325..74c50ba9594a1 100644 --- a/src/env/node/git/localGitProvider.ts +++ b/src/env/node/git/localGitProvider.ts @@ -92,7 +92,8 @@ import { createLogParserSingle, createLogParserWithFiles, getGraphParser, - getGraphRefParser, + getRefAndDateParser, + getRefParser, GitLogParser, LogType, } from '../../../git/parsers/logParser'; @@ -1620,7 +1621,7 @@ export class LocalGitProvider implements GitProvider, Disposable { }, ): Promise { const parser = getGraphParser(); - const refParser = getGraphRefParser(); + const refParser = getRefParser(); const defaultLimit = options?.limit ?? configuration.get('graph.defaultItemLimit') ?? 5000; const defaultPageLimit = configuration.get('graph.pageItemLimit') ?? 1000; @@ -2646,22 +2647,40 @@ export class LocalGitProvider implements GitProvider, Disposable { const comparisonKey = getSearchQueryComparisonKey(search); try { + const refAndDateParser = getRefAndDateParser(); + const { args: searchArgs, files, shas } = getGitArgsFromSearchQuery(search); if (shas?.size) { + const data = await this.git.show2( + repoPath, + { cancellation: options?.cancellation }, + '-s', + ...refAndDateParser.arguments, + ...shas.values(), + ...searchArgs, + '--', + ); + + const results = new Map( + map(refAndDateParser.parse(data), c => [ + c.sha, + Number(options?.ordering === 'author-date' ? c.authorDate : c.committerDate) * 1000, + ]), + ); + return { repoPath: repoPath, query: search, comparisonKey: comparisonKey, - results: shas, + results: results, }; } - const refParser = getGraphRefParser(); const limit = options?.limit ?? configuration.get('advanced.maxSearchItems') ?? 0; const similarityThreshold = configuration.get('advanced.similarityThreshold'); const args = [ - ...refParser.arguments, + ...refAndDateParser.arguments, `-M${similarityThreshold == null ? '' : `${similarityThreshold}%`}`, '--use-mailmap', ]; @@ -2669,7 +2688,7 @@ export class LocalGitProvider implements GitProvider, Disposable { args.push(`--${options.ordering}-order`); } - const results = new Set(); + const results = new Map(); let total = 0; let iterations = 0; @@ -2702,19 +2721,21 @@ export class LocalGitProvider implements GitProvider, Disposable { } let count = 0; - let last: string | undefined; - for (const r of refParser.parse(data)) { - results.add(r); + for (const r of refAndDateParser.parse(data)) { + results.set( + r.sha, + Number(options?.ordering === 'author-date' ? r.authorDate : r.committerDate) * 1000, + ); count++; - last = r; } total += count; + const lastSha = last(results)?.[0]; cursor = - last != null + lastSha != null ? { - sha: last, + sha: lastSha, skip: total - iterations, } : undefined; @@ -2743,7 +2764,7 @@ export class LocalGitProvider implements GitProvider, Disposable { repoPath: repoPath, query: search, comparisonKey: comparisonKey, - results: new Set(), + results: new Map(), }; } } diff --git a/src/git/parsers/logParser.ts b/src/git/parsers/logParser.ts index 683295737abca..581c5fad5fbfa 100644 --- a/src/git/parsers/logParser.ts +++ b/src/git/parsers/logParser.ts @@ -106,14 +106,28 @@ export function getGraphParser(): GraphParser { return _graphParser; } -type GraphRefParser = Parser; +type RefParser = Parser; -let _graphRefParser: GraphRefParser | undefined; -export function getGraphRefParser(): GraphRefParser { - if (_graphRefParser == null) { - _graphRefParser = createLogParserSingle('%H'); +let _refParser: RefParser | undefined; +export function getRefParser(): RefParser { + if (_refParser == null) { + _refParser = createLogParserSingle('%H'); } - return _graphRefParser; + return _refParser; +} + +type RefAndDateParser = Parser<{ sha: string; authorDate: string; committerDate: string }>; + +let _refAndDateParser: RefAndDateParser | undefined; +export function getRefAndDateParser(): RefAndDateParser { + if (_refAndDateParser == null) { + _refAndDateParser = createLogParser({ + sha: '%H', + authorDate: '%at', + committerDate: '%ct', + }); + } + return _refAndDateParser; } export function createLogParser>( diff --git a/src/git/search.ts b/src/git/search.ts index fc5213d927ac7..66597b6f5862e 100644 --- a/src/git/search.ts +++ b/src/git/search.ts @@ -47,7 +47,7 @@ export interface GitSearch { repoPath: string; query: SearchQuery; comparisonKey: string; - results: Set; + results: Map; readonly paging?: { readonly limit: number | undefined; diff --git a/src/plus/github/github.ts b/src/plus/github/github.ts index 685d165622909..dfd5b71237f26 100644 --- a/src/plus/github/github.ts +++ b/src/plus/github/github.ts @@ -1910,7 +1910,7 @@ export class GitHubApi implements Disposable { order?: 'asc' | 'desc' | undefined; sort?: 'author-date' | 'committer-date' | undefined; }, - ): Promise | undefined> { + ): Promise | undefined> { const scope = getLogScope(); const limit = Math.min(100, options?.limit ?? 100); @@ -1959,7 +1959,11 @@ export class GitHubApi implements Disposable { hasNextPage: hasMore, }, totalCount: data.total_count, - values: data.items.map(r => r.sha), + values: data.items.map(r => ({ + sha: r.sha, + authorDate: new Date(r.commit.author.date).getTime(), + committerDate: new Date(r.commit.committer?.date ?? r.commit.author.date).getTime(), + })), }; } catch (ex) { if (ex instanceof ProviderRequestNotFoundError) return undefined; diff --git a/src/plus/github/githubGitProvider.ts b/src/plus/github/githubGitProvider.ts index 061190c22c888..2e5fd04ad1db5 100644 --- a/src/plus/github/githubGitProvider.ts +++ b/src/plus/github/githubGitProvider.ts @@ -1597,14 +1597,23 @@ export class GitHubGitProvider implements GitProvider, Disposable { const comparisonKey = getSearchQueryComparisonKey(search); try { - const results = new Set(); + const results = new Map(); const operations = parseSearchQuery(search.query); let op; let values = operations.get('commit:'); if (values != null) { - for (const value of values) { - results.add(value.replace(doubleQuoteRegex, '')); + const commitsResults = await Promise.allSettled[]>( + values.map(v => this.getCommit(repoPath, v.replace(doubleQuoteRegex, ''))), + ); + for (const commitResult of commitsResults) { + const commit = getSettledValue(commitResult); + if (commit == null) continue; + + results.set( + commit.sha, + Number(options?.ordering === 'author-date' ? commit.author.date : commit.committer.date), + ); } return { @@ -1681,8 +1690,11 @@ export class GitHubGitProvider implements GitProvider, Disposable { return { repoPath: repoPath, query: search, comparisonKey: comparisonKey, results: results }; } - for (const sha of result.values) { - results.add(sha); + for (const commit of result.values) { + results.set( + commit.sha, + Number(options?.ordering === 'author-date' ? commit.authorDate : commit.committerDate), + ); } cursor = result.pageInfo?.endCursor ?? undefined; @@ -1710,7 +1722,7 @@ export class GitHubGitProvider implements GitProvider, Disposable { repoPath: repoPath, query: search, comparisonKey: comparisonKey, - results: new Set(), + results: new Map(), }; } } diff --git a/src/plus/webviews/graph/graphWebview.ts b/src/plus/webviews/graph/graphWebview.ts index 9c3ce83ed0915..4ea35b543a300 100644 --- a/src/plus/webviews/graph/graphWebview.ts +++ b/src/plus/webviews/graph/graphWebview.ts @@ -451,7 +451,7 @@ export class GraphWebview extends WebviewBase { DidSearchCommitsNotificationType, { results: { - ids: [...search.results.values()], + ids: Object.fromEntries(search.results), paging: { hasMore: search.paging?.hasMore ?? false }, }, selectedRows: this._selectedRows, @@ -497,14 +497,14 @@ export class GraphWebview extends WebviewBase { } if (search.results.size > 0) { - this.setSelectedRows(first(search.results)); + this.setSelectedRows(first(search.results)![0]); } void this.notify( DidSearchCommitsNotificationType, { results: { - ids: [...search.results.values()], + ids: Object.fromEntries(search.results), paging: { hasMore: search.paging?.hasMore ?? false }, }, selectedRows: this._selectedRows, @@ -778,8 +778,8 @@ export class GraphWebview extends WebviewBase { if (this._search != null) { const search = this._search; - const lastResult = last(search.results); - if (lastResult != null && updatedGraph.ids.has(lastResult)) { + const lastId = last(search.results)?.[0]; + if (lastId != null && updatedGraph.ids.has(lastId)) { queueMicrotask(() => void this.onSearchCommits({ search: search.query, more: true })); } } diff --git a/src/plus/webviews/graph/protocol.ts b/src/plus/webviews/graph/protocol.ts index a1766bff63fdf..8f7b4210d93bb 100644 --- a/src/plus/webviews/graph/protocol.ts +++ b/src/plus/webviews/graph/protocol.ts @@ -174,7 +174,7 @@ export const DidEnsureCommitNotificationType = new IpcNotificationType( diff --git a/src/webviews/apps/plus/graph/GraphWrapper.tsx b/src/webviews/apps/plus/graph/GraphWrapper.tsx index 1700bc92408cd..2fc581147d17b 100644 --- a/src/webviews/apps/plus/graph/GraphWrapper.tsx +++ b/src/webviews/apps/plus/graph/GraphWrapper.tsx @@ -99,11 +99,11 @@ const getGraphDateFormatter = (config?: GraphComponentConfig): OnFormatCommitDat return (commitDateTime: number) => formatCommitDateTime(commitDateTime, config?.dateStyle, config?.dateFormat); }; -const getSearchHighlights = (searchIds?: string[]): { [id: string]: boolean } | undefined => { +const getSearchHighlights = (searchIds?: [string, number][]): { [id: string]: boolean } | undefined => { if (!searchIds?.length) return undefined; const highlights: { [id: string]: boolean } = {}; - for (const sha of searchIds) { + for (const [sha] of searchIds) { highlights[sha] = true; } return highlights; @@ -251,8 +251,11 @@ export function GraphWrapper({ // search state const [searchQuery, setSearchQuery] = useState(undefined); const [searchResultKey, setSearchResultKey] = useState(undefined); - const [searchResultIds, setSearchResultIds] = useState(searchResults?.ids); + const [searchResultIds, setSearchResultIds] = useState( + searchResults != null ? Object.entries(searchResults.ids) : undefined, + ); const [hasMoreSearchResults, setHasMoreSearchResults] = useState(searchResults?.paging?.hasMore ?? false); + const [selectedRow, setSelectedRow] = useState(undefined); useEffect(() => { if (graphRows.length === 0) { @@ -266,8 +269,11 @@ export function GraphWrapper({ return; } - if (searchResultKey == null || (searchResultKey != null && !searchResultIds.includes(searchResultKey))) { - setSearchResultKey(searchResultIds[0]); + if ( + searchResultKey == null || + (searchResultKey != null && !searchResultIds.some(id => id[0] === searchResultKey)) + ) { + setSearchResultKey(searchResultIds[0][0]); } }, [searchResultIds]); @@ -276,16 +282,56 @@ export function GraphWrapper({ const searchPosition: number = useMemo(() => { if (searchResultKey == null || searchResultIds == null) return 0; - const idx = searchResultIds.indexOf(searchResultKey); + const idx = searchResultIds.findIndex(id => id[0] === searchResultKey); return idx < 1 ? 1 : idx + 1; }, [searchResultKey, searchResultIds]); const handleSearchNavigation = async (next = true) => { if (searchResultKey == null || searchResultIds == null) return; + let selected = searchResultKey; + if (selectedRow != null && selectedRow.sha !== searchResultKey) { + selected = selectedRow.sha; + } + let resultIds = searchResultIds; - let rowIndex = resultIds.indexOf(searchResultKey); - if (rowIndex === -1) return; + const selectedDate = selectedRow != null ? selectedRow.date + (next ? 1 : -1) : undefined; + + // Loop through the search results and: + // try to find the selected sha + // if next=true find the nearest date before the selected date + // if next=false find the nearest date after the selected date + let rowIndex: number | undefined; + let nearestDate: number | undefined; + let nearestIndex: number | undefined; + + let i = -1; + let date: number; + let sha: string; + for ([sha, date] of resultIds) { + i++; + + if (sha === selected) { + rowIndex = i; + break; + } + + if (selectedDate != null) { + if (next) { + if (date < selectedDate && (nearestDate == null || date > nearestDate)) { + nearestDate = date; + nearestIndex = i; + } + } else if (date > selectedDate && (nearestDate == null || date <= nearestDate)) { + nearestDate = date; + nearestIndex = i; + } + } + } + + if (rowIndex == null) { + rowIndex = nearestIndex == null ? resultIds.length - 1 : nearestIndex + (next ? -1 : 1); + } if (next) { if (rowIndex < resultIds.length - 1) { @@ -294,7 +340,7 @@ export function GraphWrapper({ const results = await onSearchCommitsPromise?.(searchQuery, { more: true }); if (results?.results != null) { if (resultIds.length < results.results.ids.length) { - resultIds = results.results.ids; + resultIds = Object.entries(results.results.ids); rowIndex++; } else { rowIndex = 0; @@ -312,7 +358,7 @@ export function GraphWrapper({ const results = await onSearchCommitsPromise?.(searchQuery, { limit: 0, more: true }); if (results?.results != null) { if (resultIds.length < results.results.ids.length) { - resultIds = results.results.ids; + resultIds = Object.entries(results.results.ids); } } } @@ -320,7 +366,7 @@ export function GraphWrapper({ rowIndex = resultIds.length - 1; } - const nextSha = resultIds[rowIndex]; + const nextSha = resultIds[rowIndex][0]; if (nextSha == null) return; if (onEnsureCommitPromise != null) { @@ -394,7 +440,7 @@ export function GraphWrapper({ setSubscriptionSnapshot(state.subscription); setIsPrivateRepo(state.selectedRepositoryVisibility === RepositoryVisibility.Private); setIsLoading(state.loading); - setSearchResultIds(state.searchResults?.ids); + setSearchResultIds(state.searchResults != null ? Object.entries(state.searchResults.ids) : undefined); setHasMoreSearchResults(state.searchResults?.paging?.hasMore ?? false); } @@ -443,6 +489,7 @@ export function GraphWrapper({ }; const handleSelectGraphRows = (graphRows: GraphRow[]) => { + setSelectedRow(graphRows[0]); onSelectionChange?.(graphRows.map(r => ({ id: r.sha, type: r.type as GitGraphRowType }))); }; diff --git a/src/webviews/apps/shared/components/search/search-field.ts b/src/webviews/apps/shared/components/search/search-field.ts index 89fa6e7284cb4..e95c57baf22cc 100644 --- a/src/webviews/apps/shared/components/search/search-field.ts +++ b/src/webviews/apps/shared/components/search/search-field.ts @@ -1,4 +1,4 @@ -import { attr, css, customElement, FASTElement, html, observable, ref } from '@microsoft/fast-element'; +import { attr, css, customElement, FASTElement, html, observable } from '@microsoft/fast-element'; import type { SearchQuery } from '../../../../../git/search'; import '../codicon'; @@ -207,8 +207,7 @@ export class SearchField extends FASTElement { } handleShortcutKeys(e: KeyboardEvent) { - if (e.key !== 'Enter' && e.key !== 'F3') return; - if (e.ctrlKey || e.metaKey || e.altKey) return; + if (e.key !== 'Enter' || e.ctrlKey || e.metaKey || e.altKey) return; e.preventDefault(); if (e.shiftKey) { diff --git a/src/webviews/apps/shared/components/search/search-nav.ts b/src/webviews/apps/shared/components/search/search-nav.ts index f1e6b7349bc21..bb095467dc6ea 100644 --- a/src/webviews/apps/shared/components/search/search-nav.ts +++ b/src/webviews/apps/shared/components/search/search-nav.ts @@ -1,5 +1,8 @@ import { attr, css, customElement, FASTElement, html, volatile, when } from '@microsoft/fast-element'; +import { isMac } from '@env/platform'; import { pluralize } from '../../../../../system/string'; +import type { Disposable } from '../../../shared/dom'; +import { DOM } from '../../../shared/dom'; import { numberConverter } from '../converters/number-converter'; import '../codicon'; @@ -119,11 +122,48 @@ export class SearchNav extends FASTElement { return this.total !== 0; } - handlePrevious(_e: Event) { + private _disposable: Disposable | undefined; + override connectedCallback(): void { + super.connectedCallback(); + + this._disposable = DOM.on(window, 'keyup', e => this.handleShortcutKeys(e)); + } + + override disconnectedCallback(): void { + super.disconnectedCallback(); + + this._disposable?.dispose(); + } + + next() { + this.$emit('next'); + } + + previous() { this.$emit('previous'); } + handleShortcutKeys(e: KeyboardEvent) { + if ( + (e.key !== 'F3' && e.key !== 'g') || + (e.key !== 'g' && (e.ctrlKey || e.metaKey || e.altKey)) || + (e.key === 'g' && (!e.metaKey || !isMac)) + ) { + return; + } + + if (e.shiftKey) { + this.previous(); + } else { + this.next(); + } + } + + handlePrevious(_e: Event) { + this.previous(); + } + handleNext(_e: Event) { - this.$emit('next'); + this.next(); } }