PR: More find in files improvements #4599

andfoy · 2017-06-13T05:40:08Z

File line matches are automatically expanded
Lines are cropped only if they have more than 80 characters

…chars or more

ccordoba12 · 2017-06-13T14:26:46Z

Great! Thanks a lot for working on this. I'll look at it in couple of hours.

ccordoba12 · 2017-06-15T20:12:41Z

@andfoy, unfortunately this is not working for minified Javascript files:

Perhaps we could also take 10 chars to left and right, and if their length is smaller than taking 4 words to left and right, then use that instead. What do you think?

andfoy · 2017-06-15T20:42:20Z

@ccordoba12 It sounds great! I'll implement it and test if it works

ccordoba12 · 2017-06-15T20:45:39Z

Let's make it 20 chars to left and right. 10 seems too little. El 15/06/17 a las 15:42, Edgar Andrés Margffoy Tuay escribió:

…

@ccordoba12 <https://github.com/ccordoba12> It sounds great! I'll implement it and test if it works — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4599 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAWS7f-CzL4keuBfGg358ndj_Vs1S9bfks5sEZctgaJpZM4N4Ak5>.

ccordoba12 · 2017-06-18T16:52:04Z

@andfoy, this is looking great! I fixed a minor bug I found, and still have some more comments:

Please truncate results with 30 chars to left and right, instead of 20. 20 is still too little.
Is it possible to append new files and their results at the end of the tree widget? The thing is it's quite distracting to be reading results of one file and then see those results disappear from view because the results of a new file are shown instead. The other solution would be to not sort files by name. In any case, we need to find a solution for this.
There are some terms that are not highlighted on the result line. For example, I looked for the term append in the Spyder source code, and, as you can see below, it's not highlighted in some cases. Please take a look at that too.

Merge!

…_findinfiles Merge!

andfoy · 2017-06-21T00:13:58Z

@ccordoba12 Should I disable sorting feature?

ccordoba12 · 2017-06-21T00:16:19Z

No, the idea would be that after a search has finished, users would be able to sort files, but not while it's running.

andfoy · 2017-06-21T00:20:15Z

I will implement that then!

ccordoba12 · 2017-06-21T01:12:08Z

You left a print statement around, that's why tests are failing.

ccordoba12 · 2017-06-21T02:19:28Z

spyder/widgets/findinfiles.py

@@ -786,6 +797,8 @@ def closing_widget(self):

    def search_complete(self, completed):
        """Current search thread has finished"""
+        self.result_browser.enable_sorting(True)
+        self.result_browser.enable_sorting(True)


This line is repeated.

ccordoba12 · 2017-06-21T02:25:55Z

spyder/widgets/findinfiles.py

@@ -576,6 +612,10 @@ def activated(self, item):
            filename, lineno, colno = itemdata
            self.parent().edit_goto.emit(filename, lineno, self.search_text)

+    def enable_sorting(self, flag):
+        """Enable result sorting after search is complete."""
+        self.sorting_status['status'] = flag


Why this has to be a dictionary instead of a property? I mean, you could simply have

self.sorting = True/False

instead of having a method called enable_sorting that writes to a dict.

But if I modify a property, will it propagate along all elements?

Ok, then let's refactor this a bit so it's easier to understand. What about

self.sorting['status'] = 'on'/'off'

?

andfoy · 2017-06-21T02:30:46Z

spyder/widgets/findinfiles.py

@@ -596,7 +637,8 @@ def append_result(self, results, num_matches):
        filename, lineno, colno, line = results

        if filename not in self.files:
-            item = FileMatchItem(self, filename)
+            item = FileMatchItem(self, filename, self.sorting_status)


I disable sorting only on file entries and not line entries, to accomplish that I use the dict

ccordoba12 · 2017-06-21T05:34:08Z

I tested your latest changes and there are some more bugs:

Results are sorted automatically after a search finishes. That needs to be avoided, i.e. sorting needs to be applied only if the user asks for it.
Results appear twice sometimes
The interface is quite unresponsive while a search is taking place, e.g. scrolling in the Editor is very slow. Can we do something about it?

ccordoba12 · 2017-06-21T05:52:33Z

Forget about point 2. I had the same files in two different directories because I was creating wheels for Spyder before to fix another bug ;-)

andfoy · 2017-06-21T19:08:02Z

It seems that disabling sorting features while a search is taking place helps to unblock the result browser. However, most of the overhead is caused by adding multiple results simultaneously

ccordoba12 · 2017-06-21T19:10:40Z

most of the overhead is caused by adding multiple results simultaneously

Ok, what if we show new results after a file has been scanned completely, instead of showing each new result at a time?

andfoy · 2017-06-21T20:29:58Z

So, should we fallback to our previous search behaviour and compare it with the actual?

ccordoba12 · 2017-06-21T20:37:06Z

So, should we fallback to our previous search behaviour and compare it with the actual?

What previous one exactly? The one in 3.1 or before you did this PR?

andfoy · 2017-06-21T21:56:47Z

The one present on #4056

andfoy · 2017-06-22T00:13:31Z

I think performance dropped by using whole file matches. Should I try to disable automatic expansion of results?

ccordoba12 · 2017-06-22T00:16:19Z

I think performance dropped by using whole file matches

I don't understand what you mean

Should I try to disable automatic expansion of results?

I think rendering results is not the problem. I disabled all rendering to try to find where performance is being affected, and the Editor was still very unresponsive.

So I don't think that's the cause, so it should be in SearchThread instead.

andfoy · 2017-06-22T00:24:02Z

Maybe line cropping should be done on the results viewer instead of being done on the Thread?

ccordoba12 · 2017-06-22T00:25:53Z

Probably, I was also thinking that.

andfoy · 2017-06-22T00:26:18Z

