PR: Add context menu entries for collections to a toolbar (Variable Explorer)

[dpturibio]

Description of Changes

• Wrote at least one-line docstrings (for any new functions)
• Added unit test(s) covering the changes (if testable)

• Included a screenshot or animation (if affecting the UI, see Licecap)

Incorporating a static toolbar in the top of the list/table window of Variable Explorer as shown in the picture bellow.

Issue(s) Resolved

Fixes spyder-ide/ux-improvements#67

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:

[pep8speaks]

Hello @dpturibio! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file spyder/utils/icon_manager.py:

Line 201:80: E501 line too long (107 > 79 characters)
Line 202:80: E501 line too long (105 > 79 characters)

Comment last updated at 2022-04-14 23:40:37 UTC

[dpturibio]

@dalthviz I am afraid that I can not mark you as a reviewer(as you asked me in the issue spyder-ide/ux-improvements#67) because of my user privileges. Please let me know if you can do this by yourself, or if I can take any other action in order to do this.

[dalthviz]

No problem @dpturibio I didn't know that you can only request a review with certain privileges. Will add me here and give it a review. Thanks again for helping with this 👍

[dalthviz]

Thank you @dpturibio for working on this! Checking locally things seem working except for some cases related to actions not being disabled when no items are selected. We need to somehow attach the selection of variables to the BaseTableView.refresh_menu method to change the actions state (enable/disable) dynamically.

Probably, we need to call the refresh_menu after creating the toolbar with something like self.editor.refresh_menu() but also attach that method to a signal of the editor to do the updates when needed (I'm not totally sure about what to do for that so I need to take some time to check).

So basically for this to be ready we need to figure out a way to change the enable/disable state for the actions shown in the new toolbar depending on what is currently selected in the editor.

[dalthviz]

After checking a little bit, I left a comment about some additions that could help with the missing thing I talked about in my first review (the toolbar actions state update).

If you have any question @dpturibio let us know!

[dalthviz]

Hi again @dpturibio ! After checking the traceback of the tests I think we will need to overwrite the refresh_menu method for the RemoteCollectionsEditorTableView class. For that please add this method on the RemoteCollectionsEditorTableView class:

    def refresh_menu(self):
        if self.var_properties:
            super().refresh_menu()

With that change the tests should pass 🤞🏼

[dpturibio]

Thank you @dalthviz for checking this. I was studying what could be causing the failures in the tests.
I will overwrite the method and check what happens.
Thanks a lot for the support.

[dpturibio]

Hello @dalthviz, any idea about what is causing the failures on tests?

[dalthviz]

Hi again @dpturibio I think we need to rebase your work into the latest 5.x branch to make the tests pass. For that you will need to setup as upstream the spyder repository in your local fork clone and then rebase do the upstream 5.x branch. So you will need to run something like this (with the your 5.x branch checked out in your local clone):

git remote add upstream https://github.com/spyder-ide/spyder.git
git fetch upstream
git rebase upstream/5.x
git push -f

If you need help doing the rebase let me know and I will do it for you 👍

[dpturibio]

Hello @dalthviz, after rebase the tests ran successfully. Thanks a lot for the assistance.
I think the next step is review and merge the pull request right?
Please, let me know if you need something else from my side to conclude this pull request.

[dalthviz]

Hi again @dpturibio I think we need to fix some PEP 8 issues noted here: #17473 (comment) but otherwise this looks good to me 👍

Maybe @ccordoba12 could have more comments. So as you said basically this is missing fixing the PEP 8 issues and a final review. Then we can proceed with the merge 👍

[ccordoba12]

Great work @dpturibio! The only thing I'm not so sure about are the Resize to rows/columns to contents buttons. I think mixing such long text buttons with icon-based ones is a bit odd.

If you think they are really important to be placed in the toolbar, perhaps we should look for icons for them?

[dpturibio]

Thank you @dalthviz and @ccordoba12.
Is there any repository where I can look for icons? Can I create by myself them based on other Spyder icons?
Please let me know what is the procedure in this case.

[dalthviz]

Maybe for the icons we could use:

• mdi.arrow-collapse-horizontal for columns resize:

• mdi.arrow-collapse-vertical for rows resize

Just in case @dpturibio , I checked these icons using the qta-browser of QtAwesome (the project used by Spyder for the icons): https://github.com/spyder-ide/qtawesome

[dpturibio]

@dalthviz thank you once again for the help.
bellow is the implementation with the buttons. What do you think @ccordoba12 ?

regarding pep8 remaining issues(line too long) #17473 (comment), I think that in this case is acceptable to follow the pattern of lines above and bellow. Is that right?

[dalthviz]

Thank you @dpturibio for all your work (from the idea discussion to the actual implementation here!)

regarding pep8 remaining issues(line too long) #17473 (comment), I think that in this case is acceptable to follow the pattern of lines above and bellow. Is that right?

Yep, no problem with those 👍

[dalthviz]

Thanks @dpturibio !

[dpturibio]

Thanks @dpturibio !

Thank you for all the support :-)

[ccordoba12]

This is looking great, thanks @dpturibio! One small suggestion, otherwise looks good to me.

[ccordoba12]

@dpturibio, you forgot to do a couple of changes, then I'll merge.

[ccordoba12]

Thanks for your contribution @dpturibio! Great work here!!