Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

PR: Fix extraneous reloading documents on project switching #15681

Merged
merged 7 commits into from
Jun 29, 2021
Merged

PR: Fix extraneous reloading documents on project switching #15681

merged 7 commits into from
Jun 29, 2021

Conversation

mrclary
Copy link
Contributor

@mrclary mrclary commented May 23, 2021 •
edited
Loading

Description of Changes

  • When switching projects, project close emit will call editor.setup_open_files only when not switching projects.
  • Removed redundant code in create_new_project. This lead to multiple unnecessary calls to sig_project_closed.emit and restart_consoles.
  • Removed unnecessary test for project close signal on create new project. This test fails because a project root_path is not set in the mocked dialogue window so a project is not actually created and the active project is not closed. This test is unnecessary since create_new_project should not send sig_project_closed.emit, which is sent in open_project (if switching projects) or the close_project and is already tested.

Issue(s) Resolved

Fixes #14787

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:
@mrclary

@mrclary mrclary requested a review from ccordoba12 May 23, 2021 22:08
@mrclary mrclary force-pushed the issue-14787-project-switching branch 8 times, most recently from b4a398f to 748966b Compare May 27, 2021 04:54
@mrclary mrclary linked an issue May 27, 2021 that may be closed by this pull request
Editor extraneously reloads files when switching projects #14787
Closed
@mrclary
Copy link
Contributor Author

mrclary commented May 27, 2021

@ccordoba12, this is ready for review.

@mrclary mrclary force-pushed the issue-14787-project-switching branch 2 times, most recently from c94208d to 96e594e Compare June 4, 2021 18:47
@mrclary mrclary force-pushed the issue-14787-project-switching branch from 96e594e to 652b0e1 Compare June 19, 2021 19:49
@mrclary mrclary force-pushed the issue-14787-project-switching branch 2 times, most recently from 723b2c9 to 764eebf Compare June 29, 2021 01:06
@ccordoba12 ccordoba12 added this to the v5.1.0 milestone Jun 29, 2021
@ccordoba12
Copy link
Member

When switching projects, project close emit will call editor.setup_open_files only when not switching projects.

So what happens when you switch projects? I mean, how the files of the project that is opened (after the current one is closed) are set in the editor?

@mrclary
Copy link
Contributor Author

mrclary commented Jun 29, 2021 •
edited
Loading

When switching projects, project close emit will call editor.setup_open_files only when not switching projects.

So what happens when you switch projects? I mean, how the files of the project that is opened (after the current one is closed) are set in the editor?

The editor.setup_open_files gets called with the sig_project_loaded.emit in the open_project method of the Projects plugin class. So basically, it gets called when a project gets closed, also when a project gets opened; but when one switches a project, it was called twice (once with the sig_project_closed and once with the sig_project_loaded), causing the Editor to reload the files in the first project before loading the files in the second project. So, I made a switch in sig_project_closed to distinguish between events where the project is being closed only, and events where the project is closed for switching to another project. In this way, the setup_open_files is always called, but only once.

ccordoba12
ccordoba12 reviewed Jun 29, 2021
View reviewed changes
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mrclary for your help with this! I think by using sig_project_closed with an additional signature, your PR can be made much simpler, so all my suggestions below are oriented towards that.

spyder/plugins/projects/plugin.py Outdated Show resolved Hide resolved
spyder/plugins/projects/plugin.py Outdated Show resolved Hide resolved
spyder/plugins/projects/plugin.py Outdated Show resolved Hide resolved
spyder/plugins/projects/plugin.py Outdated Show resolved Hide resolved
spyder/plugins/projects/plugin.py Outdated Show resolved Hide resolved
spyder/plugins/projects/plugin.py Outdated Show resolved Hide resolved
spyder/plugins/projects/plugin.py Outdated Show resolved Hide resolved
spyder/plugins/projects/plugin.py Outdated Show resolved Hide resolved
spyder/plugins/projects/plugin.py Outdated Show resolved Hide resolved
spyder/plugins/workingdirectory/plugin.py Outdated Show resolved Hide resolved
@mrclary
* When switching projects, do not call setup_open_files on project cl…
369569f 
…ose emit. This causes existing project files to be reloaded before new project files are loaded.
@mrclary
* Remove redundant restart_consoles in create_project. This is alread…
85d499d 
…y called in open_project.
@mrclary
* Remove redundant sig_pythonpath_changed.emit in create_project. Thi…
3f9f76d 
…s is already called in open_project.
@mrclary
* remove redundant sig_project_closed.emit. This is called in open_pr…
ef65e57 
…oject.
@mrclary
* With redundant close emit removed, this test fails because a projec…
f25574b 
…t is not actually created, so the active one is not closed and the emit signal is not sent. i.e. it relied on an unnecessary close emit.

* This test is actually redundant since a successful creation of a project will call open_project which will send the close emit and is already tested.
@mrclary
* Remove redundant check on explorer visibility. This is already done…
1cb3816 
… in open_project.
@mrclary
* Remove unused import
01ab43f
@mrclary mrclary force-pushed the issue-14787-project-switching branch from 764eebf to 01ab43f Compare June 29, 2021 06:22
@mrclary
Copy link
Contributor Author

mrclary commented Jun 29, 2021

Thanks @mrclary for your help with this! I think by using sig_project_closed with an additional signature, your PR can be made much simpler, so all my suggestions below are oriented towards that.

@ccordoba12, thanks for the suggestions! I learned a lot from that.

@mrclary mrclary requested a review from ccordoba12 June 29, 2021 06:23
@ccordoba12
Copy link
Member

@ccordoba12, thanks for the suggestions! I learned a lot from that.

I'm really glad about it!

ccordoba12
ccordoba12 approved these changes Jun 29, 2021
View reviewed changes
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me now, thanks @mrclary!

@ccordoba12 ccordoba12 merged commit c421240 into spyder-ide:5.x Jun 29, 2021
ccordoba12 added a commit that referenced this pull request Jun 29, 2021
@ccordoba12
Merge from 5.x: PR #15681
8d44b1a 
Fixes #14787
@mrclary mrclary deleted the issue-14787-project-switching branch June 29, 2021 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ccordoba12 ccordoba12 ccordoba12 approved these changes

Assignees

@mrclary mrclary

Labels
None yet
Projects
None yet
Milestone
v5.1.0
Development

Successfully merging this pull request may close these issues.

Editor extraneously reloads files when switching projects
2 participants
@mrclary @ccordoba12