Fixing multiple issues in CSS code hints extension (in getHints and insertHint APIs) #2686
Conversation
|
Should this situation: |
|
@kjgorman Thanks for reviewing and posting question! Our code hints implementation considers the cursor position before the first and after the last as part of the current string (ie. inclusive). You can see this behavior in the way Tag hints and attribute hints works in Brackets. So we're replacing here for CSS hints as well. Otherwise, we will have to auto append a whitespace, which I think is a bad design. |
|
Ah I see, what I was looking at was |
ghost
assigned 
| keepHints = true; | ||
| end.ch = start.ch; | ||
| } else { | ||
| // It's a replacement of an existing one or just typed in properety. |
There was a problem hiding this comment.
nit: typo: should be "property"
|
The changes look good overall, and this was a good chance for me to dig in and learn a bit about this part of the code. It would be nice to also see unit tests for the changes to the behavior here. |
…xisting property name.
|
Changes for review feedback are pushed. Ready for re-review. |
| if (TokenUtils.moveSkippingWhitespace(TokenUtils.moveNextToken, ctx) && ctx.token.string === ":") { | ||
| adjustCursor = true; | ||
| newCursor = cursor; | ||
| newCursor.ch = cursor.ch + (hint.length - this.info.name.length); |
There was a problem hiding this comment.
This isn't copying cursor into newCursor (after this line of code, newCursor.ch === cursor.ch) and I'd be worried about some bit of code being added later on below that uses cursor after it has been modified.
You could eliminate newCursor and make it obvious that you are modifying cursor here, or you could make newCursor a true copy of cursor (newCursor = {line: cursor.line, ch: cursor.ch};)
|
Sorry if you got some email churn... I put my comments in the wrong place (on the commit rather than the changed files...) The new behavior looks good! |
|
Ready for another review. |
|
Looks good to me. Merging. |
Fixing multiple issues in CSS code hints extension (in getHints and insertHint APIs)
This fixes issue #2626, #2628 and #2682.