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: Add toolbar to array editor (Variable Explorer) #21317

Merged
merged 6 commits into from
Nov 1, 2023

Conversation

jitseniesen
Copy link
Member

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)

This PR adds a toolbar to the array editor with five buttons: Edit, Copy, Format, Resize and Toggle Background. The last three are buttons currently at the bottom of the array editor, which are removed in this PR. The result looks as follows:

array-std

For 3d arrays, it's possible to save some space at the bottom by putting everything on one line; let me know if you would prefer that.

array-3d

Here is a video showing the toolbar buttons in action:

2023-09-08.21-11-33.mp4

Issue(s) Resolved

Fixes #21315

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: Jitse Niesen

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.

This looks really nice, thanks @jitseniesen! Besides my suggestions, I'd like to see the 3D additional widgets (i.e. Axis, Shape, Index and Slicing) appear in the same row as the Save and close/Close buttons.

Before it made sense to show them in a different row because there were buttons below them. But since you removed them in this PR, the layout looks odd now.

index = self.currentIndex()
if index.isValid():
self.edit(index)


class ArrayEditorWidget(QWidget):
Copy link
Member

Choose a reason for hiding this comment

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

Please make this widget inherit from SpyderWidgetMixin to use its API instead of building the new toolbar with low level functions.

Copy link
Member Author

Choose a reason for hiding this comment

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

I made the widget inherit from SpyderWidgetMixin but I don't think I understand how to use its API to build the toolbar. I saw the .create_toolbar() function which looks promising from its name but it creates a QToolBar instead of a SpyderToolbar.

Copy link
Member

@ccordoba12 ccordoba12 Sep 21, 2023

Choose a reason for hiding this comment

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

Right, that's something we need to think better for that mixin. Perhaps adding create_spyder_toolbar and renaming create_toolbar to create_qtoolbar would do the job, but that's not something for this PR.

spyder/plugins/variableexplorer/widgets/arrayeditor.py Outdated Show resolved Hide resolved
spyder/plugins/variableexplorer/widgets/arrayeditor.py Outdated Show resolved Hide resolved
spyder/plugins/variableexplorer/widgets/arrayeditor.py Outdated Show resolved Hide resolved
spyder/utils/icon_manager.py Outdated Show resolved Hide resolved
@ccordoba12 ccordoba12 changed the title PR: Add toolbar to array editor PR: Add toolbar to array editor (Variable Explorer) Sep 10, 2023
@jitseniesen
Copy link
Member Author

This looks really nice, thanks @jitseniesen! Besides my suggestions, I'd like to see the 3D additional widgets (i.e. Axis, Shape, Index and Slicing) appear in the same row as the Save and close/Close buttons.

Before it made sense to show them in a different row because there were buttons below them. But since you removed them in this PR, the layout looks odd now.

Done. Now it looks like this for a 3d array:

array-3d

@pep8speaks

This comment was marked as off-topic.

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 @jitseniesen for your work on this! Last suggestions for you then it should be ready.

Also, please merge with master or rebase to get the fixes to our tests merged last week.

@jitseniesen
Copy link
Member Author

SpyderWidgetMixin also has a high level API to assign icons to actions, so we can use it here too.

Fine with me, so done, though I don't see the advantage of the SpyderWidgetMixin function.

Also, please merge with master or rebase to get the fixes to our tests merged last week.

Done.

@jitseniesen jitseniesen marked this pull request as draft September 23, 2023 23:31
@ccordoba12
Copy link
Member

ccordoba12 commented Sep 24, 2023

Fine with me, so done, though I don't see the advantage of the SpyderWidgetMixin function.

You don't need to import the lower level ima.icon function to do that.

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 @jitseniesen!

@ccordoba12 ccordoba12 merged commit 467cf7a into spyder-ide:master Nov 1, 2023
@jitseniesen jitseniesen deleted the array-toolbar branch December 8, 2023 22:12
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.

Array editor should have a toolbar
3 participants