I'll give it a try

andfoy · 2017-06-22T00:44:17Z

It seems that truncating lines outside the Thread improves search performance

andfoy · 2017-06-22T01:00:43Z

@ccordoba12 Does the last commit addresses the two issues?

ccordoba12 · 2017-06-22T13:45:44Z

Absolutely!! There's still a bit of lag but the situation improved considerably! Please do the required cleanup (you left a lot of comments in your last commit), then I'll merge.

I'm really happy with the results! Thanks a lot for putting so much love in our Find in Files.

ccordoba12 · 2017-06-22T23:00:47Z

spyder/widgets/findinfiles.py

    def find_string_in_file(self, fname):
        self.error_flag = False
        self.sig_current_file.emit(fname)
-        # results = {}
+        # results = []


Please also remove this comment.

Fixes #4596

Search items are now expanded automatically, lines are cropped if 80 …

ae540fc

…chars or more

andfoy added this to the v3.2 milestone Jun 13, 2017

andfoy self-assigned this Jun 13, 2017

andfoy and others added 2 commits June 15, 2017 15:51

Line cropping limited to 20 characters from left and right

208c466

Convert lines to unicode before truncating them

0e104e8

Update with 3.x

faff20c

ccordoba12 changed the base branch from master to 3.x June 18, 2017 17:05

andfoy added 5 commits June 20, 2017 18:24

Merge remote-tracking branch 'upstream/3.x' into more_findinfiles

2962742

Merge!

Truncate lines up to 20 characters

8fe921e

Merge branch 'more_findinfiles' of github.com:andfoy/spyder into more…

bc0c7cc

…_findinfiles Merge!

Escaping HTML strings before formatting results

4b5006f

Manual HTML escaping definition

9b0e732

andfoy added 2 commits June 20, 2017 21:14

File results are sorted once the search process has completed

94a8b9c

Duplicate line removed

9d0bc77

ccordoba12 reviewed Jun 21, 2017

View reviewed changes

andfoy commented Jun 21, 2017

View reviewed changes

Minor sorting status dict refactor

ed43cdd

Sorting results only if requested by user

15cd1b5

General code cleanup

c547ac0

ccordoba12 reviewed Jun 22, 2017

View reviewed changes

Removed more left over comments

c69d277

ccordoba12 merged commit d7a6e45 into spyder-ide:3.x Jun 22, 2017

ccordoba12 added a commit that referenced this pull request Jun 22, 2017

Merge from 3.x: PR #4599

d91999c

Fixes #4596

andfoy deleted the more_findinfiles branch June 14, 2019 19:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PR: More find in files improvements #4599

PR: More find in files improvements #4599

andfoy commented Jun 13, 2017

ccordoba12 commented Jun 13, 2017

ccordoba12 commented Jun 15, 2017

andfoy commented Jun 15, 2017

ccordoba12 commented Jun 15, 2017 via email

ccordoba12 commented Jun 18, 2017

andfoy commented Jun 21, 2017

ccordoba12 commented Jun 21, 2017

andfoy commented Jun 21, 2017

ccordoba12 commented Jun 21, 2017 •

edited

Loading

ccordoba12 Jun 21, 2017

ccordoba12 Jun 21, 2017

andfoy Jun 21, 2017

ccordoba12 Jun 21, 2017

andfoy Jun 21, 2017

ccordoba12 commented Jun 21, 2017

ccordoba12 commented Jun 21, 2017

andfoy commented Jun 21, 2017

ccordoba12 commented Jun 21, 2017

andfoy commented Jun 21, 2017

ccordoba12 commented Jun 21, 2017

andfoy commented Jun 21, 2017

andfoy commented Jun 22, 2017

ccordoba12 commented Jun 22, 2017

andfoy commented Jun 22, 2017

ccordoba12 commented Jun 22, 2017

andfoy commented Jun 22, 2017

andfoy commented Jun 22, 2017

andfoy commented Jun 22, 2017

ccordoba12 commented Jun 22, 2017

ccordoba12 Jun 22, 2017

PR: More find in files improvements #4599

PR: More find in files improvements #4599

Conversation

andfoy commented Jun 13, 2017

ccordoba12 commented Jun 13, 2017

ccordoba12 commented Jun 15, 2017

andfoy commented Jun 15, 2017

ccordoba12 commented Jun 15, 2017 via email

ccordoba12 commented Jun 18, 2017

andfoy commented Jun 21, 2017

ccordoba12 commented Jun 21, 2017

andfoy commented Jun 21, 2017

ccordoba12 commented Jun 21, 2017 • edited Loading

ccordoba12 Jun 21, 2017

Choose a reason for hiding this comment

ccordoba12 Jun 21, 2017

Choose a reason for hiding this comment

andfoy Jun 21, 2017

Choose a reason for hiding this comment

ccordoba12 Jun 21, 2017

Choose a reason for hiding this comment

andfoy Jun 21, 2017

Choose a reason for hiding this comment

ccordoba12 commented Jun 21, 2017

ccordoba12 commented Jun 21, 2017

andfoy commented Jun 21, 2017

ccordoba12 commented Jun 21, 2017

andfoy commented Jun 21, 2017

ccordoba12 commented Jun 21, 2017

andfoy commented Jun 21, 2017

andfoy commented Jun 22, 2017

ccordoba12 commented Jun 22, 2017

andfoy commented Jun 22, 2017

ccordoba12 commented Jun 22, 2017

andfoy commented Jun 22, 2017

andfoy commented Jun 22, 2017

andfoy commented Jun 22, 2017

ccordoba12 commented Jun 22, 2017

ccordoba12 Jun 22, 2017

Choose a reason for hiding this comment

ccordoba12 commented Jun 21, 2017 •

edited

Loading