PR: Compute Projects switcher results in a worker to avoid freezes #21275

ccordoba12 · 2023-08-20T22:02:25Z

Description of Changes

Since the initial implementation was synchronous, the interface was freezing when using the Switcher with large projects (i.e. projects that have more than 1000 files).
Highlight matched characters of Projects switcher results that match the searched text.
Remove binary files from the results computed by fzf because they can't be opened in the editor.
Fix process workers because they were not working as expected.
Simplify the way sections are shown in the Switcher, which was not very efficient and hard to understand.
Improve test that checks the integration between Projects and the Switcher.
Set app and monospace interface fonts when running single main window tests. That avoids a ton of Qt warnings to be displayed.

Issue(s) Resolved

Fixes #20940.

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct: @ccordoba12

- ProcessWorkers don't need threads, so they are not necessary. - Also, add a method to ProcessWorker to set its current working directory.

Also, reduce the number of results shown in the switcher to display it without lags.

This is necessary now that fzf is called asynchronously.

Also, rename methods in Projects related to switcher searching to be more meaningful.

It's not clear why that was required before.

This is necessary to make some adjustments in the switcher when that's the case.

Also, avoid a small lag when opening the symbol finder by calling the `setup` method directly after that.

Also, simplify when `setup_sections` is called: instead of doing it every time an item is added, now we do it after all of them have been sorted.

Also, change that section name to "Project" because we don't support multiple projects.

…nd moves

Also, expand that test to check that: - We make sections visible for the right number of items. - Searching in the switcher is working as expected. - The switcher is updated when removing files.

That avoids a ton of Qt warnings to be displayed.

dalthviz

Thanks @ccordoba12 ! I was checking this locally and seems like there is some delays still for certain files? I tried with a dummy project with a lot of .parquet files and I noticed a lot of lag and also the files not appearing vs checking .py was more quick.

Checked with latest master and although there you can sense some general lag it shows the .parquet as well as the .py files:

Maybe there is now some logic/validation about what files to show that needs to be move to a worker too?

Also, I was thinking and maybe adding/showing some sort of loading indicator while searching could be worthy?

That's much faster than trying to detect if they are binary to exclude them.

ccordoba12 · 2023-08-22T01:41:43Z

I was checking this locally and seems like there is some delays still for certain files? I tried with a dummy project with a lot of .parquet files and I noticed a lot of lag and I noticed a lot of lag and also the files not appearing vs checking .py was more quick.

I was using encoding.is_text_file to detect binary files and exclude them from the switcher results. That's necessary because they can't be opened in the Editor, so there's no value in showing them. But I didn't realize that that's quite expensive for large binary files.

However, I changed that to simply compare the file extension of the fzf results with the list of extensions that can be opened in the editor, and the lag was removed for me. So, please test again.

Checked with latest master and although there you can sense some general lag it shows the .parquet as well as the .py files

As I said, there's no value in showing binary files if you want to quickly find and open a project file in the editor.

Also, I was thinking and maybe adding/showing some sort of loading indicator while searching could be worthy?

I agree, but that's a UI improvement that I have planned for later.

dalthviz

Thanks @ccordoba12 ! Left comment/suggestion regarding a commented out line and a comment. Also, checking locally seems like an entry for the project root directory is created (not sure if that is intended or not). Other than that this LGTM 👍

spyder/plugins/switcher/widgets/switcher.py

spyder/plugins/projects/widgets/main_widget.py

This only happened on Windows

ccordoba12 · 2023-08-23T16:58:03Z

@dalthviz, besides fixing the problem you reported in your last review, I also added some changes to prevent trying to populate the switcher when closing it.

dalthviz · 2023-08-23T20:04:08Z

Thanks for the fixes @ccordoba12 ! Checked again and the pointed elements look good now 👍 . However while checking again, I realized that I haven't checked this without fzf installed and seems like in that case another weird behavior happens. Seems like without fzf the open files filtering doesn't work :/

- This helps to simplify the _on_search_text_changed method of its widget. - Use that method from Projects to remove the items added by it before painting new ones.

That way its results can be filtered when fzf is not present.

That created a special case for Projects, which is not really necessary.

ccordoba12 · 2023-08-25T01:45:13Z

