Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

Multiple ColorEditor fixes #10401

Merged
merged 4 commits into from
Jan 22, 2015
Merged

Multiple ColorEditor fixes #10401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
52254f4
Multiple ColorEditor fixes
Jan 17, 2015
43844fc
Fix for #2805
Jan 17, 2015
5ed4ee4
Code review changes
Jan 22, 2015
dfc1341
Ensure _marker is not null
Jan 22, 2015
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
11 changes: 10 additions & 1 deletion src/extensions/default/InlineColorEditor/ColorEditor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,15 @@ define(function (require, exports, module) {
});
};

/**
* Checks whether colorVal is a valid color
* @param {!string} colorVal
* @return {boolean} Whether colorVal is valid
*/
ColorEditor.prototype.isValidColor = function (colorVal) {
return tinycolor(colorVal).isValid();
};

/**
* Sets _hsv and _color based on an HSV input, and updates the UI. Attempts to preserve
* the previous color format.
Expand Down Expand Up @@ -518,7 +527,7 @@ define(function (require, exports, module) {

/**
* Handles undo gestures while color picker has focus. We don't want to let CodeMirror's
* usual undo logic run since it will destroy our bookmarks.
* usual undo logic run since it will destroy our marker.
*/
ColorEditor.prototype.undo = function () {
if (this._originalColor.toString() !== this._color.toString()) {
Expand Down
75 changes: 35 additions & 40 deletions src/extensions/default/InlineColorEditor/InlineColorEditor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,11 @@ define(function (require, exports, module) {
/**
* Inline widget containing a ColorEditor control
* @param {!string} color Initially selected color
* @param {!CodeMirror.Bookmark} startBookmark
* @param {!CodeMirror.Bookmark} endBookmark
* @param {!CodeMirror.TextMarker} marker
*/
function InlineColorEditor(color, startBookmark, endBookmark) {
function InlineColorEditor(color, marker) {
this._color = color;
this._startBookmark = startBookmark;
this._endBookmark = endBookmark;
this._marker = marker;
this._isOwnChange = false;
this._isHostChange = false;
this._origin = "+InlineColorEditor_" + (lastOriginId++);
Expand All @@ -69,17 +67,10 @@ define(function (require, exports, module) {
InlineColorEditor.prototype._color = null;

/**
* Start of the range of code we're attached to; _startBookmark.find() may by null if sync is lost.
* @type {!CodeMirror.Bookmark}
* Range of code we're attached to; _marker.find() may by null if sync is lost.
* @type {!CodeMirror.TextMarker}
*/
InlineColorEditor.prototype._startBookmark = null;

/**
* End of the range of code we're attached to; _endBookmark.find() may by null if sync is lost or even
* in some cases when it's not. Call getCurrentRange() for the definitive text range we're attached to.
* @type {!CodeMirror.Bookmark}
*/
InlineColorEditor.prototype._endBookmark = null;
InlineColorEditor.prototype._marker = null;

/** @type {boolean} True while we're syncing a color picker change into the code editor */
InlineColorEditor.prototype._isOwnChange = null;
Expand All @@ -97,25 +88,24 @@ define(function (require, exports, module) {
* @return {?{start:{line:number, ch:number}, end:{line:number, ch:number}}}
*/
InlineColorEditor.prototype.getCurrentRange = function () {
var start, end;
var pos, start, end;

start = this._startBookmark.find();
pos = this._marker && this._marker.find();

start = pos && pos.from;
if (!start) {
return null;
}

end = this._endBookmark.find();
end = pos.to;
if (!end) {
end = { line: start.line };
end = {line: start.line};
}

// Even if we think we have a good end bookmark, we want to run the
// regexp match to see if there's a valid match that extends past the bookmark.
// Even if we think we have a good range end, we want to run the
// regexp match to see if there's a valid match that extends past the marker.
// This can happen if the user deletes the end of the existing color and then
// types some more.
// TODO: when we migrate to CodeMirror v3, we might be able to use markText()
// instead of two bookmarks to track the range. (In our current old version of
// CodeMirror v2, markText() isn't robust enough for this case.)

var line = this.hostEditor.document.getLine(start.line),
matches = line.substr(start.ch).match(ColorUtils.COLOR_REGEX);
Expand All @@ -124,12 +114,12 @@ define(function (require, exports, module) {
// the matched length here.
if (matches && (end.ch === undefined || end.ch - start.ch < matches[0].length)) {
end.ch = start.ch + matches[0].length;
this._endBookmark.clear();
this._endBookmark = this.hostEditor._codeMirror.setBookmark(end);
this._marker.clear();
this._marker = this.hostEditor._codeMirror.markText(start, end);
Copy link
Member

Choose a reason for hiding this comment

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

@marcelgerber Would you be interested in making a similar cleanup to the easing function editor at some point? Bonus points for finding a way to share some bookmark-management code between the two :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did that and found that there's a lot of shared code in getCurrentRange - where to put shared utility functions for Inline Editor Extensions?

}

if (end.ch === undefined) {
// We were unable to resync the end bookmark.
// We were unable to resync the marker.
return null;
} else {
return {start: start, end: end};
Expand All @@ -150,14 +140,20 @@ define(function (require, exports, module) {

// Don't push the change back into the host editor if it came from the host editor.
if (!this._isHostChange) {
var endPos = {
line: range.start.line,
ch: range.start.ch + colorString.length
};
this._isOwnChange = true;
this.hostEditor.document.batchOperation(function () {
// Replace old color in code with the picker's color, and select it
self.hostEditor.setSelection(range.start, range.end); // workaround for #2805
self.hostEditor.document.replaceRange(colorString, range.start, range.end, self._origin);
self.hostEditor.setSelection(range.start, {
line: range.start.line,
ch: range.start.ch + colorString.length
});
self.hostEditor.setSelection(range.start, endPos);
if (self._marker) {
self._marker.clear();
self._marker = self.hostEditor._codeMirror.markText(range.start, endPos);
}
});
this._isOwnChange = false;
}
Expand Down Expand Up @@ -202,11 +198,8 @@ define(function (require, exports, module) {
InlineColorEditor.prototype.onClosed = function () {
InlineColorEditor.prototype.parentClass.onClosed.apply(this, arguments);

if (this._startBookmark) {
this._startBookmark.clear();
}
if (this._endBookmark) {
this._endBookmark.clear();
if (this._marker) {
this._marker.clear();
}

var doc = this.hostEditor.document;
Expand Down Expand Up @@ -273,15 +266,17 @@ define(function (require, exports, module) {
if (range) {
var newColor = this.hostEditor.document.getRange(range.start, range.end);
if (newColor !== this._color) {
this._isHostChange = true;
this.colorEditor.setColorFromString(newColor);
this._isHostChange = false;
if (this.colorEditor.isValidColor(newColor)) { // only update the editor if the color string is valid
this._isHostChange = true;
this.colorEditor.setColorFromString(newColor);
this._isHostChange = false;
}
}
} else {
// The edit caused our range to become invalid. Close the editor.
this.close();
}
};

module.exports.InlineColorEditor = InlineColorEditor;
exports.InlineColorEditor = InlineColorEditor;
});
16 changes: 7 additions & 9 deletions src/extensions/default/InlineColorEditor/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,10 @@ define(function (require, exports, module) {
*
* @param {Editor} hostEditor
* @param {{line:Number, ch:Number}} pos
* @return {?{color:String, start:TextMarker, end:TextMarker}}
* @return {?{color:String, marker:TextMarker}}
*/
function prepareEditorForProvider(hostEditor, pos) {
var colorRegEx, cursorLine, match, sel, start, end, startBookmark, endBookmark;
var colorRegEx, cursorLine, match, sel, start, end, endPos, marker;

sel = hostEditor.getSelection();
if (sel.start.line !== sel.end.line) {
Expand All @@ -68,16 +68,14 @@ define(function (require, exports, module) {
// Adjust pos to the beginning of the match so that the inline editor won't get
// dismissed while we're updating the color with the new values from user's inline editing.
pos.ch = start;
endPos = {line: pos.line, ch: end};

startBookmark = hostEditor._codeMirror.setBookmark(pos);
endBookmark = hostEditor._codeMirror.setBookmark({ line: pos.line, ch: end });

hostEditor.setSelection(pos, { line: pos.line, ch: end });
marker = hostEditor._codeMirror.markText(pos, endPos);
hostEditor.setSelection(pos, endPos);

return {
color: match[0],
start: startBookmark,
end: endBookmark
marker: marker
};
}

Expand All @@ -98,7 +96,7 @@ define(function (require, exports, module) {
if (!context) {
return null;
} else {
inlineColorEditor = new InlineColorEditor(context.color, context.start, context.end);
inlineColorEditor = new InlineColorEditor(context.color, context.marker);
inlineColorEditor.load(hostEditor);

result = new $.Deferred();
Expand Down
36 changes: 22 additions & 14 deletions src/extensions/default/InlineColorEditor/unittests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,16 @@ define(function (require, exports, module) {
expect(inline.getCurrentRange()).toBeTruthy();
});
});

it("should update correct range of host document when the in-editor color string is invalid", function () {
makeColorEditor({line: 1, ch: 18});
runs(function () {
testDocument.replaceRange("", {line: 1, ch: 22}, {line: 1, ch: 24});
inline.colorEditor.setColorFromString("#c0c0c0");
expect(inline.getCurrentRange()).toEqual({start: {line: 1, ch: 16}, end: {line: 1, ch: 23}});
expect(testDocument.getRange({line: 1, ch: 16}, {line: 1, ch: 23})).toBe("#c0c0c0");
});
});

});

Expand All @@ -232,12 +242,12 @@ define(function (require, exports, module) {
});
});

it("should close itself if edit is made that destroys end bookmark and leaves color invalid", function () {
it("should close itself if edit is made that destroys end textmark and leaves color invalid", function () {
makeColorEditor({line: 1, ch: 18});
runs(function () {
spyOn(inline, "close");

// Replace everything including the semicolon, so it crosses the bookmark boundary.
// Replace everything including the semicolon, so it crosses the textmark boundary.
testDocument.replaceRange("rgb(255, 25", {line: 1, ch: 16}, {line: 1, ch: 24});
expect(inline.close).toHaveBeenCalled();
});
Expand All @@ -256,25 +266,23 @@ define(function (require, exports, module) {
});
});

it("should not update the end bookmark to a shorter valid match if the bookmark still exists and the color becomes invalid", function () {
it("should not update the end textmark and the color shown to a shorter valid match if the marker still exists and the color becomes invalid", function () {
makeColorEditor({line: 1, ch: 18});
runs(function () {
testDocument.replaceRange("", {line: 1, ch: 22}, {line: 1, ch: 23});
expect(inline._color).toBe("#abcde");
expect(inline._color).toBe("#abcdef");
expect(inline.getCurrentRange()).toEqual({start: {line: 1, ch: 16}, end: {line: 1, ch: 22}});
});
});

// TODO: (issue #2166) The following test fails because if the end bookmark is deleted, we match the shorter
// #xxx string at the beginning of the color and assume that's valid, and then reset the bookmark
// to the end of that location.
// it("should not update the end bookmark to a shorter valid match if the bookmark no longer exists and the color becomes invalid", function () {
// makeColorEditor({line: 1, ch: 18}).done(function (inline) {
// testDocument.replaceRange("", {line: 1, ch: 22}, {line: 1, ch: 24});
// expect(inline._color).toBe("#abcde");
// expect(inline.getCurrentRange()).toEqual({start: {line: 1, ch: 16}, end: {line: 1, ch: 22}});
// });
// });
it("should not update the end textmark and the color shown to a shorter valid match if the marker no longer exists and the color becomes invalid", function () {
makeColorEditor({line: 1, ch: 18});
runs(function () {
testDocument.replaceRange("", {line: 1, ch: 22}, {line: 1, ch: 24});
expect(inline._color).toBe("#abcdef");
expect(inline.getCurrentRange()).toEqual({start: {line: 1, ch: 16}, end: {line: 1, ch: 22}});
});
});

});

Expand Down