PR: Fix 3 bugs and add 4 improvements to Find in Files #6095

jnsebgosselin · 2017-12-30T06:08:18Z

Fix issue reported in Paths in the "Search in" combobox of the "Find in File" plugin becomes corrupted #5306 (comment)
Fix issue reported in Paths in the "Search in" combobox of the "Find in File" plugin becomes corrupted #5306 (comment)
Fix issue reported in Paths in the "Search in" combobox of the "Find in File" plugin becomes corrupted #5306 (comment)
When populating the Search in QComboBox, do not add the paths that do not exists on the file system.
When hovering over an item of the QComboBox and pressing Del, remove the item from the list.
Add a Clear this list option below Select other directory.
Show the full path when hovering over an item in the Search in QComboBox.
Expand the tests for the FindInFiles Widget.
Expand the tests for the FindInFiles Plugin.

- All the code related to the management of the search path for the FindInFiles widget was isolated in a new class. - The inheritance to the BaseComboBox was dropped since it was causing problems. - Dropped the custom model that was used for the combobox. Everything is managed with the default model of the QComboBox class. This fixed another issue with the previous implementation of the combo-box.

When the combobox popup list is visible and the delete key is pressed, the highlighted external path is removed from the list.

jnsebgosselin · 2017-12-30T19:23:32Z

Mouse Over and Delete key press demo:

Select other directory and Clear this list demo:

ccordoba12 · 2017-12-30T21:25:27Z

Jean, could you also fix codecov to show failures only when coverage decreases in 1%? Thanks!

pep8speaks · 2017-12-30T23:43:00Z

Hello @jnsebgosselin! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on January 09, 2018 at 05:20 Hours UTC

jnsebgosselin · 2017-12-31T03:08:28Z

@ccordoba12 While working on the tests for the FindInFiles plugin, I found out an additional bug.

The get_options method of the FindOptions widget is used for two things:

It is used to get the options when find is run. In this case, all=False and the options are defined here.
It is also used in the closing_plugin method of the FindInFiles plugin for saving the options to the config file. In this case, all=True and the options are defined here.

The problem is that there is safeguards within the get_options method to prevent the find method to be run if one of the parameters is not correct. However, these safeguards also prevent the find in files options to be saved to the config file when Spyder is closed.

For example, the options won't be saved to the config file if the Search pattern line edit is empty:

Go in the FindInFiles plugin and add an external path to the Search in combobox.
Leave the Search pattern line edit empty and close Spyder.
Restart Spyder: the external path that was added in 1 does not appear in the list of the Search in combobox. It was not saved to the config file due to the safeguard here.

I think this should be fixed in another PR. I will built my tests around this issue for the moment.

jnsebgosselin · 2017-12-31T05:32:38Z

@ccordoba12 I think this is ready to be reviewed. I'll go over it again tomorrow just to be sure. There is a lot of stuff to review in this, so feel free to push it back to 3.2.7 if you do not have the time for 3.2.6. I'll try to document a little better the changes I've made later in the description of this PR.

Also, I'm not sure I understand well the use of to_text_string. It was used everywhere in findinfiles.py, so I did the same in the changes I've made.

More than 350 lines of tests added for a meager 0.3% increase in coverage... damn :D

ccordoba12 · 2017-12-31T14:43:08Z

spyder/widgets/findinfiles.py

-from qtpy.QtCore import QMutex, QMutexLocker, Qt, QThread, Signal, Slot, QSize
-from qtpy.QtWidgets import (QHBoxLayout, QLabel, QListWidget, QSizePolicy,
+from qtpy.QtCore import (QMutex, QMutexLocker, Qt, QThread, Signal, Slot,
+                         QSize, QEvent)


Please organize these imports in alphabetical order

ccordoba12 · 2017-12-31T14:43:18Z

spyder/widgets/findinfiles.py

                            QTreeWidgetItem, QVBoxLayout, QWidget,
                            QStyledItemDelegate, QStyleOptionViewItem,
-                            QApplication, QStyle, QListWidgetItem)
+                            QApplication, QStyle, QMessageBox)


