For #6093: Added button to show all results in a list

[marcelgerber]

This PR adds a button to show all results in a list, as requested in #6093.
Including:

• Make the find-counter clickable
• New string (title attribute of find-counter)

@peterflynn

[TomMalbran]

Nice feature. The new button might need to be put in another place if something like this is done in the Find/Replace UI Cleanup card, and I might also just call it "Find All". But @larz0 would have a better opinion on that.

To Show the status bar, try calling modalBar.close(true, animate); before calling the command.

I am not sure why the busy indicator is not hiding, since it should hide it after the search is done.

[marcelgerber]

@peterflynn @larz0 Do we still need this after the Find/Replace overhaul?

[marcelgerber]

@peterflynn @larz0 I'm asking again, do we still need this?
I'd like to fit it into the new Find/Replace bar.

[TomMalbran]

I wouldn't add this to Find/Replace. I think this could go in Find in Files, by changing the in project text to be a select, where you select the scope, and File can be one of the. I am planning on try this eventually.

[marcelgerber]

But it seems illogical to do Find in Files in order to search just one file.

[redmunds]

@TomMalbran You can already do Find In Files in a single file by right-clicking on file in project tree and using Find in....

@SAplayer sometimes it helps to see the list of hits for a single file in the bottom panel.

[TomMalbran]

@redmunds I know, but you can't do a search in the Working Set yet, and sometimes is easier to press ctrl+shift+f and then make it search on the currently selected folder/file.

[larz0]

Just make the find-result at the end of the find input a blue "link" that opens up the bottom panel. This will reduce clutter and it also gives context. Would be nice to be able to tab to it as well.

[redmunds]

I'd like an option to "Find in Working Set Files"

[marcelgerber]

@redmunds Wonderful idea!

@larz0 So should I carry on with this?

[TomMalbran]

I had a Find in Working Set Files command implemented in #3151. So I should get to it in one of the next Find in Files Improvements iteration. But instead of just a command, I thought it might be nice to be able to change the scope inside the find in files bar too.

[marcelgerber]

Yes, that's a good idea and the way Visual Studio (2013) does it:

(current block, selection, current doc, all opened docs (similar to Working Files), current project, all projects)

[larz0]

@SAplayer I think you should go ahead with that. For all the other drop down items we could put them on top of search history drop down items if we ever get them.

[marcelgerber]

Just pushed a commit (sorry, the old ones got lost) to make this PR up-to-date.
The only thing missing is an icon for the button.

[marcelgerber]

Icon missing - I'd take https://github.com/topcoat/icons/blob/master/svg/arrow_down.svg

[TomMalbran]

If this will go only in Find, maybe it could be a text button? since there is lots of space. If we want it in Replace too, it will need an icon.

[marcelgerber]

I don't think it makes sense to show this button in Replace, we already have the Replace All button which does basically the same.
In case we want a text, we should put it on the right side, just like the Replace All button is placed on the right.

[marcelgerber]

Another not-so-cool thing: The same search is done twice. Would be better to just pass the results to fill the list.
@TomMalbran Looks like your #7059 (I don't really understand it, sorry) is introducing a new way to fill a search results panel, so it might be the best to wait for that to land in master.

[larz0]

@SAplayer this was what I meant (see screenshot), we don't need an extra button:

[TomMalbran]

If it will be just a link on the search numbers, I would add it to replace too.

@SAplayer I din't think it is a problem to do a double search. Replace All already search again to get the necessary information. If the highlight info had the necessary information and was stored to be later used, maybe we could use #7059 with some modifications to create the Find All Results class, but seems like a small optimization to be worth it, at least for now.

BTW, I think we should also increase the maximum number of results from a single file, which is hardcoded at 300.

[larz0]

Yes. All search numbers should be clickable.

@redmunds here's the idea for Find in Working Files that I mentioned previously. I've included find history.

[TomMalbran]

@larz0 For find in Working Set, besides adding a new commands, I was thinking of replacing the in project text when doing a Find in Files with a dropdown button, to change between the possible options. But that would only go in Find in Files.

[larz0]

@TomMalbran should we just use the Find/Replace UI for Find/Replace in Files?

[marcelgerber]

Next commit online.
I hope this one is right as I'm gonna be on vacation until Sunday. Feel free to do changes on your own.

[TomMalbran]

