Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.
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
10 changes: 5 additions & 5 deletions lib/atom/decoration.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,12 @@ class BareDecoration extends React.Component {
componentDidUpdate(prevProps) {
if (this.props.editorHolder !== prevProps.editorHolder) {
this.editorSub.dispose();
this.editorSub = this.state.editorHolder.observe(this.observeParents);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you change from state to props because that's easier to test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it was actually just broken before 😅 We hadn't had any tests that covered an update of an existing component, and apparently it doesn't come up in our package yet. But editorHolder is a member of the state in the wrapper class that reads Context, not in the BareDecoration!

this.editorSub = this.props.editorHolder.observe(this.observeParents);
}

if (this.props.decorableHolder !== prevProps.decorableHolder) {
this.decorableSub.dispose();
this.decorableSub = this.state.decorableHolder.observe(this.observeParents);
this.decorableSub = this.props.decorableHolder.observe(this.observeParents);
}

if (
Expand All @@ -95,10 +95,10 @@ class BareDecoration extends React.Component {
this.decorationHolder.map(decoration => decoration.destroy());

const editorValid = this.props.editorHolder.map(editor => !editor.isDestroyed()).getOr(false);
const markableValid = this.props.decorableHolder.map(decorable => !decorable.isDestroyed()).getOr(false);
const decorableValid = this.props.decorableHolder.map(decorable => !decorable.isDestroyed()).getOr(false);

// Ensure the Marker or MarkerLayer corresponds to the context's TextEditor
const markableMatches = this.props.decorableHolder.map(decorable => this.props.editorHolder.map(editor => {
const decorableMatches = this.props.decorableHolder.map(decorable => this.props.editorHolder.map(editor => {
const layer = decorable.layer || decorable;
const displayLayer = editor.getMarkerLayer(layer.id);
if (!displayLayer) {
Expand All @@ -110,7 +110,7 @@ class BareDecoration extends React.Component {
return true;
}).getOr(false)).getOr(false);

if (!editorValid || !markableValid || !markableMatches) {
if (!editorValid || !decorableValid || !decorableMatches) {
return;
}

Expand Down
9 changes: 2 additions & 7 deletions lib/controllers/editor-conflict-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,7 @@ export default class EditorConflictController extends React.Component {
commandRegistry: PropTypes.object.isRequired,
resolutionProgress: PropTypes.object.isRequired,
isRebase: PropTypes.bool.isRequired,
refreshResolutionProgress: PropTypes.func,
}

static defaultProps = {
refreshResolutionProgress: () => {},
refreshResolutionProgress: PropTypes.func.isRequired,
}

constructor(props, context) {
Expand All @@ -49,7 +45,6 @@ export default class EditorConflictController extends React.Component {
const buffer = this.props.editor.getBuffer();

this.subscriptions.add(
this.props.editor.onDidStopChanging(() => this.forceUpdate()),
this.props.editor.onDidDestroy(() => this.props.refreshResolutionProgress(this.props.editor.getPath())),
buffer.onDidReload(() => this.reparseConflicts()),
);
Expand Down Expand Up @@ -171,7 +166,7 @@ export default class EditorConflictController extends React.Component {
}

dismissConflicts(conflicts) {
this.setState((prevState, props) => {
this.setState(prevState => {
const {added} = compareSets(new Set(conflicts), prevState.conflicts);
return {conflicts: added};
});
Expand Down
1 change: 1 addition & 0 deletions lib/models/conflicts/banner.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export default class Banner {
revert() {
const range = this.getMarker().getBufferRange();
this.editor.setTextInBufferRange(range, this.originalText);
this.getMarker().setBufferRange(range);
}

delete() {
Expand Down
1 change: 1 addition & 0 deletions lib/models/conflicts/side.js
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ export default class Side {
revert() {
const range = this.getMarker().getBufferRange();
this.editor.setTextInBufferRange(range, this.originalText);
this.getMarker().setBufferRange(range);
}

deleteBanner() {
Expand Down
10 changes: 6 additions & 4 deletions lib/models/repository-states/present.js
Original file line number Diff line number Diff line change
Expand Up @@ -610,9 +610,6 @@ export default class Present extends State {
const bundle = await this.git().getStatusBundle();
const results = await this.formatChangedFiles(bundle);
results.branch = bundle.branch;
if (!results.branch.aheadBehind) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is unused, I take it? good catch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup! getStatusBundle will always return an empty aheadBehind with our versions of git and what-the-status.

results.branch.aheadBehind = {ahead: null, behind: null};
}
return results;
} catch (err) {
if (err instanceof LargeRepoError) {
Expand Down Expand Up @@ -855,7 +852,7 @@ export default class Present extends State {
// Direct blob access

getBlobContents(sha) {
return this.cache.getOrSet(Keys.blob(sha), () => {
return this.cache.getOrSet(Keys.blob.oneWith(sha), () => {
return this.git().getBlobContents(sha);
});
}
Expand All @@ -876,6 +873,7 @@ export default class Present extends State {

// Cache

/* istanbul ignore next */
getCache() {
return this.cache;
}
Expand Down Expand Up @@ -974,6 +972,7 @@ class Cache {
this.didUpdate();
}

/* istanbul ignore next */
[Symbol.iterator]() {
return this.storage[Symbol.iterator]();
}
Expand All @@ -988,6 +987,7 @@ class Cache {
this.emitter.emit('did-update');
}

/* istanbul ignore next */
onDidUpdate(callback) {
return this.emitter.on('did-update', callback);
}
Expand Down Expand Up @@ -1025,6 +1025,7 @@ class CacheKey {
}
}

/* istanbul ignore next */
toString() {
return `CacheKey(${this.primary})`;
}
Expand All @@ -1041,6 +1042,7 @@ class GroupKey {
}
}

/* istanbul ignore next */
toString() {
return `GroupKey(${this.group})`;
}
Expand Down
63 changes: 63 additions & 0 deletions test/atom/decoration.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,69 @@ describe('Decoration', function() {
});
});

describe('when called with changed props', function() {
let wrapper, originalDecoration;

beforeEach(function() {
const app = (
<Decoration
editor={editor}
decorable={marker}
type="line"
position="head"
className="something"
/>
);
wrapper = mount(app);

originalDecoration = editor.getLineDecorations({position: 'head', class: 'something'})[0];
});

it('redecorates when a new Editor and Marker are provided', async function() {
const newEditor = await workspace.open(require.resolve('../../package.json'));
const newMarker = newEditor.markBufferRange([[0, 0], [2, 0]]);

wrapper.setProps({editor: newEditor, decorable: newMarker});

assert.isTrue(originalDecoration.isDestroyed());
assert.lengthOf(editor.getLineDecorations({position: 'head', class: 'something'}), 0);
assert.lengthOf(newEditor.getLineDecorations({position: 'head', class: 'something'}), 1);
});

it('redecorates when a new MarkerLayer is provided', function() {
const newLayer = editor.addMarkerLayer();

wrapper.setProps({decorable: newLayer, decorateMethod: 'decorateMarkerLayer'});

assert.isTrue(originalDecoration.isDestroyed());
assert.lengthOf(editor.getLineDecorations({position: 'head', class: 'something'}), 0);

// Turns out Atom doesn't provide any public way to query the markers on a layer.
assert.lengthOf(
Array.from(editor.decorationManager.layerDecorationsByMarkerLayer.get(newLayer)),
1,
);
});

it('updates decoration properties', function() {
wrapper.setProps({className: 'different'});

assert.lengthOf(editor.getLineDecorations({position: 'head', class: 'something'}), 0);
assert.lengthOf(editor.getLineDecorations({position: 'head', class: 'different'}), 1);
assert.isFalse(originalDecoration.isDestroyed());
assert.strictEqual(originalDecoration.getProperties().class, 'different');
});

it('does not redecorate when the decorable is on the wrong TextEditor', async function() {
const newEditor = await workspace.open(require.resolve('../../package.json'));

wrapper.setProps({editor: newEditor});

assert.isTrue(originalDecoration.isDestroyed());
assert.lengthOf(editor.getLineDecorations({}), 0);
});
});

it('destroys its decoration on unmount', function() {
const app = (
<Decoration
Expand Down
71 changes: 71 additions & 0 deletions test/controllers/editor-conflict-controller.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,49 @@ describe('EditorConflictController', function() {
assert.strictEqual(conflicts[2].getChosenSide(), conflicts[2].getSide(THEIRS));
});

it('resolves multiple conflicts as "ours"', function() {
assert.isFalse(conflicts[0].isResolved());
assert.isFalse(conflicts[1].isResolved());
assert.isFalse(conflicts[2].isResolved());

editor.setCursorBufferPosition([8, 3]); // On "Your changes"
editor.addCursorAtBufferPosition([11, 2]); // On "Text in between 0 and 1."
editor.addCursorAtBufferPosition([14, 5]); // On "My middle changes"
editor.addCursorAtBufferPosition([15, 0]); // On "======="
commandRegistry.dispatch(editorView, 'github:resolve-as-ours');

assert.isTrue(conflicts[0].isResolved());
assert.strictEqual(conflicts[0].getChosenSide(), conflicts[0].getSide(OURS));
assert.deepEqual(conflicts[0].getUnchosenSides(), [conflicts[0].getSide(THEIRS)]);

assert.isTrue(conflicts[1].isResolved());
assert.strictEqual(conflicts[1].getChosenSide(), conflicts[1].getSide(OURS));
assert.deepEqual(conflicts[1].getUnchosenSides(), [conflicts[1].getSide(THEIRS)]);

assert.isFalse(conflicts[2].isResolved());
});

it('resolves multiple conflicts as "theirs"', function() {
assert.isFalse(conflicts[0].isResolved());
assert.isFalse(conflicts[1].isResolved());
assert.isFalse(conflicts[2].isResolved());

editor.setCursorBufferPosition([8, 3]); // On "Your changes"
editor.addCursorAtBufferPosition([11, 2]); // On "Text in between 0 and 1."
editor.addCursorAtBufferPosition([22, 5]); // On "More of my changes"
commandRegistry.dispatch(editorView, 'github:resolve-as-theirs');

assert.isTrue(conflicts[0].isResolved());
assert.strictEqual(conflicts[0].getChosenSide(), conflicts[0].getSide(THEIRS));
assert.deepEqual(conflicts[0].getUnchosenSides(), [conflicts[0].getSide(OURS)]);

assert.isFalse(conflicts[1].isResolved());

assert.isTrue(conflicts[2].isResolved());
assert.strictEqual(conflicts[2].getChosenSide(), conflicts[2].getSide(THEIRS));
assert.deepEqual(conflicts[2].getUnchosenSides(), [conflicts[2].getSide(OURS)]);
});

it('disregards conflicts with cursors on both sides', function() {
editor.setCursorBufferPosition([6, 3]); // On "Multi-line even"
editor.addCursorAtBufferPosition([14, 1]); // On "My middle changes"
Expand Down Expand Up @@ -316,6 +359,26 @@ describe('EditorConflictController', function() {

await assert.async.isTrue(refreshResolutionProgress.calledWith(fixtureFile));
});

it('performs a resolution from the context menu', function() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh cool - I did not know resolving from the context menu was a thing.

const conflict = conflicts[1];
assert.isFalse(conflict.isResolved());

wrapper.find('ConflictController').at(1).prop('resolveAsSequence')([OURS]);

assert.isTrue(conflict.isResolved());
assert.strictEqual(conflict.getChosenSide(), conflict.getSide(OURS));
});

it('dismisses a conflict from the context menu', function() {
const conflict = conflicts[2];

wrapper.find('ConflictController').at(2).prop('dismiss')();
wrapper.update();

assert.lengthOf(wrapper.find(ConflictController), 2);
assert.isFalse(wrapper.find(ConflictController).someWhere(cc => cc.prop('conflict') === conflict));
});
});

describe('on a file with 3-way diff markers', function() {
Expand Down Expand Up @@ -442,4 +505,12 @@ describe('EditorConflictController', function() {
assert.equal(editor.getText(), 'These are my changes\nThese are your changes\n\nPast the end\n');
});
});

it('cleans up its subscriptions when unmounting', async function() {
await useFixture('triple-2way-diff.txt');
wrapper.unmount();

editor.destroy();
assert.isFalse(refreshResolutionProgress.called);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
Before the start

<<<<<<< HEAD
These are my changes
=======
>>>>>>> master

Past the end
Loading