Skip to content

Commit dbe6985

Browse files
navn-rfcollonval
navn-r
and
fcollonval
committed
Apply suggestions from code review
Co-authored-by: Frédéric Collonval <[email protected]>
1 parent 97829b9 commit dbe6985

File tree

8 files changed

+71
-62
lines changed

8 files changed

+71
-62
lines changed

‎jupyterlab_git/git.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -351,6 +351,7 @@ def read_notebook(content):
351351
else:
352352
return nbformat.reads(content, as_version=4)
353353

354+
# TODO Fix this in nbdime
354355
def remove_cell_ids(nb):
355356
for cell in nb.cells:
356357
del cell["id"]

‎src/commandsAndMenu.tsx

Lines changed: 41 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -471,45 +471,45 @@ export function addCommands(
471471

472472
diffWidget.toolbar.addItem('spacer', Toolbar.createSpacerItem());
473473

474-
const refreshButton = new ToolbarButton({
475-
label: trans.__('Refresh'),
476-
onClick: async () => {
477-
await widget.refresh();
478-
refreshButton.hide();
479-
},
480-
tooltip: trans.__('Refresh diff widget'),
481-
className: 'jp-git-diff-refresh'
482-
});
483-
484-
const resolveButton = new ToolbarButton({
485-
label: trans.__('Mark as resolved'),
486-
onClick: async () => {
487-
try {
488-
const resolvedFile: string = await widget.getResolvedFile();
489-
await serviceManager.contents.save(model.filename, {
490-
type: 'file',
491-
format: 'text',
492-
content: resolvedFile
493-
});
494-
await gitModel.add(model.filename);
495-
await gitModel.refresh();
496-
} catch (reason) {
497-
logger.log({
498-
message: reason.message ?? reason,
499-
level: Level.ERROR
500-
});
501-
} finally {
502-
diffWidget.dispose();
503-
}
504-
},
505-
tooltip: trans.__('Mark file as resolved'),
506-
className: 'jp-git-diff-resolve'
507-
});
508-
509474
// Do not allow the user to refresh during merge conflicts
510-
if (model.isConflict) {
475+
if (model.hasConflict) {
476+
const resolveButton = new ToolbarButton({
477+
label: trans.__('Mark as resolved'),
478+
onClick: async () => {
479+
try {
480+
const resolvedFile: string = await widget.getResolvedFile();
481+
await serviceManager.contents.save(model.filename, {
482+
type: 'file',
483+
format: 'text',
484+
content: resolvedFile
485+
});
486+
await gitModel.add(model.filename);
487+
await gitModel.refresh();
488+
} catch (reason) {
489+
logger.log({
490+
message: reason.message ?? reason,
491+
level: Level.ERROR
492+
});
493+
} finally {
494+
diffWidget.dispose();
495+
}
496+
},
497+
tooltip: trans.__('Mark file as resolved'),
498+
className: 'jp-git-diff-resolve'
499+
});
500+
511501
diffWidget.toolbar.addItem('resolve', resolveButton);
512502
} else {
503+
const refreshButton = new ToolbarButton({
504+
label: trans.__('Refresh'),
505+
onClick: async () => {
506+
await widget.refresh();
507+
refreshButton.hide();
508+
},
509+
tooltip: trans.__('Refresh diff widget'),
510+
className: 'jp-git-diff-refresh'
511+
});
512+
513513
refreshButton.hide();
514514
diffWidget.toolbar.addItem('refresh', refreshButton);
515515

@@ -526,7 +526,7 @@ export function addCommands(
526526
content.addWidget(widget);
527527
} catch (reason) {
528528
console.error(reason);
529-
const msg = `Load Diff Model Error (${reason.message ?? reason})`;
529+
const msg = `Load Diff Model Error (${reason.message || reason})`;
530530
modelIsLoading.reject(msg);
531531
}
532532
}
@@ -621,7 +621,10 @@ export function addCommands(
621621
: { git: diffContext.currentRef };
622622

623623
// Base props used for Diff Model
624-
const props: Omit<Git.Diff.IModel<string>, 'changed' | 'isConflict'> = {
624+
const props: Omit<
625+
Git.Diff.IModel<string>,
626+
'changed' | 'hasConflict'
627+
> = {
625628
challenger: {
626629
content: async () => {
627630
return requestAPI<Git.IDiffContent>(

‎src/components/FileList.tsx

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import { ListChildComponentProps } from 'react-window';
99
import { addMenuItems, CommandArguments } from '../commandsAndMenu';
1010
import { getDiffProvider, GitExtension } from '../model';
1111
import { hiddenButtonStyle } from '../style/ActionButtonStyle';
12-
import { fileListWrapperClass } from '../style/FileListStyle';
12+
import { fileListWrapperClass, unmergedRowStyle } from '../style/FileListStyle';
1313
import {
1414
addIcon,
1515
diffIcon,
@@ -362,6 +362,7 @@ export class FileList extends React.Component<IFileListProps, IFileListState> {
362362
const diffButton = this._createDiffButton(file);
363363
return (
364364
<FileItem
365+
className={unmergedRowStyle}
365366
trans={this.props.trans}
366367
actions={!file.is_binary && diffButton}
367368
contextMenu={this.openContextMenu}
@@ -370,7 +371,7 @@ export class FileList extends React.Component<IFileListProps, IFileListState> {
370371
selected={this._isSelectedFile(file)}
371372
selectFile={this.updateSelectedFile}
372373
onDoubleClick={() => this._openDiffView(file)}
373-
style={{ ...style, color: 'var(--jp-warn-color0)' }}
374+
style={{ ...style }}
374375
/>
375376
);
376377
};

‎src/components/diff/NotebookDiff.ts

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ export class NotebookDiff
145145
if (this._areUnchangedCellsHidden !== v) {
146146
Private.toggleShowUnchanged(
147147
this._scroller,
148-
this._isConflict,
148+
this._hasConflict,
149149
this._areUnchangedCellsHidden
150150
);
151151
this._areUnchangedCellsHidden = v;
@@ -155,8 +155,8 @@ export class NotebookDiff
155155
/**
156156
* Helper to determine if a notebook merge should be shown.
157157
*/
158-
private get _isConflict(): boolean {
159-
return !!this._model.base;
158+
private get _hasConflict(): boolean {
159+
return this._model.hasConflict;
160160
}
161161

162162
/**
@@ -222,13 +222,13 @@ export class NotebookDiff
222222
try {
223223
await nbdWidget.init();
224224

225-
Private.markUnchangedRanges(this._scroller.node, this._isConflict);
225+
Private.markUnchangedRanges(this._scroller.node, this._hasConflict);
226226
} catch (reason) {
227227
// FIXME there is a bug in nbdime and init got reject due to recursion limit hit
228228
// console.error(`Failed to init notebook diff view: ${reason}`);
229229
// getReady.reject(reason);
230230
console.debug(`Failed to init notebook diff view: ${reason}`);
231-
Private.markUnchangedRanges(this._scroller.node, this._isConflict);
231+
Private.markUnchangedRanges(this._scroller.node, this._hasConflict);
232232
}
233233
} catch (reason) {
234234
this.showError(reason);
@@ -333,7 +333,7 @@ namespace Private {
333333
*/
334334
export function toggleShowUnchanged(
335335
root: Widget,
336-
isConflict: boolean,
336+
hasConflict: boolean,
337337
show?: boolean
338338
): void {
339339
const hiding = root.hasClass(HIDE_UNCHANGED_CLASS);
@@ -346,7 +346,7 @@ namespace Private {
346346
if (show) {
347347
root.removeClass(HIDE_UNCHANGED_CLASS);
348348
} else {
349-
markUnchangedRanges(root.node, isConflict);
349+
markUnchangedRanges(root.node, hasConflict);
350350
root.addClass(HIDE_UNCHANGED_CLASS);
351351
}
352352
root.update();
@@ -375,10 +375,10 @@ namespace Private {
375375
*/
376376
export function markUnchangedRanges(
377377
root: HTMLElement,
378-
isConflict: boolean
378+
hasConflict: boolean
379379
): void {
380-
const CELL_CLASS = isConflict ? CELLMERGE_CLASS : CELLDIFF_CLASS;
381-
const UNCHANGED_CLASS = isConflict
380+
const CELL_CLASS = hasConflict ? CELLMERGE_CLASS : CELLDIFF_CLASS;
381+
const UNCHANGED_CLASS = hasConflict
382382
? UNCHANGED_MERGE_CLASS
383383
: UNCHANGED_DIFF_CLASS;
384384

‎src/components/diff/PlainTextDiff.ts

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -64,8 +64,8 @@ export class PlainTextDiff
6464
/**
6565
* Helper to determine if three-way diff should be shown.
6666
*/
67-
private get _isConflict(): boolean {
68-
return this._base !== null;
67+
private get _hasConflict(): boolean {
68+
return this._model.hasConflict;
6969
}
7070

7171
/**
@@ -99,7 +99,7 @@ export class PlainTextDiff
9999
this.createDiffView(
100100
this._challenger,
101101
this._reference,
102-
this._isConflict ? this._base : undefined
102+
this._hasConflict ? this._base : null
103103
);
104104
}
105105
})
@@ -141,7 +141,7 @@ export class PlainTextDiff
141141
this.createDiffView(
142142
this._challenger,
143143
this._reference,
144-
this._isConflict ? this._base : undefined
144+
this._hasConflict ? this._base : null
145145
);
146146

147147
this._challenger = null;
@@ -193,8 +193,8 @@ export class PlainTextDiff
193193
};
194194

195195
// Show three-way diff on merge conflict
196-
// Note: Empty base content ("") can be an edge case.
197-
if (baseContent !== undefined) {
196+
// Note: Empty base content ("") is an edge case.
197+
if (baseContent !== null && baseContent !== undefined) {
198198
options = {
199199
...options,
200200
value: baseContent,

‎src/components/diff/model.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import { Git } from '../../tokens';
66
* Base DiffModel class
77
*/
88
export class DiffModel<T> implements IDisposable, Git.Diff.IModel<T> {
9-
constructor(props: Omit<Git.Diff.IModel<T>, 'changed' | 'isConflict'>) {
9+
constructor(props: Omit<Git.Diff.IModel<T>, 'changed' | 'hasConflict'>) {
1010
this._challenger = props.challenger;
1111
this._filename = props.filename;
1212
this._reference = props.reference;
@@ -86,10 +86,10 @@ export class DiffModel<T> implements IDisposable, Git.Diff.IModel<T> {
8686
}
8787

8888
/**
89-
* Helper to check if the file is conflicted.
89+
* Helper to check if the file has conflicts.
9090
*/
91-
get isConflict(): boolean {
92-
return !!this._base;
91+
get hasConflict(): boolean {
92+
return this._base !== undefined;
9393
}
9494

9595
/**

‎src/style/FileListStyle.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,3 +7,7 @@ export const fileListWrapperClass = style({
77
overflow: 'hidden',
88
overflowY: 'auto'
99
});
10+
11+
export const unmergedRowStyle = style({
12+
color: 'var(--jp-warn-color0) !important'
13+
});

‎src/tokens.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -568,9 +568,9 @@ export namespace Git {
568568
*/
569569
base?: IContent<T>;
570570
/**
571-
* Helper to check if the file is conflicted.
571+
* Helper to check if the file has conflicts.
572572
*/
573-
isConflict: boolean;
573+
hasConflict?: boolean;
574574
}
575575

576576
/**

0 commit comments

Comments
 (0)