Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

url hinting support #1747

Merged
merged 8 commits into from Oct 5, 2012

Conversation

Projects
None yet
2 participants
Contributor

redmunds commented Oct 2, 2012

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:

  • page relative urls
  • filter on input
  • "../" path support
  • use Tab key for "select and continue hinting"
  • fix sorting for Windows (i.e. folders first, then files)

Not yet supported (https://trello.com/c/nZx8Z3Ti):

  • filter by desired file type based on tag, type attr, etc.
    • e.g. only show .css, .less, etc. files for
  • add list item to popup modal File Finder dialog
  • make code hints window wider
  • site-root relative urls

@ghost ghost assigned RaymondLim Oct 3, 2012

Contributor

RaymondLim commented Oct 4, 2012

Reviewing....

@RaymondLim RaymondLim commented on the diff Oct 4, 2012

src/extensions/default/HTMLCodeHints/main.js
@@ -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);
@RaymondLim

RaymondLim Oct 4, 2012

Contributor

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.

@redmunds

redmunds Oct 5, 2012

Contributor

Fixed. The encoded url is shown in the list and inserted in the page.

@RaymondLim

RaymondLim Oct 5, 2012

Contributor

Actually, I prefer the other solution. But I think it is acceptable to show encoded url in the list.

@redmunds

redmunds Oct 5, 2012

Contributor

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.

@RaymondLim

RaymondLim Oct 5, 2012

Contributor

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.

@RaymondLim RaymondLim and 1 other commented on an outdated diff Oct 4, 2012

