Skip to content

Commit 77b13bd

Browse files
committed
Ensure query compare order matches expectation
A user typically expects that the first selection would be the query that they are comparing _from_ and the second query is being compared _to_. This commit ensures that something like this expectation will always hold. So, when there are two queries selected, the first one selected will always be _from_ and appear on the left side of the compare view. The one selected later will be _to_ and appear on the right. There is a corner case when there are 3 or more selected queries and a user *unselects* a query. We do not track the selection order of the remaining two queries.
1 parent f4e983e commit 77b13bd

File tree

2 files changed

+73
-4
lines changed

2 files changed

+73
-4
lines changed

extensions/ql-vscode/src/query-history.ts

Lines changed: 34 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ export class QueryHistoryManager {
167167
treeDataProvider: HistoryTreeDataProvider;
168168
treeView: vscode.TreeView<CompletedQuery>;
169169
lastItemClick: { time: Date; item: CompletedQuery } | undefined;
170-
170+
compareWithItem: CompletedQuery | undefined;
171171

172172
constructor(
173173
ctx: ExtensionContext,
@@ -185,6 +185,7 @@ export class QueryHistoryManager {
185185
treeDataProvider,
186186
canSelectMany: true,
187187
});
188+
188189
// Lazily update the tree view selection due to limitations of TreeView API (see
189190
// `updateTreeViewSelectionIfVisible` doc for details)
190191
this.treeView.onDidChangeVisibility(async (_ev) =>
@@ -195,6 +196,7 @@ export class QueryHistoryManager {
195196
if (ev.selection.length == 0) {
196197
this.updateTreeViewSelectionIfVisible();
197198
}
199+
this.updateCompareWith(ev.selection);
198200
});
199201
logger.log('Registering query history panel commands.');
200202
ctx.subscriptions.push(
@@ -349,8 +351,8 @@ export class QueryHistoryManager {
349351
throw new Error('Please select a successful query.');
350352
}
351353

352-
const from = singleItem;
353-
const to = await this.findOtherQueryToCompare(singleItem, multiSelect);
354+
const from = this.compareWithItem || singleItem;
355+
const to = await this.findOtherQueryToCompare(from, multiSelect);
354356

355357
if (from && to) {
356358
this.doCompareCallback(from, to);
@@ -588,4 +590,33 @@ the file in the file explorer and dragging it into the workspace.`
588590
}
589591
return true;
590592
}
593+
594+
/**
595+
* Updates the compare with source query. This ensures that all compare command invocations
596+
* when exactly 2 queries are selected always have the proper _from_ query. Always use
597+
* compareWithItem as the _from_ query.
598+
*
599+
* The heuristic is this:
600+
*
601+
* 1. If selection is empty or has length > 2 delete compareWithItem.
602+
* 2. If selection is length 1, then set that item to compareWithItem.
603+
* 3. If selection is length 2, then make sure compareWithItem is one of the selected items
604+
* if not, then delete compareWithItem. If it is then, do nothing.
605+
*
606+
* This ensures that compareWithItem is always the first item selected if there are only
607+
* two selected items.
608+
*
609+
* @param newSelection the new selection after the most recent selection change
610+
*/
611+
private updateCompareWith(newSelection: CompletedQuery[]) {
612+
if (newSelection.length === 1) {
613+
this.compareWithItem = newSelection[0];
614+
} else if (
615+
newSelection.length !== 2 ||
616+
!this.compareWithItem ||
617+
!newSelection.includes(this.compareWithItem)
618+
) {
619+
this.compareWithItem = undefined;
620+
}
621+
}
591622
}

extensions/ql-vscode/src/vscode-tests/no-workspace/query-history.test.ts

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,42 @@ describe('query-history', () => {
169169
}
170170
});
171171
});
172+
173+
describe('updateCompareWith', () => {
174+
it('should update compareWithItem when there is a single item', () => {
175+
const queryHistory = createMockQueryHistory([]);
176+
queryHistory.updateCompareWith(['a']);
177+
expect(queryHistory.compareWithItem).to.be.eq('a');
178+
});
179+
180+
it('should delete compareWithItem when there are 0 items', () => {
181+
const queryHistory = createMockQueryHistory([]);
182+
queryHistory.compareWithItem = 'a';
183+
queryHistory.updateCompareWith([]);
184+
expect(queryHistory.compareWithItem).to.be.undefined;
185+
});
186+
187+
it('should delete compareWithItem when there are more than 2 items', () => {
188+
const queryHistory = createMockQueryHistory([]);
189+
queryHistory.compareWithItem = 'a';
190+
queryHistory.updateCompareWith(['a', 'b', 'c']);
191+
expect(queryHistory.compareWithItem).to.be.undefined;
192+
});
193+
194+
it('should delete compareWithItem when there are 2 items and disjoint from compareWithItem', () => {
195+
const queryHistory = createMockQueryHistory([]);
196+
queryHistory.compareWithItem = 'a';
197+
queryHistory.updateCompareWith(['b', 'c']);
198+
expect(queryHistory.compareWithItem).to.be.undefined;
199+
});
200+
201+
it('should do nothing when compareWithItem exists and exactly 2 items', () => {
202+
const queryHistory = createMockQueryHistory([]);
203+
queryHistory.compareWithItem = 'a';
204+
queryHistory.updateCompareWith(['a', 'b']);
205+
expect(queryHistory.compareWithItem).to.be.eq('a');
206+
});
207+
});
172208
});
173209

174210
function createMockQueryHistory(allHistory: {}[]) {
@@ -177,6 +213,8 @@ function createMockQueryHistory(allHistory: {}[]) {
177213
findOtherQueryToCompare: (QueryHistoryManager.prototype as any).findOtherQueryToCompare,
178214
treeDataProvider: {
179215
allHistory
180-
}
216+
},
217+
updateCompareWith: (QueryHistoryManager.prototype as any).updateCompareWith,
218+
compareWithItem: undefined as undefined | string,
181219
};
182220
}

0 commit comments

Comments
 (0)