Issue 5237: Show "no results" for Find in Files #5371

cosmosgenius · 2013-09-28T15:37:50Z

This is for #5237.

Removed the promise from dialog and create a function doSearch() which is called from the keydown event handler.

Moved "dialog" object to module scope so that function _showSearchResults can directly handle it.

Adding a css class "no-results" to the input box when no results are present.

redmunds · 2013-09-30T21:26:40Z

@cosmosgenius I made a first pass through your code and it looks pretty good!

My first question is actually for @larz0 -- I like how this code now uses the red border around search field to indicate no results, but is that enough? Seems like it should also display "no results".

redmunds · 2013-09-30T21:29:01Z

src/search/FindInFiles.js

            _hideSearchResults();
+
+            if(dialog instanceof FindInFilesDialog){
+                dialog.getDialogTextField().addClass('no-results');


After search with no results, the red border (i.e. 'no-results' class) should go away as soon as I start typing as new search string.

larz0 · 2013-09-30T21:34:10Z

@redmunds @cosmosgenius

Ok let's do this:

When the user starts typing a new search switch it back to:

redmunds · 2013-09-30T21:37:50Z

src/search/FindInFiles.js

+            if (!currentQueryExpr) {
+                return;
+            }
+            StatusBar.showBusyIndicator(true);


Seems to be a long time before the busy indicator is shown. It looks like this was not introduced with this code, but it would be great if you could take a look at it. If you can't figure it out, then we can open a separate issue.

would it be good if i move it inside the keydown handler for the dialog

You could try it. I'm wondering if it's more of a painting problem.

redmunds · 2013-09-30T21:46:48Z

Dialog allows typing during search phase. The text is not updated until search is complete so it's hard to see, but I can see edits briefly when dialog is being dismissed. Text field should be disabled when search is in progress.

redmunds · 2013-09-30T21:49:00Z

src/search/FindInFiles.js

-
+
+            if(dialog instanceof FindInFilesDialog){
+                dialog._close();


It's safe to set dialog = null; after dialog is closed, right? If so, it should be done to free memory sooner.

redmunds · 2013-09-30T21:53:50Z

Done with initial code review. Thanks for taking on this change. It's a bigger change than it first seemed, but this will make searching much better.

…closing it

redmunds · 2013-10-03T15:20:52Z

I apologize for the late comments on this pull request, but I had a crazy idea about this.

For me, another annoying thing about Find in Files is that sometimes I search for something in the code, do whatever, think I'm done and close the results panel. Later (sometimes only a few milliseconds), I realize there's something else I wanted to do. Unfortunately, the data is now lost -- which is a shame since the results panel data is now updated as you edit your code. So, I'd like a way to re-open the FiF results to see the previous search.

What does that have to do with this pull request? I thought of this following workflow that may change what's been done so far in this PR:

When you invoke Find in Files, the dialog is opened at the top, but the results panel is also opened.
The results panel displays nothing if no search has been done yet, or previous results (which could be "no results") if a search has already been done.
During a search, the dialog is disabled and the results panel displays "searching...".
When search completes, results panel shows either a list of results (as it currently does) or a message such as "no results" if the search argument is not found. Seems like the dialog at top should always stay open. If so, keyboard shortcut could be used to toggle dialog/panel open/closed.

Maybe need a way to see results panel without dialog open at top?

@larz0 @cosmosgenius @TomMalbran Any thoughts on this?

larz0 · 2013-10-03T16:10:24Z

@redmunds I remember @RaymondLim mentioned something about tabbed search panel with previous search results. Maybe we could wait for that? We can open the panel via a view search history icon in the modal bar or shortcut.

larz0 · 2013-10-03T16:12:15Z

The tab label could show just the searched text up to a number of characters and the full info could go below the tab.

redmunds · 2013-10-03T16:24:19Z

@larz0 I forgot about the tabbed interface (that I'm sure we'll have at some point). I think that solves the "want to see previous results" case because there less need to close the panel when they're sharing space. We might also want to preserve the previous results when the panel is closed, but I think that can wait until we have a tabbed panel.

In anticipation of a ~~closed~~ tabbed panel, should "no results" be displayed in the panel instead of the dialog? Also, how does that effect whether dialog remains open (or not)? I'm just trying to avoid doing work that may be made obsolete soon.

larz0 · 2013-10-03T16:35:09Z

If we want to show the panel on search we should move the search input on the panel so it feels like they're connected and stop the eyes from pogo-sticking.

…r message display for no result

cosmosgenius · 2013-10-03T19:46:09Z

Seems like a lot of design changes. From the discussion it seems now all the responses to the user will be done on the search panel instead of the dialog, which would be in the disabled state when the search is going on.

And now rather than destroying the previous results we would store it and show it the next time FiF is opened.
I am confused on the tabbed interface part though.
Also on what @larz0 commented, moving the search input to the panel means complete removal of the dialog right?

njx · 2013-10-03T19:55:16Z

Definitely seems like a bit of scope creep :) I haven't followed this PR, but could we accept this as is and add the other idea to the backlog?

redmunds · 2013-10-03T20:01:35Z

@cosmosgenius Sorry. I mentioned it because I thought it could actually simplify this, but I only made it more confusing. Let's continue with the original plan.

cosmosgenius · 2013-10-03T20:07:58Z

i liked the part about keep the previous results and showing them when next time FiF is opened. I'll try implementing it and see how it goes. I'll update on this.

redmunds · 2013-10-03T20:12:01Z

@cosmosgenius This PR pretty close to being done. Let's finish this PR, and then submit that as a new PR.

…e code clean up

…g of text field when search is going on

cosmosgenius · 2013-10-03T20:53:20Z

How would i cancel an already initiated search?? I guess it wont be required as currently text field is disabled when search is in progress, but would like to know on how we could achieve the same.

Other than this i am done.

redmunds · 2013-10-03T23:12:42Z

Currently, there is no mechanism for cancelling a search, but it would be easy to add. The _inScope() function could check a flag and return false for all remaining files.

redmunds · 2013-10-04T00:28:35Z

This looks pretty good. We're finishing off Sprint 32, so I won't be able to merge this for a few days.

TomMalbran · 2013-10-04T01:17:12Z

src/search/FindInFiles.js

+
+            if (dialog) {
+                dialog.getDialogTextField().addClass('no-results')
+                                            .removeAttr('disabled')


Use double quotes on these 2 lines.

redmunds · 2013-10-10T00:52:55Z

I'm still seeing a JSLint error: 234 '_doSearch' was used before it was defined.

redmunds · 2013-10-10T00:55:29Z

This branch seems to have introduced a problem when you initiate a Find in Files with an empty search argument. The wait cursor and disabled modalbar never go away.

redmunds · 2013-10-10T01:00:37Z

Done with second code review. This is looking good. Just 1 bug and a little cleanup.

TomMalbran · 2013-10-10T01:03:40Z

src/search/FindInFiles.js

+                        that._close(null);
+                    } else if (event.keyCode === KeyEvent.DOM_VK_RETURN) {
+                        StatusBar.showBusyIndicator(true);
+                        that.getDialogTextField().attr('disabled', 'disabled');


Use double quotes here.
Same for lines 242 and 245.

cosmosgenius · 2013-10-10T18:30:26Z

I moved the query null check to _getQueryRegExp. Seem when the reg ex is invalid the same getting stuck situation is happening.

cosmosgenius · 2013-10-10T18:35:41Z

If we press "enter key" for three to four times for a "noresult search" it doesn't respond to "enter or esc" keys anymore. Only after some interaction it works (like any other key or mouse click on input box). I couldn't figure out why it is happening. It seems something related to brackets-core.

redmunds · 2013-10-10T21:06:55Z

If we press "enter key" for three to four times for a "noresult search" it doesn't respond to "enter or esc" keys anymore.

Where are you doing that?

redmunds · 2013-10-11T15:36:41Z

Added Trello Card to Icebox for side discussion: https://trello.com/c/KY4eJL7V/1038-find-in-files-preserve-previous-results

This was squashed into #5477. Closing this one.

Fix for issue 5237.

5ef1f7d

ghost assigned redmunds Sep 30, 2013

redmunds reviewed Sep 30, 2013
View reviewed changes

cosmosgenius added 3 commits October 2, 2013 16:55

remove commentted line and implemented JSLint suggestions

f632c85

added return if query is not available in doSearch function

eaab4ed

removed instanceof check for dialog and deleting dialog object after …

e7cec87

…closing it

removing no-results class as soon as user start typing and added Erro…

8f6e1ef

…r message display for no result

cosmosgenius added 2 commits October 4, 2013 01:51

Added function comments. Moved doSearch as a private function and som…

c922aed

…e code clean up

moved busy indicator loading to keydown handler. Implemented disablin…

2d66078

…g of text field when search is going on

TomMalbran reviewed Oct 4, 2013
View reviewed changes

jslint suggestions and removed if check for dialog

0588c82

TomMalbran reviewed Oct 10, 2013
View reviewed changes

cosmosgenius added 4 commits October 10, 2013 22:48

single quotes to double

f083d87

Fix for when empty search is initiated and gets stuck

140f843

Reordered functions so that functions are defined before they are called

3f0ef41

Moved query null check to _getQueryRegExp

9f4bd4d

Merge branch 'master' into findinfiles-noresult

60e89f4

redmunds mentioned this pull request Oct 11, 2013

Fix for issue #5237 - Find in Files - No response for no result found #5477

Merged

redmunds closed this Oct 11, 2013

njx mentioned this pull request Oct 28, 2013

Needs two tries to rerun Find in Files after search with zero results #5699

Closed

This was referenced Aug 30, 2021

[CLOSED] Issue 5237: Show "no results" for Find in Files brackets-archive/bracketsIssues#4940

Open

[CLOSED] Fix for issue #5237 - Find in Files - No response for no result found brackets-archive/bracketsIssues#5029

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 5237: Show "no results" for Find in Files #5371

Issue 5237: Show "no results" for Find in Files #5371

cosmosgenius commented Sep 28, 2013

redmunds commented Sep 30, 2013

redmunds Sep 30, 2013

larz0 commented Sep 30, 2013

redmunds Sep 30, 2013

cosmosgenius Oct 3, 2013

redmunds Oct 3, 2013

redmunds commented Sep 30, 2013

redmunds Sep 30, 2013

redmunds commented Sep 30, 2013

redmunds commented Oct 3, 2013

larz0 commented Oct 3, 2013

larz0 commented Oct 3, 2013

redmunds commented Oct 3, 2013

larz0 commented Oct 3, 2013

cosmosgenius commented Oct 3, 2013

njx commented Oct 3, 2013

redmunds commented Oct 3, 2013

cosmosgenius commented Oct 3, 2013

redmunds commented Oct 3, 2013

cosmosgenius commented Oct 3, 2013

redmunds commented Oct 3, 2013

redmunds commented Oct 4, 2013

TomMalbran Oct 4, 2013

redmunds commented Oct 10, 2013

redmunds commented Oct 10, 2013

redmunds commented Oct 10, 2013

TomMalbran Oct 10, 2013

cosmosgenius commented Oct 10, 2013

cosmosgenius commented Oct 10, 2013

redmunds commented Oct 10, 2013

redmunds commented Oct 11, 2013

Issue 5237: Show "no results" for Find in Files #5371

Issue 5237: Show "no results" for Find in Files #5371

Conversation

cosmosgenius commented Sep 28, 2013

redmunds commented Sep 30, 2013

redmunds Sep 30, 2013

Choose a reason for hiding this comment

larz0 commented Sep 30, 2013

redmunds Sep 30, 2013

Choose a reason for hiding this comment

cosmosgenius Oct 3, 2013

Choose a reason for hiding this comment

redmunds Oct 3, 2013

Choose a reason for hiding this comment

redmunds commented Sep 30, 2013

redmunds Sep 30, 2013

Choose a reason for hiding this comment

redmunds commented Sep 30, 2013

redmunds commented Oct 3, 2013

larz0 commented Oct 3, 2013

larz0 commented Oct 3, 2013

redmunds commented Oct 3, 2013

larz0 commented Oct 3, 2013

cosmosgenius commented Oct 3, 2013

njx commented Oct 3, 2013

redmunds commented Oct 3, 2013

cosmosgenius commented Oct 3, 2013

redmunds commented Oct 3, 2013

cosmosgenius commented Oct 3, 2013

redmunds commented Oct 3, 2013

redmunds commented Oct 4, 2013

TomMalbran Oct 4, 2013

Choose a reason for hiding this comment

redmunds commented Oct 10, 2013

redmunds commented Oct 10, 2013

redmunds commented Oct 10, 2013

TomMalbran Oct 10, 2013

Choose a reason for hiding this comment

cosmosgenius commented Oct 10, 2013

cosmosgenius commented Oct 10, 2013

redmunds commented Oct 10, 2013

redmunds commented Oct 11, 2013