src/extensions/default/HTMLCodeHints/main.js
+ // empty list, so no code hints are displayed. In the async callback, the code
+ // hints and the original query are stored in a cache, and then the process to
+ // show code hints is re-initiated.
+ //
+ // During the next pass, there should now be code hints cached from the initial
+ // pass, but user may have typed while file/folder info was being retrieved from
+ // disk, so we need to make sure code hints still apply to current query. If so,
+ // display them, otherwise, clear cache and start over.
+ //
+ // As user types within a folder, the same unfiltered file/folder list is still
+ // valid and re-used from cache. Filtering based on user input is done outside
+ // of this method. When user moves to a new folder, then the cache is deleted,
+ // and file/folder info for new folder is then retrieved.
+
+ if (this.cachedHints && !this.cachedHints.waiting) {
+ // url hints have been cached, so determine if they're are stale
@RaymondLim

RaymondLim Oct 4, 2012

Contributor

Typo in " they're are stale"

@redmunds

redmunds Oct 5, 2012

Contributor

Fixed.

@RaymondLim RaymondLim and 1 other commented on an outdated diff Oct 4, 2012

src/extensions/default/HTMLCodeHints/main.js
+ // if (cachedHints.waiting === true), then we'll return an empty list
+ // and not start another readEntries() operation
+ unfiltered = this.cachedHints.unfiltered;
+ } else {
+ var self = this;
+
+ // create empty object so we can detect "waiting" state
+ self.cachedHints = {};
+ self.cachedHints.unfiltered = [];
+ self.cachedHints.waiting = true;
+
+ NativeFileSystem.requestNativeFileSystem(targetDir, function (dirEntry) {
+ dirEntry.createReader().readEntries(function (entries) {
+ entries.forEach(function (entry) {
+ var entryStr = entry.fullPath.replace(docDir, "");
+ unfiltered.push(entryStr);
@RaymondLim

RaymondLim Oct 4, 2012

Contributor

We should filter out hidden files and folders here. Otherwise, we're showing folders like ".git" in code hints.

@redmunds

redmunds Oct 5, 2012

Contributor

Fixed.

@RaymondLim RaymondLim and 1 other commented on an outdated diff Oct 4, 2012

src/extensions/default/HTMLCodeHints/main.js
+ self.cachedHints.waiting = true;
+
+ NativeFileSystem.requestNativeFileSystem(targetDir, function (dirEntry) {
+ dirEntry.createReader().readEntries(function (entries) {
+ entries.forEach(function (entry) {
+ var entryStr = entry.fullPath.replace(docDir, "");
+ unfiltered.push(entryStr);
+ });
+
+ self.cachedHints.unfiltered = unfiltered;
+ self.cachedHints.query = query;
+ self.cachedHints.queryDir = queryDir;
+ self.cachedHints.waiting = false;
+
+ // re-initiate code hints
+ CodeHintManager.showHint(EditorManager.getFocusedEditor());
@RaymondLim

RaymondLim Oct 4, 2012

Contributor

I'm really concerned that you're calling EditorManager.getFocusedEditor() here with an asynchronous retrieval of data. Can you pass the original editor to this method? I know you're calling this from AttrHints.prototype.search(query) and it does not have the original editor either. I would suggest you update all search apis to have the editor as the second parameter.

@redmunds

redmunds Oct 5, 2012

Contributor

Fixed. Code calls getFocusedEditor() both before and after retrieving data to make sure we're not holding on to a stale editor reference.

@RaymondLim RaymondLim and 1 other commented on an outdated diff Oct 4, 2012

src/extensions/default/HTMLCodeHints/main.js
@@ -247,6 +255,126 @@ define(function (require, exports, module) {
};
/**
+ * Helper function for search(). Create a list of urls to existing files base on the query.
@RaymondLim

RaymondLim Oct 4, 2012

Contributor

Typo. base -> based

@redmunds

redmunds Oct 5, 2012

Contributor

Fixed.

@RaymondLim RaymondLim and 1 other commented on an outdated diff Oct 4, 2012

src/extensions/default/HTMLCodeHints/main.js
+
+ if (this.cachedHints) {
+ // if (cachedHints.waiting === true), then we'll return an empty list
+ // and not start another readEntries() operation
+ unfiltered = this.cachedHints.unfiltered;
+ } else {
+ var self = this;
+
+ // create empty object so we can detect "waiting" state
+ self.cachedHints = {};
+ self.cachedHints.unfiltered = [];
+ self.cachedHints.waiting = true;
+
+ NativeFileSystem.requestNativeFileSystem(targetDir, function (dirEntry) {
+ dirEntry.createReader().readEntries(function (entries) {
+ entries.forEach(function (entry) {
@RaymondLim

RaymondLim Oct 4, 2012

Contributor

For consistency with other existing code, can you replace .forEach calls with $.map?
ie. unfiltered = $.map(entries, function(entry) { ... return entryStr; });

@redmunds

redmunds Oct 5, 2012

Contributor

I disagree with this suggestion. We are supposed to use Array.foreach instead of $.each for better performance, so I suspect the same problem with $.map, which seems to be supported by the data here: http://jsperf.com/javascript-map-vs-jquery-map-vs-jquery-each

I think we may want to convert calls from $.map to Array.map in the places where the operation is on an Array (not a jQuery result set). If so, then it might make sense to convert my code to Array.map.

@RaymondLim

RaymondLim Oct 5, 2012

Contributor

Nice performance data!

I agree with you. We can make the decision later on.

Contributor

RaymondLim commented Oct 4, 2012

Done initial review.

@RaymondLim RaymondLim commented on the diff Oct 4, 2012

src/extensions/default/HTMLCodeHints/main.js
@@ -285,6 +415,7 @@ define(function (require, exports, module) {
}
if (unfiltered.length) {
+ console.assert(!result.length);
result = $.map(unfiltered, function (item) {
@RaymondLim

RaymondLim Oct 4, 2012

Contributor

For folder and file sorting you need to call your custom sort() function here instead of $.map().sort().

@redmunds

redmunds Oct 5, 2012

Contributor

Fixed. I pass a custom sorting function to sort().

@RaymondLim RaymondLim commented on the diff Oct 4, 2012

src/extensions/default/HTMLCodeHints/main.js
+ doc = DocumentManager.getCurrentDocument();
+ if (!doc || !doc.file) {
+ return result;
+ }
+
+ var docUrl = window.PathUtils.parseUrl(doc.file.fullPath);
+ if (!docUrl) {
+ return result;
+ }
+
+ var docDir = docUrl.domain + docUrl.directory;
+
+ // get relative path from query string
+ // TODO: handle site-root relative
+ var queryDir = "";
+ var queryUrl = window.PathUtils.parseUrl(query.queryStr);
@RaymondLim

RaymondLim Oct 4, 2012

Contributor

Since site relative path is not supported yet, you need to check whether query.Str starts with "/". If it is, then just return here so that we're not showing incorrect code hints.

@redmunds

redmunds Oct 5, 2012

Contributor

Good catch! Fixed.

Contributor

redmunds commented Oct 5, 2012

Changes for code review pushed.

@RaymondLim RaymondLim and 1 other commented on an outdated diff Oct 5, 2012

src/extensions/default/HTMLCodeHints/main.js
+ }
+ }
+
+ if (this.cachedHints) {
+ // if (cachedHints.waiting === true), then we'll return an empty list
+ // and not start another readEntries() operation
+ unfiltered = this.cachedHints.unfiltered;
+
+ } else {
+ var self = this,
+ origEditor = EditorManager.getFocusedEditor();
+
+ // create empty object so we can detect "waiting" state
+ self.cachedHints = {};
+ self.cachedHints.unfiltered = [];
+ self.cachedHints.waiting = true;
@RaymondLim

RaymondLim Oct 5, 2012

Contributor

I don't see you checking on this boolean cachedHints.waiting. If you don't need it anymore, then you should remove all instances.

@redmunds

redmunds Oct 5, 2012

Contributor

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.

@RaymondLim RaymondLim commented on the diff Oct 5, 2012

src/extensions/default/HTMLCodeHints/main.js
+
+ // create empty object so we can detect "waiting" state
+ self.cachedHints = {};
+ self.cachedHints.unfiltered = [];
+
+ NativeFileSystem.requestNativeFileSystem(targetDir, function (dirEntry) {
+ dirEntry.createReader().readEntries(function (entries) {
+
+ entries.forEach(function (entry) {
+ if (ProjectManager.shouldShow(entry)) {
+ // convert to doc relative path
+ var entryStr = entry.fullPath.replace(docDir, "");
+
+ // code hints show the same strings that are inserted into text,
+ // so strings in list will be encoded. wysiwyg, baby!
+ unfiltered.push(encodeURI(entryStr));
@RaymondLim

RaymondLim Oct 5, 2012

Contributor

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.

@redmunds

redmunds Oct 5, 2012

Contributor

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.

@RaymondLim

RaymondLim Oct 5, 2012

Contributor

If an existing filename or folder name has these characters, then we won't be encoding them after url insertion.

@redmunds

redmunds Oct 5, 2012

Contributor

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

@RaymondLim

RaymondLim Oct 5, 2012

Contributor

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.

@RaymondLim RaymondLim commented on the diff Oct 5, 2012

src/extensions/default/HTMLCodeHints/main.js
+ if (!docUrl) {
+ return result;
+ }
+
+ var docDir = docUrl.domain + docUrl.directory;
+
+ // get relative path from query string
+ // TODO: handle site-root relative
+ var queryDir = "";
+ var queryUrl = window.PathUtils.parseUrl(query.queryStr);
+ if (queryUrl) {
+ queryDir = queryUrl.directory;
+ }
+
+ // build target folder path
+ var targetDir = docDir + decodeURI(queryDir);
@RaymondLim

RaymondLim Oct 5, 2012

Contributor

Why do you need decodeURI here if you're going to show encoded urls in the list?

@redmunds

redmunds Oct 5, 2012

Contributor

targetDir is the folder path passed to NativeFileSystem.requestNativeFileSystem() to retrieve the contents of that folder. It is not seen by user. The list that it returns is the list used for hints, which is encoded.

@RaymondLim

RaymondLim Oct 5, 2012

Contributor

I see.

Contributor

RaymondLim commented Oct 5, 2012

Done reviewing new changes.

RaymondLim added a commit that referenced this pull request Oct 5, 2012

@RaymondLim RaymondLim merged commit a856faa into master Oct 5, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment