Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Find in subset of all files #2084

Merged
merged 5 commits into from
Nov 14, 2012
Merged

Find in subset of all files #2084

merged 5 commits into from
Nov 14, 2012

Conversation

peterflynn
Copy link
Member

Adds a "Find in..." item to the project tree context menu to search within a specific file or subtree. Tweaks 'Find in Files' search bar & results panel UI to indicate the scope.

Also fixes two minor escaping bugs in the results panel.

…scoped

to the specific project tree node that was right-clicked.

Find in Files search bar & results list indicates the scope.

Still unresolved: what to do when right-click a file instead of a folder.
dir. Avoids current context menu UI limitations, and also provides more
functionality.

Also fix two escaping bugs: use our standard escaping util; escape file
names in search result headings.
…s-scope

* origin/master: (206 commits)
  Update 'de' locale
  Add cleanup bug number to TODO. Improve docs on Resizer.
  update recent projects extension. remove reference to settings button.
  encode the URLs
  Fix Menu.removeMenuItem() unit tests: update for new setup where all tests in spec share one window (use unique ids for everything). Code cleanup in removeMenuItem(): use existing utility method to compute menu item id. Plus some docs tweaks.
  also check status 0
  Fix bug with Select Line around end of inline editor range, and add unit tests for it.
  Remove unused argument to highlightMatch()
  Fix edge case with string range reporting from stringMatch()
  trap status from XMLHttpRequest and fail accordingly
  Unit tests for new Select Line command (pull #2002)
  pass return value
  Comment formatting tweak per code review
  Fix Quick Open so discontiguous matches are highlighted in modes other than file search: - Hoist out generic highlightMatch() utility to generate bolded strings - Use it in both defaultResultsFormatter() and _filenameResultsFormatter() Also: - Clarify stringMatch() docs a bit - Fix an older glitch in basicMatchSort(): the sort field precedence list had a gap, leading to a no-op comparison step
  fix margin/padding in recent project drop down
  add Project Settings... item to file menu
  code review changes
  More focus bug fixes
  remove commented out code
  Tweaks to docs in various files: clarifications & added/fixed type annotations. Fix lots of JSLint errors in CSSUtils-test. Remove unused dependencies in WorkingSetView.
  ...

Conflicts:
	src/command/Menus.js
@peterflynn
Copy link
Member Author

@gruehle, you interested in reviewing?

@ghost ghost assigned gruehle Nov 9, 2012
": <input type='text' id='findInFilesInput' style='width: 10em'> <span style='color: #888'>(" +
Strings.SEARCH_REGEXP_INFO + ")</span>";
": <input type='text' id='findInFilesInput' style='width: 10em'> <span id='findInFilesScope'></span> " +
"<span style='color: #888'>(" + Strings.SEARCH_REGEXP_INFO + ")</span>";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a suggestion for simplifying this dialog a bit. As implemented here, when searching all files, the dialog looks like:

Find in Files: [__________] in all of brackets (use /re/ syntax for regexp search)

When doing a scoped search, the dialog looks like:

Find in Files: [__________] in brackets/src (use /re/ syntax for regexp search)

As an alternative, how about adding the scope to the prompt label instead. Global searches would look like:

Find in project: [___________] (use /re/ syntax for regexp search)

Scoped searches would look like:

Find in src/brackets: [_________] (use /re/ syntax for regexp search)

Bolding the scope folder/file will help draw attention to the fact that this is a scoped search.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern with putting the folder name on the left is it would make the horizontal position of the text field vary quite a bit, perhaps unpredictably so.

Your other proposed changes would still work if we left the label on the right, though...

My only other hesitation is that we've avoided the term "project" up until recently (pretty sure that's why I didn't use it when I originally wrote this). But we started using it fairly prominently in the UI last sprint, so I think that change would be ok now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was hesitant to use "project" at first, but we are using it in the UI, and it does sound better (and is more accurate) than "all files". The problem with using the project name here is we will eventually want to search the working set, too, and that may contain files that aren't in the main project directory.

My concern with putting the folder name on the left is it would make the horizontal position of the text field vary quite a bit, perhaps unpredictably so.

Good point. It wouldn't be a problem for shallow paths, but could be a problem with long document names and/or paths. How about using just "Find:" on the left and adding the path (or "project") on the right.

@gruehle
Copy link
Member

gruehle commented Nov 12, 2012

Done with initial review. This is awesome! I've been wanting this feature for a long time.

I just had one minor suggestion on improving the UI. The code looks great.

… right.

Bold the filename to call out the scope more strongly. Use the generic word
"project" instead of the full project name.
@peterflynn
Copy link
Member Author

@gruehle: changes pushed.

It's a little gross having to use html() instead of text() to accommodate the formatting, since it makes it unpredictable which strings need to be escaped vs. unescaped in strings.js. But it's not the first time we've done that, and to be honest I haven't seen any great solutions to localization that contains embedded formatting -- since the word order can change, you can't just define a static template of formatted spans and inject the strings into it separately...

@@ -107,7 +107,9 @@ define({
"NO_UPDATE_TITLE" : "You're up to date!",
"NO_UPDATE_MESSAGE" : "You are running the latest version of {APP_NAME}.",

"FIND_IN_FILES_TITLE" : "for \"{4}\" - {0} {1} in {2} {3}",
"FIND_IN_FILES_TITLE" : "for \"{4}\" {5} - {0} {1} in {2} {3}",
"FIND_IN_FILES_SCOPED" : "in <b>{0}</b>",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use <span class='dialog-filename'> instead of <b> for consistency with other dialog strings displaying filenames.

Use CSS class instead of raw <b> tags where filenames are displayed.
@peterflynn
Copy link
Member Author

Fixes pushed. I also fixed one other usage of <b> in an existing string.

@gruehle
Copy link
Member

gruehle commented Nov 14, 2012

Looks great! Merging.

gruehle added a commit that referenced this pull request Nov 14, 2012
@gruehle gruehle merged commit 92732a7 into master Nov 14, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants