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

Factor out width-resizing logic for inline widgets into the InlineWidget base class #2285

Merged
merged 5 commits into from
Dec 11, 2012
Merged
Show file tree
Hide file tree
Changes from 3 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
2 changes: 1 addition & 1 deletion src/command/Menus.js
Original file line number Diff line number Diff line change
Expand Up @@ -748,7 +748,7 @@ define(function (require, exports, module) {
},
false);
}
ContextMenu.prototype = new Menu();
ContextMenu.prototype = Object.create(Menu.prototype);
Copy link
Member

Choose a reason for hiding this comment

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

ContextMenu is the only one of these that doesn't call its super constructor. But it easily could, since all Menu does is initialize this.id. Should we do that?

ContextMenu.prototype.constructor = ContextMenu;
ContextMenu.prototype.parentClass = Menu.prototype;

Expand Down
9 changes: 7 additions & 2 deletions src/editor/InlineTextEditor.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ define(function (require, exports, module) {
/* @type {Array.<{Editor}>}*/
this.editors = [];
}
InlineTextEditor.prototype = new InlineWidget();
InlineTextEditor.prototype = Object.create(InlineWidget.prototype);
InlineTextEditor.prototype.constructor = InlineTextEditor;
InlineTextEditor.prototype.parentClass = InlineWidget.prototype;

Expand Down Expand Up @@ -125,6 +125,8 @@ define(function (require, exports, module) {
* Called any time inline was closed, whether manually (via close()) or automatically
*/
InlineTextEditor.prototype.onClosed = function () {
InlineTextEditor.prototype.parentClass.onClosed.call(this);

_syncGutterWidths(this.hostEditor);

this.editors.forEach(function (editor) {
Expand Down Expand Up @@ -170,6 +172,8 @@ define(function (require, exports, module) {
* editor is constructed and added to the DOM
*/
InlineTextEditor.prototype.onAdded = function () {
InlineTextEditor.prototype.parentClass.onAdded.call(this);

this.editors.forEach(function (editor) {
editor.refresh();
});
Expand Down Expand Up @@ -260,7 +264,7 @@ define(function (require, exports, module) {
* @param {Editor} hostEditor
*/
InlineTextEditor.prototype.load = function (hostEditor) {
this.hostEditor = hostEditor;
InlineTextEditor.prototype.parentClass.load.call(this, hostEditor);

// TODO: incomplete impelementation. It's not clear yet if InlineTextEditor
// will fuction as an abstract class or as generic inline editor implementation
Expand All @@ -271,6 +275,7 @@ define(function (require, exports, module) {
* Called when the editor containing the inline is made visible.
*/
InlineTextEditor.prototype.onParentShown = function () {
InlineTextEditor.prototype.parentClass.onParentShown.call(this);
// We need to call this explicitly whenever the host editor is reshown, since
// we don't actually resize the inline editor while its host is invisible (see
// isFullyVisible() check in sizeInlineWidgetToContents()).
Expand Down
37 changes: 31 additions & 6 deletions src/editor/InlineWidget.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ define(function (require, exports, module) {
e.stopImmediatePropagation();
}
});

this.updateWidth = this.updateWidth.bind(this);
}
InlineWidget.prototype.htmlContent = null;
InlineWidget.prototype.$htmlContent = null;
Expand All @@ -63,6 +65,29 @@ define(function (require, exports, module) {
*/
InlineWidget.prototype.height = 0;

/**
* Automatically updates the width of the inline editor when the parent editor's width changes due to
* edits or window resizes.
*/
InlineWidget.prototype.updateWidth = function () {
// Set the minimum width of the widget (which doesn't include the padding) to the width
// of CodeMirror's linespace, so that the total width will be at least as large as the
// width of the host editor's code plus any internal padding required by the widget.
// We can't just set the min-width to 100% because that would be 100% of the clientWidth of
// the host editor, rather than the overall scroll width.
// We also can't just use the host editor's scrollWidth, because if the host editor's own
// content becomes less wide, our own width will continue to prop open the host editor's
// scrollWidth.
// So instead, we peg our width to the right edge of CodeMirror's lineSpace (which is its
// width plus its offset from the left edge of the $htmlContent container).
// If the lineSpace is less than the scroller's clientWidth, we want to use the clientWidth instead.
// This is a bit of a hack since it relies on knowing some detail about the innards of CodeMirror.
var lineSpace = this.hostEditor._getLineSpaceElement(),
scroller = this.hostEditor.getScrollerElement(),
minWidth = Math.max(scroller.clientWidth, $(lineSpace).offset().left - this.$htmlContent.offset().left + lineSpace.scrollWidth);
this.$htmlContent.css("min-width", minWidth + "px");
};

/**
* Closes this inline widget and all its contained Editors
*/
Expand All @@ -76,26 +101,26 @@ define(function (require, exports, module) {
* Called any time inline is closed, whether manually or automatically
*/
InlineWidget.prototype.onClosed = function () {
// do nothing - base implementation
$(this.hostEditor).off("change", this.updateWidth);
$(window).off("resize", this.updateWidth);
};

/**
* Called once content is parented in the host editor's DOM. Useful for performing tasks like setting
* focus or measuring content, which require htmlContent to be in the DOM tree.
*/
InlineWidget.prototype.onAdded = function () {
// do nothing - base implementation
// Autosize the inline widget to the scrollable width of the main editor.
$(window).on("resize", this.updateWidth);
$(this.hostEditor).on("change", this.updateWidth);
window.setTimeout(this.updateWidth, 0);
};

/**
* @param {Editor} hostEditor
*/
InlineWidget.prototype.load = function (hostEditor) {
this.hostEditor = hostEditor;

// TODO: incomplete impelementation. It's not clear yet if InlineTextEditor
// will fuction as an abstract class or as generic inline editor implementation
// that just shows a range of text. See CSSInlineEditor.css for an implementation of load()
};

/**
Expand Down
23 changes: 6 additions & 17 deletions src/editor/MultiRangeInlineEditor.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ define(function (require, exports, module) {

this._selectedRangeIndex = -1;
}
MultiRangeInlineEditor.prototype = new InlineTextEditor();
MultiRangeInlineEditor.prototype = Object.create(InlineTextEditor.prototype);
MultiRangeInlineEditor.prototype.constructor = MultiRangeInlineEditor;
MultiRangeInlineEditor.prototype.parentClass = InlineTextEditor.prototype;

Expand All @@ -113,7 +113,7 @@ define(function (require, exports, module) {
*
*/
MultiRangeInlineEditor.prototype.load = function (hostEditor) {
this.parentClass.load.call(this, hostEditor);
MultiRangeInlineEditor.prototype.parentClass.load.call(this, hostEditor);

// Container to hold all editors
var self = this;
Expand Down Expand Up @@ -320,7 +320,7 @@ define(function (require, exports, module) {
*/
MultiRangeInlineEditor.prototype.onClosed = function () {
// Superclass onClosed() destroys editor
this.parentClass.onClosed.call(this);
MultiRangeInlineEditor.prototype.parentClass.onClosed.call(this);

// remove resize handlers for relatedContainer
$(this.hostEditor).off("change", this._updateRelatedContainer);
Expand Down Expand Up @@ -416,17 +416,6 @@ define(function (require, exports, module) {

// Add extra padding to the right edge of the widget to account for the range list.
this.$htmlContent.css("padding-right", this.$relatedContainer.outerWidth() + "px");

// Set the minimum width of the widget (which doesn't include the padding) to the width
// of CodeMirror's linespace, so that the total width will be at least as large as the
// width of the host editor's code plus the padding for the range list. We need to do this
// rather than just setting min-width to 100% because adding padding for the range list
// actually pushes out the width of the container, so we would end up continuously
// growing the overall width.
// This is a bit of a hack since it relies on knowing some detail about the innards of CodeMirror.
var lineSpace = this.hostEditor._getLineSpaceElement(),
minWidth = $(lineSpace).offset().left - this.$htmlContent.offset().left + lineSpace.scrollWidth;
this.$htmlContent.css("min-width", minWidth + "px");
};

/**
Expand Down Expand Up @@ -476,7 +465,7 @@ define(function (require, exports, module) {
// Ignore when the editor's content got lost due to a deleted file
if (cause && cause.type === "deleted") { return; }
// Else yield to the parent's implementation
return this.parentClass._onLostContent.apply(this, arguments);
return MultiRangeInlineEditor.prototype.parentClass._onLostContent.apply(this, arguments);
};

/**
Expand Down Expand Up @@ -515,7 +504,7 @@ define(function (require, exports, module) {
*/
MultiRangeInlineEditor.prototype.sizeInlineWidgetToContents = function (force, ensureVisibility) {
// Size the code mirror editors height to the editor content
this.parentClass.sizeInlineWidgetToContents.call(this, force);
MultiRangeInlineEditor.prototype.parentClass.sizeInlineWidgetToContents.call(this, force);
Copy link
Member

Choose a reason for hiding this comment

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

If we don't change this to apply(), should probably add a comment that ensureVisibility is an arg added by the subclass -- otherwise it looks like a bug that it's not being passed along.

// Size the widget height to the max between the editor content and the related ranges list
var widgetHeight = Math.max(this.$relatedContainer.find(".related").height(), this.$editorsDiv.height());
this.hostEditor.setInlineWidgetHeight(this, widgetHeight, ensureVisibility);
Expand All @@ -530,6 +519,7 @@ define(function (require, exports, module) {
* @override
*/
MultiRangeInlineEditor.prototype.refresh = function () {
MultiRangeInlineEditor.prototype.parentClass.refresh.call(this);
this.sizeInlineWidgetToContents(true);
this._updateRelatedContainer();
this.editors.forEach(function (editor, j, arr) {
Expand All @@ -542,7 +532,6 @@ define(function (require, exports, module) {
* @returns {MultiRangeInlineEditor}
*/
function _getFocusedMultiRangeInlineEditor() {

var focusedMultiRangeInlineEditor = null,
result = EditorManager.getFocusedInlineWidget();

Expand Down
44 changes: 13 additions & 31 deletions src/extensions/default/InlineColorEditor/InlineColorEditor.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,7 @@ define(function (require, exports, module) {

this._handleColorChange = this._handleColorChange.bind(this);
this._handleHostDocumentChange = this._handleHostDocumentChange.bind(this);
this._handleHostResize = this._handleHostResize.bind(this);

$(window).on("resize", this._handleHostResize);
window.setTimeout(this._handleHostResize, 0);

InlineWidget.call(this);
}

Expand All @@ -65,7 +61,7 @@ define(function (require, exports, module) {
*/
InlineColorEditor.COLOR_REGEX = /#[a-f0-9]{6}|#[a-f0-9]{3}|rgb\( ?\b([0-9]{1,2}|1[0-9]{2}|2[0-4][0-9]|25[0-5])\b ?, ?\b([0-9]{1,2}|1[0-9]{2}|2[0-4][0-9]|25[0-5])\b ?, ?\b([0-9]{1,2}|1[0-9]{2}|2[0-4][0-9]|25[0-5])\b ?\)|rgba\( ?\b([0-9]{1,2}|1[0-9]{2}|2[0-4][0-9]|25[0-5])\b ?, ?\b([0-9]{1,2}|1[0-9]{2}|2[0-4][0-9]|25[0-5])\b ?, ?\b([0-9]{1,2}|1[0-9]{2}|2[0-4][0-9]|25[0-5])\b ?, ?\b(1|0|0\.[0-9]{1,3}) ?\)|hsl\( ?\b([0-9]{1,2}|[12][0-9]{2}|3[0-5][0-9]|360)\b ?, ?\b([0-9]{1,2}|100)\b% ?, ?\b([0-9]{1,2}|100)\b% ?\)|hsla\( ?\b([0-9]{1,2}|[12][0-9]{2}|3[0-5][0-9]|360)\b ?, ?\b([0-9]{1,2}|100)\b% ?, ?\b([0-9]{1,2}|100)\b% ?, ?\b(1|0|0\.[0-9]{1,3}) ?\)/gi;

InlineColorEditor.prototype = new InlineWidget();
InlineColorEditor.prototype = Object.create(InlineWidget.prototype);
InlineColorEditor.prototype.constructor = InlineColorEditor;
InlineColorEditor.prototype.parentClass = InlineWidget.prototype;

Expand Down Expand Up @@ -121,15 +117,15 @@ define(function (require, exports, module) {
// 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.editor.document.getLine(start.line),
var line = this.hostEditor.document.getLine(start.line),
matches = line.substr(start.ch).match(InlineColorEditor.COLOR_REGEX);

// Note that end.ch is exclusive, so we don't need to add 1 before comparing to
// 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.editor._codeMirror.setBookmark(end);
this._endBookmark = this.hostEditor._codeMirror.setBookmark(end);
}

if (end.ch === undefined) {
Expand All @@ -155,9 +151,9 @@ define(function (require, exports, module) {
if (!this._isHostChange) {
// Replace old color in code with the picker's color, and select it
this._isOwnChange = true;
this.editor.document.replaceRange(colorString, range.start, range.end);
this.hostEditor.document.replaceRange(colorString, range.start, range.end);
this._isOwnChange = false;
this.editor.setSelection(range.start, {
this.hostEditor.setSelection(range.start, {
line: range.start.line,
ch: range.start.ch + colorString.length
});
Expand All @@ -167,23 +163,15 @@ define(function (require, exports, module) {
}
};

/**
* Update the width of the inline editor based on the host editor's width.
*/
InlineColorEditor.prototype._handleHostResize = function () {
this.$htmlContent.css("min-width", this.hostEditor.getScrollerElement().scrollWidth + "px");
};

/**
* @override
* @param {!Editor} hostEditor
*/
InlineColorEditor.prototype.load = function (hostEditor) {
this.editor = hostEditor;
this.parentClass.load.call(this, hostEditor);
InlineColorEditor.prototype.parentClass.load.call(this, hostEditor);

// Create color picker control
var allColorsInDoc = this.editor.document.getText().match(InlineColorEditor.COLOR_REGEX);
var allColorsInDoc = this.hostEditor.document.getText().match(InlineColorEditor.COLOR_REGEX);
var swatchInfo = this._collateColors(allColorsInDoc, MAX_USED_COLORS);
this.colorEditor = new ColorEditor(this.$htmlContent, this._color, this._handleColorChange, swatchInfo);
};
Expand All @@ -198,9 +186,9 @@ define(function (require, exports, module) {
* Perform sizing & focus once we've been added to Editor's DOM
*/
InlineColorEditor.prototype.onAdded = function () {
this.parentClass.onAdded.call(this);
InlineColorEditor.prototype.parentClass.onAdded.call(this);

var doc = this.editor.document;
var doc = this.hostEditor.document;
doc.addRef();
$(doc).on("change", this._handleHostDocumentChange);

Expand All @@ -213,20 +201,18 @@ define(function (require, exports, module) {
* Called whenever the inline widget is closed, whether automatically or explicitly
*/
InlineColorEditor.prototype.onClosed = function () {
InlineColorEditor.prototype.parentClass.onClosed.call(this);

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

var doc = this.editor.document;
var doc = this.hostEditor.document;
$(doc).off("change", this._handleHostDocumentChange);
doc.releaseRef();

$(window).off("resize", this._handleHostResize);

this.parentClass.onClosed.call(this);
};

InlineColorEditor.prototype._sizeEditorToContent = function () {
Expand Down Expand Up @@ -282,18 +268,14 @@ define(function (require, exports, module) {
* When text in the code editor changes, update color picker to reflect it
*/
InlineColorEditor.prototype._handleHostDocumentChange = function () {
// Any host document change might change the scroll width, so we need to
// recalculate our own width.
this._handleHostResize();

// Don't push the change into the color editor if it came from the color editor.
if (this._isOwnChange) {
return;
}

var range = this.getCurrentRange();
if (range) {
var newColor = this.editor.document.getRange(range.start, range.end);
var newColor = this.hostEditor.document.getRange(range.start, range.end);
if (newColor !== this._color) {
this._isHostChange = true;
this.colorEditor.setColorFromString(newColor);
Expand Down
4 changes: 2 additions & 2 deletions src/extensions/samples/InlineImageViewer/InlineImageViewer.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ define(function (require, exports, module) {
this.fullPath = fullPath;
InlineWidget.call(this);
}
InlineImageViewer.prototype = new InlineWidget();
InlineImageViewer.prototype = Object.create(InlineWidget.prototype);
InlineImageViewer.prototype.constructor = InlineImageViewer;
InlineImageViewer.prototype.parentClass = InlineWidget.prototype;

Expand All @@ -49,7 +49,7 @@ define(function (require, exports, module) {
InlineImageViewer.prototype.$image = null;

InlineImageViewer.prototype.load = function (hostEditor) {
this.parentClass.load.call(this, hostEditor);
InlineImageViewer.prototype.parentClass.load.call(this, hostEditor);

this.$wrapperDiv = $(inlineEditorTemplate);

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't InlineImageViewier.onAdded() call up to the super method so that it gets proper width resizing?

Expand Down
6 changes: 3 additions & 3 deletions src/file/NativeFileSystem.js
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ define(function (require, exports, module) {
NativeFileSystem.FileEntry = function (name) {
NativeFileSystem.Entry.call(this, name, false);
};
NativeFileSystem.FileEntry.prototype = new NativeFileSystem.Entry();
NativeFileSystem.FileEntry.prototype = Object.create(NativeFileSystem.Entry.prototype);
Copy link
Member

Choose a reason for hiding this comment

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

Should these have prototype.parentClass and prototype.constructor set too? (I'm not actually sure I understand what function .constructor serves, but parentClass at least seems useful).

Copy link
Member

Choose a reason for hiding this comment

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

Or rather, setting prototype.constructor on a subclass is necessary if you want 'subclassInstance.constructor === Subclass' instead of '=== Superclass'. (I've yet to hear a satisfactory explanation of why this isn't needed for base classes, though... is the default prototype object special somehow?).

But anyway, it seems like we might as well set both here for completeness.


NativeFileSystem.FileEntry.prototype.toString = function () {
return "[FileEntry " + this.fullPath + "]";
Expand Down Expand Up @@ -535,7 +535,7 @@ define(function (require, exports, module) {

// TODO (issue #241): void removeRecursively (VoidCallback successCallback, optional ErrorCallback errorCallback);
};
NativeFileSystem.DirectoryEntry.prototype = new NativeFileSystem.Entry();
NativeFileSystem.DirectoryEntry.prototype = Object.create(NativeFileSystem.Entry.prototype);

NativeFileSystem.DirectoryEntry.prototype.toString = function () {
return "[DirectoryEntry " + this.fullPath + "]";
Expand Down Expand Up @@ -849,7 +849,7 @@ define(function (require, exports, module) {
this.onloadend = null;
};
// TODO (issue #241): extend EventTarget (draft status, not implememnted in webkit)
// NativeFileSystem.FileReader.prototype = new NativeFileSystem.EventTarget()
// NativeFileSystem.FileReader.prototype = Object.create(NativeFileSystem.EventTarget.prototype);

NativeFileSystem.FileReader.prototype.readAsArrayBuffer = function (blob) {
// TODO (issue #241): implement
Expand Down