These imports too

ccordoba12 · 2017-12-31T14:51:16Z

spyder/widgets/findinfiles.py

+        current position and added back at the end. If the maximum number of
+        paths is reached, the oldest external path is removed from the list.
+        """
+        if not os.path.exists(path):


Change os.path to osp (that's our custom in Spyder).

ccordoba12 · 2017-12-31T14:52:20Z

spyder/widgets/findinfiles.py

+            return
+        self.removeItem(self.findText(path))
+        self.addItem(path)
+        self.setItemData(self.count()-1, path, Qt.ToolTipRole)


Spaces around -

ccordoba12 · 2017-12-31T14:52:30Z

spyder/widgets/findinfiles.py

+        self.removeItem(self.findText(path))
+        self.addItem(path)
+        self.setItemData(self.count()-1, path, Qt.ToolTipRole)
+        while self.count() > MAX_PATH_HISTORY+EXTERNAL_PATHS:


Spaces around +

ccordoba12 · 2017-12-31T14:55:53Z

spyder/widgets/findinfiles.py

+
+    def get_external_paths(self):
+        """Returns a list of the external paths listed in the combobox."""
+        return [to_text_string(self.itemText(i))


We probably need to use here to_unicode_from_fs (from utils/encoding.py) to deal with non-ascii dirs on Windows that uses encodings like cp-1251 (Russian).

Sorry, this is not the proper place to do it. I'll point to the right place below.

Should I replace to_text_string(self.itemText(i) by self.itemText(i)?

I do not understand what to_text_string is doing here since self.itemText(i) is already returning a unicode str by default as stated, for instance, in the PySide documentation). Or is it returning a QString in some version of PyQt on python 2? In PySide, the QString class does not even exist I think.

All this encoding stuff is mumbo jumbo to me, no matter how many times I've read on this subject XD.

Ok, I've seen that to_text_string was always used with itemText everywhere else. So maybe it is safer to keep it that way?

OK, I've read that Python 2, PyQt API #1 is returning a QString, so I'll keep the to_text_string.

Ok, good idea.

ccordoba12 · 2017-12-31T14:57:08Z

spyder/widgets/findinfiles.py

+            external_path = self.select_directory()
+            if len(external_path) > 0:
+                self.add_external_path(external_path)
+                self.setCurrentIndex(self.count()-1)


Spaces around -

ccordoba12 · 2017-12-31T15:15:25Z

spyder/widgets/findinfiles.py

+        directory = getexistingdirectory(
+                self, _("Select directory"), self.path)
+        if directory:
+            directory = to_text_string(osp.abspath(to_text_string(directory)))


This is the right place to use to_unicode_from_fs. So this should be

directory = encoding.to_unicode_from_fs(osp.abspath(directory))

ccordoba12 · 2017-12-31T15:17:50Z

spyder/widgets/findinfiles.py

+            if self.currentIndex() == PROJECT:
+                self.setCurrentIndex(CWD)
+        else:
+            path = to_text_string(osp.abspath(to_text_string(path)))


Change this to

path = encoding.to_unicode_from_fs(osp.abspath(path))

Could you add some tests to verity that this is correct in Python 2?

Mmmh, it seems we're not transforming to Unicode our project paths, so maybe this is for another PR.

I opened issue #6103 about this (which I'll fix for 3.2.6), so please change this line to

path = osp.abspath(path)

ccordoba12 · 2017-12-31T15:18:25Z

spyder/widgets/findinfiles.py

+                self.removeItem(index)
+                self.showPopup()
+                # Set the view selection so that it doesn't bounce around.
+                new_index = min(self.count()-1, index)


Spaces around -

ccordoba12 · 2017-12-31T15:26:54Z

I like a lot the new SearchInComboBox class you added. It makes the code more modular and easier to understand. And, of course, I also like the new tests you added!

I left some minor comments, except for the use of to_unicode_from_fs in one place. Then I'd say this is ready.

jnsebgosselin · 2017-12-31T16:50:32Z

Thank you very much Carlos for this quick review, this is very nice!

To do:

Assert non-ASCII path in the tests.
Do the requested style changes.
Fix the to_unicode_from_fs and to_text_string.

jnsebgosselin · 2017-12-31T17:58:18Z

spyder/widgets/findinfiles.py

+                self.clear_external_paths()
+            self.setCurrentIndex(0)
+        elif idx >= EXTERNAL_PATHS:
+            self.external_path = to_text_string(self.itemText(idx))


Same comment here, should I replace to_text_string(self.itemText(idx)) by self.itemText(idx).

jnsebgosselin · 2017-12-31T18:04:14Z

spyder/widgets/findinfiles.py


    def set_directory(self, directory):
-        self.path = to_text_string(osp.abspath(to_text_string(directory)))
+        self.path_selection_combo.path = to_text_string(osp.abspath(
+                to_text_string(directory)))


Should to_text_string(osp.abspath(to_text_string(directory))) be changed to osp.abspath(directory) or to_unicode_from_fs(osp.abspath(directory))?

I see this is only used in one place:

https://github.com/jnsebgosselin/spyder/blob/8b9483d72967c4580143fde6adaeec38704f0dbd/spyder/plugins/findinfiles.py#L78

So this should be osp.abspath(directory) because getcwd_or_home already returns unicode.

Ok thank you. I'll remove the to_text_string here.

jnsebgosselin · 2017-12-31T18:05:37Z

spyder/widgets/findinfiles.py


    def set_file_path(self, path):
-        self.file_path = path
+        self.path_selection_combo.file_path = path


Should path be changed for osp.abspath(path) or to_unicode_from_fs(osp.abspath(path))?

Again, this is only used in one place

https://github.com/jnsebgosselin/spyder/blob/8b9483d72967c4580143fde6adaeec38704f0dbd/spyder/plugins/findinfiles.py#L86

and, since it sets the path of current file in the Editor, I think it's fine as it is, or you can change it to osp.abspath(path) if you want to be certain to have a full path.

Ok, I'll leave it like that. If it ain't broke, don't fix it.

jnsebgosselin · 2017-12-31T21:24:44Z

I added a non-ASCII path to the tests... and all hell breaks loose lol

The test is passing for Python3 in AppVeyor, but not for Python 2. I don't understand why the tests are failing on Travis though, I'm not sure it is related to this PR...

jnsebgosselin · 2017-12-31T21:51:07Z

The "good news" is that I'm able to make the test crash locally with Python2, so I should be able to debug this... but probably not until next year XD

This reverts commit 2ce5810.

…find_in_files_fixes

jnsebgosselin · 2018-01-08T16:49:40Z

@ccordoba12 Just so you know, this is ready, but I would like you to check again carefully some of the places where we return a path.

So good thinking in pushing it back to 3.2.7. It wouldn't be a bad idea if you could play a little bit with the changes I've made locally also.

ccordoba12 · 2018-01-08T17:16:23Z

but I would like you to check again carefully some of the places where we return a path.

I left my comments above.

So good thinking in pushing it back to 3.2.7.

Yep, I agree too. If things go as planned, we'll release 3.2.7 by the end of January.

The directory is already unicode. So there is no need to use to_text_string here.

ccordoba12

Thanks a lot Jean, this is a really great improvement!

Fixes #5306 Conflicts: - spyder/widgets/findinfiles.py - spyder/widgets/tests/test_findinfiles.py

ccordoba12 added this to the v3.2.6 milestone Dec 30, 2017

jnsebgosselin added 2 commits December 30, 2017 12:00

Remove item when Key_Delete is pressed

ee6035a

When the combobox popup list is visible and the delete key is pressed, the highlighted external path is removed from the list.

Add paths in the function to test the widget.

abaa55c

ccordoba12 changed the title ~~PR: Fixes bugs (3) and adds improvements (4) to the find in files functionality.~~ PR: Fix 3 bugs and add 4 improvements to Find in Files Dec 30, 2017

Add a "Clear this list" option to the combobox

05c33c3

jnsebgosselin added 3 commits December 30, 2017 18:00

Changed behaviour after Clear this list is refused

def85bb

fix codecov PR failed treshold to 1%

2ce5810

Add tests for the SearchInComboBox class

91cf958

jnsebgosselin added 3 commits December 30, 2017 23:21

Add a test for the FindInFiles plugin

0d046a3

Increase further test coverage for SearchInComboBox

5ceeaf2

Changed how paths are added to increase coverage

6d3f80b

jnsebgosselin self-assigned this Dec 31, 2017

ccordoba12 reviewed Dec 31, 2017

View reviewed changes

jnsebgosselin added 2 commits December 31, 2017 12:06

Organize imports in alphabetical order

a881a82

Minor code style changes

85d8455

jnsebgosselin commented Dec 31, 2017

View reviewed changes

jnsebgosselin added 2 commits December 31, 2017 14:43

Fix the to_unicode_from_fs and to_text_string

3ac0560

Assert non-ASCII path in the tests

278134b

Specify unicode string for NONASCII_DIR for python2

280bb30

ccordoba12 modified the milestones: v3.2.6, v3.2.7 Jan 4, 2018

jnsebgosselin mentioned this pull request Jan 6, 2018

PR: Mark introspection test slow and re-enable it on certain CIs #6138

Merged

jnsebgosselin added 2 commits January 8, 2018 01:58

Revert "fix codecov PR failed treshold to 1%"

434774a

This reverts commit 2ce5810.

Merge remote-tracking branch 'refs/remotes/spyder-ide/3.x' into some_…

8b9483d

…find_in_files_fixes

Removed not needed to_text_string in one method.

048e0d6

The directory is already unicode. So there is no need to use to_text_string here.

jnsebgosselin added component:Find in files User Experience (UX) labels Jan 9, 2018

jnsebgosselin mentioned this pull request Jan 9, 2018

Find in Files options not saved to spyder.ini #6176

Closed

ccordoba12 approved these changes Jan 10, 2018

View reviewed changes

ccordoba12 merged commit 5b0a09e into spyder-ide:3.x Jan 10, 2018

ccordoba12 added a commit that referenced this pull request Jan 10, 2018

Merge from 3.x: PR #6095

3ef9813

Fixes #5306 Conflicts: - spyder/widgets/findinfiles.py - spyder/widgets/tests/test_findinfiles.py

jnsebgosselin deleted the some_find_in_files_fixes branch January 10, 2018 02:23

PR: Fix 3 bugs and add 4 improvements to Find in Files #6095

PR: Fix 3 bugs and add 4 improvements to Find in Files #6095

Conversation

jnsebgosselin commented Dec 30, 2017 • edited Loading

jnsebgosselin commented Dec 30, 2017

Mouse Over and Delete key press demo:

Select other directory and Clear this list demo:

ccordoba12 commented Dec 30, 2017

pep8speaks commented Dec 30, 2017 • edited Loading

Comment last updated on January 09, 2018 at 05:20 Hours UTC

jnsebgosselin commented Dec 31, 2017

jnsebgosselin commented Dec 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnsebgosselin Dec 31, 2017 • edited Loading

Choose a reason for hiding this comment

jnsebgosselin Dec 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccordoba12 commented Dec 31, 2017

jnsebgosselin commented Dec 31, 2017 • edited Loading

To do:

Choose a reason for hiding this comment

jnsebgosselin Dec 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnsebgosselin commented Dec 31, 2017

jnsebgosselin commented Dec 31, 2017

jnsebgosselin commented Jan 8, 2018

ccordoba12 commented Jan 8, 2018

ccordoba12 left a comment

Choose a reason for hiding this comment

jnsebgosselin commented Dec 30, 2017 •

edited

Loading

pep8speaks commented Dec 30, 2017 •

edited

Loading

jnsebgosselin commented Dec 31, 2017 •

edited

Loading

jnsebgosselin Dec 31, 2017 •

edited

Loading

jnsebgosselin Dec 31, 2017 •

edited

Loading

jnsebgosselin commented Dec 31, 2017 •

edited

Loading

jnsebgosselin Dec 31, 2017 •

edited

Loading