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.

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

PR: Make the switcher a plugin #20714

Merged
merged 33 commits into from
Apr 7, 2023

Conversation

AngelaRemolina
Copy link
Contributor

@AngelaRemolina AngelaRemolina commented Mar 22, 2023

Description of Changes

  • I transformed the File Switcher widget into a SpyderPluginV2. However it is not working yet, because the old widget worked in the editor with a manager called EditorSwitcherManager and the methods that this manager handles are not migrated yet.
  • Skipped switcher tests until migration is finished

What is left to do

  • The switcher plugin needs a public API that contains the methods of the EditorSwitcherManager
  • The order of the file menu got mixed up when the switcher action section was added. I tried to fix it with the before_section parameter in each section, but it didn't work. There might be a bug in the way the menus are getting ordered:
    def _add_section(self, section, before_section=None):
  • Included a screenshot or animation (if affecting the UI, see Licecap)
    mixed_order

Issue(s) Resolved

This is necessary to solve #3860

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

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.

Hey @angelasofiaremolinagutierrez, your code looks really good and almost ready to be merged. Thanks for your work on it!

Most of my suggestions are purely stylistic. In case you haven't worked with Github suggestions before, please take a look at this page to understand how to apply them in a single commit.

spyder/plugins/switcher/confpage.py Outdated Show resolved Hide resolved
spyder/plugins/switcher/container.py Show resolved Hide resolved
spyder/plugins/switcher/container.py Outdated Show resolved Hide resolved
spyder/plugins/switcher/container.py Show resolved Hide resolved
spyder/plugins/switcher/container.py Show resolved Hide resolved
spyder/plugins/switcher/widgets/switcheritem.py Outdated Show resolved Hide resolved
spyder/plugins/switcher/widgets/switcheritem.py Outdated Show resolved Hide resolved
spyder/plugins/switcher/widgets/switcheritem.py Outdated Show resolved Hide resolved
spyder/plugins/switcher/widgets/switcheritem.py Outdated Show resolved Hide resolved
spyder/plugins/switcher/widgets/switcheritem.py Outdated Show resolved Hide resolved
@ccordoba12 ccordoba12 changed the title Migrate switcher widget to plugin PR: Make the switcher a plugin Mar 25, 2023
@ccordoba12 ccordoba12 added this to the v6.0alpha2 milestone Mar 25, 2023
@ccordoba12
Copy link
Member

Besides my comments above, you also need to add __init__.py files to the widgets and tests directories like this one:

https://github.com/spyder-ide/spyder/blob/master/spyder/plugins/plots/__init__.py

That way Python will recognize those directories as proper modules.

AngelaRemolina and others added 6 commits March 25, 2023 23:43
Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>
Fixes missing from first time

Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>
Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>
Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

Thank you for all the work here @angelasofiaremolinagutierrez ! Left some suggestions to hopefully fix the failing tests. The suggestions mainly focus on:

  • Some extra code that got readded and should be removed related to editor actions that were being done inside the mainwindow but that are not there anymore (create_edit_action related things).
  • Some extra changes in the config related files to make the switcher shortcuts work.
  • A way to keep the switcher working by creating the EditorSwitcherManager instance
  • Some code style things (extra readded comments to remove, a white space, etc).

If you have any question regarding the suggestions let me know 👍

spyder/app/mainwindow.py Outdated Show resolved Hide resolved
spyder/config/main.py Outdated Show resolved Hide resolved
spyder/plugins/switcher/container.py Outdated Show resolved Hide resolved
spyder/config/main.py Outdated Show resolved Hide resolved
spyder/config/main.py Outdated Show resolved Hide resolved
spyder/config/main.py Outdated Show resolved Hide resolved
spyder/plugins/editor/plugin.py Outdated Show resolved Hide resolved
spyder/plugins/editor/plugin.py Outdated Show resolved Hide resolved
AngelaRemolina and others added 2 commits March 28, 2023 23:51
Standardize code style and remove code that was re-added from resolving conflict

Co-authored-by: Daniel Althviz Moré <d.althviz10@uniandes.edu.co>
add switcher to context extra valid list
@ccordoba12
Copy link
Member

Closing and reopening to restart our tests.

@ccordoba12 ccordoba12 closed this Mar 29, 2023
@ccordoba12 ccordoba12 reopened this Mar 29, 2023
@ccordoba12 ccordoba12 closed this Mar 29, 2023
@ccordoba12 ccordoba12 reopened this Mar 29, 2023
These functions were previously removed because they were only used in tests that used to be skipped. Now that they are not skipped they are added again.
Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

After checking for some time I think I found a way to workaround the issue with the sections order in the File menu @angelasofiaremolinagutierrez @ccordoba12 !

Left some comments to address that. Also, left some code style suggestions that I found while checking the code.

Finally, checked locally and seems like everything is working as expected 🎉, so other than the above this LGTM 👍

spyder/plugins/switcher/plugin.py Outdated Show resolved Hide resolved
spyder/plugins/switcher/plugin.py Outdated Show resolved Hide resolved
spyder/plugins/switcher/plugin.py Outdated Show resolved Hide resolved
spyder/plugins/editor/plugin.py Outdated Show resolved Hide resolved
spyder/plugins/switcher/widgets/item.py Outdated Show resolved Hide resolved
spyder/plugins/switcher/widgets/proxymodel.py Outdated Show resolved Hide resolved
@ccordoba12 ccordoba12 modified the milestones: v6.0alpha2, v6.0alpha1 Apr 7, 2023
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 @angelasofiaremolinagutierrez for your great work here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants