url hinting support #1747
url hinting support #1747
Conversation
ghost
assigned 
|
Reviewing.... |
| @@ -274,6 +402,8 @@ define(function (require, exports, module) { | |||
| if (attrInfo) { | |||
| if (attrInfo.type === "boolean") { | |||
| unfiltered = ["false", "true"]; | |||
| } else if (attrInfo.type === "url") { | |||
| unfiltered = this._getUrlList(query); | |||
There was a problem hiding this comment.
I didn't see you encoding the path for unsafe characters like '(', ')', or space character in _getUrlList. If you're not going to show encoded urls in code hints, then at least you have to encode it before insertion. You also need to decode query before passing it to _getUrlList so that it can match to items in code hint list.
There was a problem hiding this comment.
Fixed. The encoded url is shown in the list and inserted in the page.
There was a problem hiding this comment.
Actually, I prefer the other solution. But I think it is acceptable to show encoded url in the list.
There was a problem hiding this comment.
I tried to implement what you described, but it's too hard to get filtering correct. Part of the reason is that it implies you can, for example, filter by typing a space char when %20 will get inserted in page, which is a conflict . I think it's best to show user encoded url and they learn how they should really look, so I think this is the best solution.
There was a problem hiding this comment.
Not quite if we have to consider double-byte characters and high-ascii characters. Anyway, we can leave it for users who can't stand to read encoded url to log an issue.
|
Done initial review. |
| @@ -285,6 +415,7 @@ define(function (require, exports, module) { | |||
| } | |||
|
|
|||
| if (unfiltered.length) { | |||
| console.assert(!result.length); | |||
| result = $.map(unfiltered, function (item) { | |||
There was a problem hiding this comment.
For folder and file sorting you need to call your custom sort() function here instead of $.map().sort().
There was a problem hiding this comment.
Fixed. I pass a custom sorting function to sort().
|
Changes for code review pushed. |
| // create empty object so we can detect "waiting" state | ||
| self.cachedHints = {}; | ||
| self.cachedHints.unfiltered = []; | ||
| self.cachedHints.waiting = true; |
There was a problem hiding this comment.
I don't see you checking on this boolean cachedHints.waiting. If you don't need it anymore, then you should remove all instances.
There was a problem hiding this comment.
Good catch! Fixed. I ran into a problem where the waiting flag got stuck on so cache was never cleared. Now, we just return the list whether it had items or not.
|
|
||
| // code hints show the same strings that are inserted into text, | ||
| // so strings in list will be encoded. wysiwyg, baby! | ||
| unfiltered.push(encodeURI(entryStr)); |
There was a problem hiding this comment.
encodeURI won't encode '?' and ';' that can be used in filenames. We may have to consider encodeURIComponent or a combination of escape and encodeURI. I can't remember what we did for our default brackets folder path.
There was a problem hiding this comment.
This is just encoding existing file and folder names retrieved from disk. So, there won't be any '?' or ';' chars. This is not user input.
Maybe I do not understand your concern.
There was a problem hiding this comment.
If an existing filename or folder name has these characters, then we won't be encoding them after url insertion.
There was a problem hiding this comment.
This is the same encoding function we use every where in Brackets so I'm not sure why it's an issue for this feature. Since this is just a bonus feature, can we add this issue to the followup user story?
https://trello.com/c/nZx8Z3Ti
There was a problem hiding this comment.
I understand. I'm just trying to help you making this bonus feature better. I will try to find the potential issue with these characters later in Brackets.
|
Done reviewing new changes. |
Added a new code hinting type of "url".
Retrieving directory info is asynchronous, so had to cache results and re-initiate code hints. Also use cached info as long as query is in same folder, so very performant.
Current support:
Not yet supported (https://trello.com/c/nZx8Z3Ti):