PR: Unify UI of editors in Variable Explorer and simplify code #21666
Conversation
The toolbar is styled as a pane toolbar. This function used to return a QToolBar, but it was not used anywhere.
The new parameter is added to the funcions create_menu(), create_toolbar() and create_toolbutton. It indicates whether the menu, toolbar or toolbuttion is added to the global registry. This is to allow the caller to prevent adding temporary menus, toolbars and toolbuttons, such as those in dialog boxes, to the registry forever.
This is to allow dialog boxes to use the mixin to create toolbars.
Use the function create_menu from SpyderWidgetMixin to create menus in the array editor, the collections editor, the dataframe editor and the object explorer. This is to standardize the appearance and the code.
Use the function create_toolbar from SpyderWidgetMixin to create toolbars and toolbuttons in the array editor, the collections editor, the dataframe editor and the object explorer. This is to standardize the appearance and the code. The biggest change is in the object explorer, where the toolbar is clearly styled differently (and consistent with the rest of Spyder).
Use the function create_action from SpyderWidgetMixin to create actions in the array editor, the collections editor, the dataframe editor and the object explorer. This is to standardize the UX and the code. This does remove the keyboard shortcuts for the actions in the object explorer, but these shortcuts were not shown anywhere so users had little change to discover them. This means that users are unlikely to miss the shortcuts.
Simplify "Show callable attributes" and "Show __special__ attributes" by: * adding the actions to the toolbar, instead of first creating a toolbutton and then adding that to the toolbar, * adding a new function to connect the action to and connect only once, instead of disconnecting and re-connecting every time that the value in the editor changes, * using the functions in SpyderWidgetMixin to handle the configuration option.
- Remove verticle space between toolbar and editor - Align right edge of editor and buttons at bottom - Remove extra spacing around buttons at bottom
This commit re-orders the buttons in the toolbars for the array editor, collections editor, dataframe editor and object explorer. The Refresh button is the only button present for every editor and is probably one of the most common buttons users will click, as well as being something that applies to the whole object and always enabled, so it makes sense to put it first. The Resize buttons (only present for the array and dataframe editors) also apply to the whole object and are always enabled, so they are put next.
Remove the Refresh item from the context menu of the dataframe editor and the Resize Rows and Resize Column items from the context menus of both the collections and the dataframe editors. The reason is that these are already present in the toolbar and not context specific.
This is a refactor to simplify the code.
* Rename action in order to match the wording of the other editors in the Variable Explorer and avoid confusion in nested arrays. * Remove from toolbar because it is already in the context menu and that is the logical place because it acts on the selected item.
Specifically, * Remove Edit and Copy from the toolbar of the array editor, and add Edit to the context menu (for consistency). * Remove Edit and Copy from the toolbar of the dataframe editor. * Remove Edit, Copy, Paste and Rename from the toolbar of the collections editor. These actions do not belong to the toolbars because they are highly context-sensitive, basic and already easy to trigger.
Specifically, change the context menus and toolbars of the collections and dataframe editors, and put Remove (Row/Column) after Duplicate (Row/Column), where it logically belongs. Also put Copy before Edit in the context menus for consistency.
The functionality from the four buttons is handled as follows: * Add Format button to toolbar * There already is a Resize button in the toolbar * Add Background Color button to toolbar * Add options menu and put Column Min/Max item in there
This fixes a mistake in commit 7d76a88. That commit introduced a new parameter `register` in the member function create_toolbutton() in SpyderToolButtonMixin. The default value made this a change in behaviour which broke some tests of the outline explorer. The new default value ensures that the behaviour is not changed if no value for the new parameter is given, making the change backward compatible.
This fixes the test failure in test_arrayeditor_with_inf_array.
* Move Refresh button to the right in the array and dataframe editors and put it immediately before the options menu. * Move the Search, Filter and Refresh buttons in the main widget of the Variable Editor immediately before the options menu. in the array and dataframe editor * Move the Toggle background and Format actions in the array and dataframe editors to the options menu. * Add separators in the toolbar of dataframe and collections editors.
|
I added a commit which reflects the latest comment on the issue regarding the UI. I also updated the screenshots at the top of this pull request and added a screenshot of the Variable Editor itself. |
There was a problem hiding this comment.
Thanks so much for these great improvements, @jitseniesen ! (Didn't see this until now, sorry, since I was watching the issue but not this PR)
I just have a few followup comments/suggestions:
- Seems like
insert aboveis belowinsert belowfor some reason? Seems like they should be reversed (as in my original suggestion) - Seems like the refresh button is on the left rather than the right in the collections editor toolbar?
- The (normally blank) circular progress indicator leaves a gap between the refresh button and the options menu; we should move that to the left of the right toolbar button group (i.e. to the left of the search button)
- Upon seeing how they look in the UI, I think we should split the row and column actions in DF viewer context menu and toolbar into separate groups, instead of having them all in one big one. This would make them easier to navigate and not seem too overwhelming to users. So instead of
Insert above,Insert below,Insert beforeInsert afterDuplicate rowDuplicate columnRemove RowRemove Column; I suggestInsert above,Insert below,Duplicate rowRemove Row|Insert beforeInsert afterDuplicate columnRemove Column. - Finally, I went back and forth on this a lot before and almost suggested it previously but didn't, but given the column resize buttons in the array and df editors affect only the presentation rather than the content itself, I think we should move them to immediately to the left of the refresh button instead of as a separate group at the beginning of the toolbar. This also puts core content-focused toolbar actions first in the df editor, as is generally expected and done in the other toolbars.
Thanks!
Specifically: * Move "Insert before" to be before "Insert after" in the dataframe editor. * Move "Refresh" to the right in the collections editor, just before the options menu. * Move "Search", "Filter" and "Refresh" in the Variable Explorer pane to be between the spinner and the options menu. * Split the row and column actions in the dataframe editor. * Move "Resize rows" and "Resize columns" to the left of "Refresh" in the array, collections and dataframe editors.
|
Thanks @CAM-Gerlach for your further comments. I implemented them and updated the screenshots at the top of this PR.
Done, under the assumption that you meant that
Done.
Yes, I noticed that as well and I was planning to ask you about it, but I forgot, so I'm glad you picked that up. I did not find a clear way to do this using Spyder's API so I ended up overriding the private
Done.
Done. This does mean that the toolbar in the array editor only has buttons on the right and nothing on the left, but so be it. I did the same move in the collection editor. |
There was a problem hiding this comment.
Great work @jitseniesen!! These improvements look really nice!
My suggestions are directed to improve code style and API usage.
spyder/plugins/variableexplorer/widgets/objectexplorer/objectexplorer.py
Outdated
Show resolved
Hide resolved
|
Hello @jitseniesen! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2024-02-14 12:35:10 UTC |
* Consistently use add_item_to_menu() and add_item_to_toolbar() to add items to menus and toolbars. * Use sections to add separators to menus and toolbars. * Define constants for the names of actions, widgets, etc in enum-like classes. * A few minor improvements to code style.
jitseniesen
force-pushed
the
refresh-followup
branch
from
4e2c559
to
b2cb5c4
Compare
|
@ccordoba12 As requested, the PR is now using add_item_to_menu() and add_item_to_toolbar() from SpyderWidgetMixin to build the menus and toolbars, and it's using sections to add the separators. For consistency, I also "declared" all the names of actions and widgets in enum-style classes like DataframeEditorActions. It does make the code more verbose but I guess that is the price to pay for consistency. I also addressed your other comments. The PR does need a rebase, but I don't want to mess up your review process. So please let me know when you want to rebase. |
There was a problem hiding this comment.
This is a terrific improvement!! Thanks @jitseniesen for your hard work on this!
|
Note: There are some really minor issues about this that I'd like to discuss with @jitseniesen and @CAM-Gerlach, but I'm going to merge because @dalthviz needs this so he can finish the migration of the editor to the new API. So, I'll open a new issue about them. |
This is a follow up after PR spyder-ide#21666 was merged.
Description of Changes
This PR is a follow-up to PR #21312 which added the Refresh button, in order to make the UI and the code in the array editor, collections editor, dataframe editor and object explorer more uniform, as suggested in issue #21556:
After the PR, the editors look as follows:
Here are the corresponding images from before the PR:
Issue(s) Resolved
Fixes #21556
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