@larz0 Do you mean using one command for all 3? It pops up the find/replace modalbar which does the same as now, but you could also use it for find/replace in files? If yes, it might be hard to place everything, including the exceptions and more options, unless we end up using 2 lines.

If not, the bars would look similar, but not exactly the same. Since the modalbar closes after doing the search, the arrows are not required, and the replace buttons would be just one, which means that there might be extra space for the scope and filters dropdowns.

[larz0]

Sounds good. Not one command for all three but just the same Replace UI with the filter instead of the arrows.

[TomMalbran]

I am not sure, might look weird to have it in the history popdown. There is also the Find In... that shows the scope next to the search input. Maybe we could place this in the space used to show the results count inside the input?

[larz0]

Hmm I'll need to think about that a bit more. It could be hard to scale.

[marcelgerber]

@TomMalbran The Find in... can get quite long (imagine src/node_modules/grunt/...) so it's not a good idea to do that.
But here are some impressions from Visual Studio again:
Find

Replace

Yes, they got a simple, but functional Find/Replace UI.

[TomMalbran]

I am not sure why we need a return for this function since is not used. But if we need it, you should add a @return in the JSDocs.

[marcelgerber]

Pushed changes.

[njx]

@larz0 - did you add the "needs review" tag to this? You don't need to do that for pull requests - we always look at unassigned pull requests in the standup anyway. Just for bugs.

[larz0]

@njx yeah that was me. Now I know :)

[njx]

Hey - was looking at this while combing through PRs that might be affected by my intended refactoring of FindReplace/FindInFiles. Without going into whether the UI approach in this PR is a good idea or not (I haven't had time to form an opinion :)), looking at the code it seems like the basic approach would still work even after the refactoring, though there will probably be enough code change that it might be easier to just re-fix it afterwards.

Conversely, depending on how far I go with the refactoring to separate out and unify the various "results" UIs we have (for Replace All and Find in Files), it might be possible to actually implement this directly from the single-file Find code, by just letting it pass a set of results to the results UI. Not sure if we'll get that far though.

[njx]

BTW, one other thing to think about...We also have as a backlog item (and someone requested recently) the idea of a "Find All" button in the single-file Find bar that multiply-selects all the results, like what Alt-F3/Cmd-Ctrl-G does today. That's obviously a different feature from this one (and I think they're both useful), but it would be good to think about how we would want to include that in the UI as well.

[pthiess]

@peterflynn marking needs review, you and @njx are better suited to make a call on this.

[njx]

Hey - getting back to look at this; thanks for continuing to work it out. A few thoughts:

• As mentioned above, this will need to be modified once the nj/replace-on-disk branch lands, but this should be trivial: instead of executing the Find in Subtree command, you can call FindInFilesUI.searchAndShowResults() directly, passing the queryInfo and the scope (and your changes to FindInFiles shouldn't be necessary). You can see how this is called from FindReplace.doReplace() in that branch to implement Replace All - your case should be the same, just with different arguments.
• On the UI side, I don't think clicking on the results number is that discoverable. What if we made it look like a link (with an underline)?
• Also, we need to think about how we'll eventually provide the behavior of selecting all the results (as a multiple selection) instead of opening the results list. Perhaps the link could pop up a dropdown that lets you choose between the two.

@larz0 want to weigh in on the last two questions?

[njx]

@larz0 ping - see my last comment above

[larz0]

@njx I like both of those ideas!

[marcelgerber]

@njx I'll just wait until your branch lands in master.
Can you think of a better method without searching twice?

[njx]

I wouldn't worry about redoing the search - it should be fast enough and it doesn't seem worth trying to do the work to bridge the single-file Find results over into the Find in Files SearchModel stuff. We could optimize that later if we want.

[njx]

FYI, the replace-in-files branch has landed in master. I'd suggest waiting to do more work until after we branch for the current release (probably within the next few days) in case there are major bug fixes we need to make in the new code, although I would hope that's not too likely :)

[dangoor]

Marking as triage complete, given the state of discussion here.

[njx]

Yup, would love to see this move forward. Unassigning myself so this can go in the general PR bucket per our new process.

[marcelgerber]

@njx Unfortunately, I probably won't get into this before Monday 21.

[njx]

@SAplayer No hurry. Thanks!

[marcelgerber]

I'm going to close this now. It's pretty outdated and I don't think it's the best user experience ever.
Feel free to pick this up and to submit a PR.