Factor out width-resizing logic for inline widgets into the InlineWidget base class #2285
Conversation
…get base class. This addresses #2221 and #2218. As part of this, cleaned up some issues around how we're doing inheritance: * For #872, switched to using `Object.create()` to hook up prototypes (this avoids subtle bugs when binding event handlers in base classes). * Made it so all child classes call base class implementation for overridden functions (even if the base class impl is empty right now). The one exception is `close()`, which is a bit weird right now--will file a separate issue on this.
ghost
assigned 
|
@peterflynn Would you mind taking a look at this one? |
|
#2137 gives us a later opportunity to clean up the close()/_editorHasFocus() assumption -- ideally we would just eliminate the need for removeInlineWidget()'s shouldMoveFocus arg altogether, which removes the need for _editorHasFocus() and InlineImageViewer's close() override in one fell swoop. |
| @@ -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); | |||
There was a problem hiding this comment.
Would it be less bug-prone to have all the super calls be of this form? Otherwise anyone adding a new arg in the future must remember to either add it to the call() args or switch call() to apply().
There was a problem hiding this comment.
Good idea. I'll fix them up.
|
@njx: Done reviewing |
|
Fixes for all comments pushed. Ready for re-review. |
|
Also, I added documentation tying together all the prototypal inheritance stuff to our "Brackets Coding Conventions" page--could you review it and see if it looks good to you? https://github.com/adobe/brackets/wiki/Brackets-Coding-Conventions (near the bottom) |
| @@ -748,7 +748,7 @@ define(function (require, exports, module) { | |||
| }, | |||
| false); | |||
| } | |||
| ContextMenu.prototype = new Menu(); | |||
| ContextMenu.prototype = Object.create(Menu.prototype); | |||
There was a problem hiding this comment.
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?
|
@njx: Done re-reviewing |
… assigning a new menu to this.menu, which isn't actually used).
|
Fixes pushed. |
|
Perfect, thanks for the changes. Docs on the wiki look good too. Merging... |
Factor out width-resizing logic for inline widgets into the InlineWidget base class; use a cleaner pattern (and use it more consistently) for inheritance
This addresses #2221 and #2218.
As part of this, cleaned up some issues around how we're doing inheritance:
Object.create()to hook up prototypes(this avoids subtle bugs when binding event handlers in base classes).
(even if the base class impl is empty right now). The one exception is
close(), whichis a bit weird right now.
parentClasspattern for supers--we were callingthis.parentClass.myMethod.call(this), which doesn't work if the current method being called is on anintermediate class in the inheritance chain. Instead, it should be
MySubclass.prototype.parentClass.myMethod.call(this).this.editorproperty in favor of using thethis.hostEditorset up by theInlineWidget.load()method.The issue with
close()is that the base version in InlineWidget seems to assume that there is an_editorHasFocus()method onthis, but InlineWidget doesn't actually define one. Need to figure out if this should be given a default implementation that just returnstrue. If so, then we could just eliminateclose()from InlineImageWidget, and the dummy_editorHasFocus()implementation on InlineColorEditor.