-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Conversation
|
||
start = this._startBookmark.find(); | ||
pos = this._marker.find(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see that this problem existed before with _startBookmark
/_endBookmark
, but _marker
can be undefined
or null
, so that should be checked here to prevent a NPE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it can ever be null
- cm.markText()
always returns a TextMarker
instance, and we initialize it in the constructor and only .clear()
it on close, so when could this ever be null
?
Of course, this._marker.find()
can be null
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tested this - as long as the InlineColorEditor is open, this._marker
is always an instance of TextMarker
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm certain that this._marker
will never be null
in the current code. But the constructor does not enforce that a valid marker is passed in and you can see some unit tests where creating new InlineColorEditor()
does not pass any parameters (i.e. marker
is undefined
). This is just to be safe for any future usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah, now that makes sense. Changed.
Done with review. |
Looks good. Merging. |
this._endBookmark.clear(); | ||
this._endBookmark = this.hostEditor._codeMirror.setBookmark(end); | ||
this._marker.clear(); | ||
this._marker = this.hostEditor._codeMirror.markText(start, end); |
There was a problem hiding this comment.
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 :-)
There was a problem hiding this comment.
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?
Fixes multiple issues with InlineColorEditor:
cm.markText()
instead ofcm.setBookmark()
(implements this comment)