However while checking again, I realized that I haven't checked this without fzf installed and seems like in that case another weird behavior happens

Thanks for noticing that @dalthviz! It should be fixed now with my last commits.

When I was doing those changes, I also realized that we don't need to create a special, unnecessary dependency between the Switcher and Projects. So, I decided to remove it.

dalthviz · 2023-08-25T20:49:57Z

Checked again @ccordoba12 and seems like while the filtering works without fzf, when it is installed the project section always shows regardles of the text in the search line edit?:

Also, should we add some tests for the different scenarios we have been discussing here (trying to search for a binary file, trying to search with and without fzf available, searching for a non existent file, etc)?

ccordoba12 · 2023-08-25T21:34:42Z

and seems like while the filtering works without fzf

Great!

when it is installed the project section always shows regardles of the text in the search line edit?

Yep, that's the behavior implemented before and I kept it in this PR too. I think that's more a UX issue than a backend problem (which is what I'm trying to fix here), so it should be left for later. In addition, we're currently discussing about that in our UX meetings and haven't decided what to do yet.

dalthviz

Thanks @ccordoba12 ! This LGTM. However, before merging, do you think that adding more tests is worthy? If not feel free to merge 👍

ccordoba12 · 2023-08-25T22:24:01Z

However, before merging, do you think that adding more tests is worthy?

Good point and sorry for missing the comment you posted about it (I just read it):

Also, should we add some tests for the different scenarios we have been discussing here (trying to search for a binary file, trying to search with and without fzf available, searching for a non existent file, etc)?

I'll expand our current test for the projects/switcher integration to cover those scenarios as well.

This includes new cases for binary and nonexistent files, and missing fzf.

ccordoba12 · 2023-08-26T22:14:50Z

Test improved, so I'm going to merge this one.

Thanks @dalthviz for all your help!

ccordoba12 added 16 commits August 14, 2023 22:47

Utils: Don't use threads when starting ProcessWorkers in WorkerManager

0cf2757

- ProcessWorkers don't need threads, so they are not necessary. - Also, add a method to ProcessWorker to set its current working directory.

Utils: Add some logging to WorkerManager

6bfa0e0

Projects: Remove unnecessary method

47a9d37

Projects: Call fzf in a worker to not block the interface

071e163

Also, reduce the number of results shown in the switcher to display it without lags.

Projects: Detect if fzf is present in the system before trying to use it

79bbc04

Projects: Call switcher setup method after adding project items

a81d5b2

This is necessary now that fzf is called asynchronously.

Projects: Remove repeated code in several places

7a9d7b1

Switcher: Remove items_data of sig_search_text_available signal

3aa2b35

Also, rename methods in Projects related to switcher searching to be more meaningful.

Remove lower case path names in the Switcher

ed7d6fa

It's not clear why that was required before.

Projects: Remove binary files from the results returned by fzf

df972f4

Switcher: Detect when a project is active

4840e17

This is necessary to make some adjustments in the switcher when that's the case.

Switcher: Fix searching when no project is active

d96203f

Also, avoid a small lag when opening the symbol finder by calling the `setup` method directly after that.

Switcher: Fix showing sections when a project is active

6ee0ef3

Also, simplify when `setup_sections` is called: instead of doing it every time an item is added, now we do it after all of them have been sorted.

Switcher: Remove hard-coded projects section

5099dfe

Also, change that section name to "Project" because we don't support multiple projects.

Projects: Update default switcher paths on files creation, deletion a…

db685ef

…nd moves

Testing: Fix test_switcher_project_files

e932b7f

Also, expand that test to check that: - We make sections visible for the right number of items. - Searching in the switcher is working as expected. - The switcher is updated when removing files.

ccordoba12 added type:Bug component:File Switcher labels Aug 20, 2023

ccordoba12 added this to the v6.0alpha2 milestone Aug 20, 2023

ccordoba12 requested a review from dalthviz August 20, 2023 22:02

ccordoba12 self-assigned this Aug 20, 2023

ccordoba12 added the type:Enhancement label Aug 20, 2023

App: Set app and monospace interface fonts when running single tests

843535f

That avoids a ton of Qt warnings to be displayed.

ccordoba12 force-pushed the issue-20940 branch from 4c9975a to 843535f Compare August 20, 2023 22:06

dalthviz reviewed Aug 21, 2023

View reviewed changes

Projects: Filter switcher files according to their extension

3843986

That's much faster than trying to detect if they are binary to exclude them.

ccordoba12 added the component:Projects label Aug 22, 2023

dalthviz reviewed Aug 22, 2023

View reviewed changes

spyder/plugins/switcher/widgets/switcher.py Outdated Show resolved Hide resolved

spyder/plugins/switcher/widgets/switcher.py Show resolved Hide resolved

spyder/plugins/projects/widgets/main_widget.py Outdated Show resolved Hide resolved

Projects: Prevent showing project path when fzf results are empty

e5483ae

This only happened on Windows

Switcher: Prevent to populate the switcher when closing it

b3624fe

ccordoba12 added 4 commits August 24, 2023 19:45

Switcher: Add a new public method to remove all items in a section

36a39c7

- This helps to simplify the _on_search_text_changed method of its widget. - Use that method from Projects to remove the items added by it before painting new ones.

Editor: Connect to the Switcher's sig_search_text_available signal

60466aa

That way its results can be filtered when fzf is not present.

Switcher: Add attribute to items to check if their score can be changed

5480966

Switcher: Remove direct dependency on Projects

e6140b1

That created a special case for Projects, which is not really necessary.

dalthviz approved these changes Aug 25, 2023

View reviewed changes

Projects: Fix clearing the switcher when fzf can't find any match

a36cc6f

ccordoba12 force-pushed the issue-20940 branch from 4b863ac to 907395c Compare August 26, 2023 16:45

Testing: Expand test that checks Switcher/Projects integration

d51c98d

This includes new cases for binary and nonexistent files, and missing fzf.

ccordoba12 force-pushed the issue-20940 branch from 907395c to d51c98d Compare August 26, 2023 16:49

ccordoba12 merged commit c5de2ed into spyder-ide:master Aug 26, 2023
20 checks passed

ccordoba12 deleted the issue-20940 branch August 26, 2023 22:15

ccordoba12 mentioned this pull request Jan 1, 2024

PR: Fix errors when displaying the Symbols switcher #21667

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PR: Compute Projects switcher results in a worker to avoid freezes #21275

PR: Compute Projects switcher results in a worker to avoid freezes #21275

ccordoba12 commented Aug 20, 2023 •

edited

Loading

dalthviz left a comment

ccordoba12 commented Aug 22, 2023 •

edited

Loading

dalthviz left a comment

ccordoba12 commented Aug 23, 2023

dalthviz commented Aug 23, 2023

ccordoba12 commented Aug 25, 2023 •

edited

Loading

dalthviz commented Aug 25, 2023

ccordoba12 commented Aug 25, 2023 •

edited

Loading

dalthviz left a comment

ccordoba12 commented Aug 25, 2023 •

edited

Loading

ccordoba12 commented Aug 26, 2023

PR: Compute Projects switcher results in a worker to avoid freezes #21275

PR: Compute Projects switcher results in a worker to avoid freezes #21275

Conversation

ccordoba12 commented Aug 20, 2023 • edited Loading

Description of Changes

Issue(s) Resolved

Affirmation

dalthviz left a comment

Choose a reason for hiding this comment

ccordoba12 commented Aug 22, 2023 • edited Loading

dalthviz left a comment

Choose a reason for hiding this comment

ccordoba12 commented Aug 23, 2023

dalthviz commented Aug 23, 2023

ccordoba12 commented Aug 25, 2023 • edited Loading

dalthviz commented Aug 25, 2023

ccordoba12 commented Aug 25, 2023 • edited Loading

dalthviz left a comment

Choose a reason for hiding this comment

ccordoba12 commented Aug 25, 2023 • edited Loading

ccordoba12 commented Aug 26, 2023

ccordoba12 commented Aug 20, 2023 •

edited

Loading

ccordoba12 commented Aug 22, 2023 •

edited

Loading

ccordoba12 commented Aug 25, 2023 •

edited

Loading

ccordoba12 commented Aug 25, 2023 •

edited

Loading

ccordoba12 commented Aug 25, 2023 •

edited

Loading