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 Refresh button to editors from Variable Explorer #21312

Merged
merged 16 commits into from
Nov 25, 2023

Conversation

jitseniesen
Copy link
Member

@jitseniesen jitseniesen commented Sep 7, 2023

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 adds a Refresh button to the array editor, the dataframe editor, the collection editor and the object explorer.

Clicking the Refresh button in the array editor refreshes the editor with the current value of the array variable:

refresh-std.mp4

The same for the dataframe editor:

refresh-dataframe.mp4

The same for the collection editor:

refresh-collection.mp4

The same of the object explorer:

refresh-object.mp4

It is possible to refresh editors opened from the Variable Explorer or editors opened from other editors, so the user can for instance refresh an array nested inside a dictionary inside an object inside another object inside a list:

refresh-nested.mp4

However, you can only refresh editors which ultimately originate from the Variable Explorer. For instance, you can open a collection editor from the IPython Console to display the contents of sys.path, but the Refresh button on that editor is disabled:

refresh-disabled.mp4

If the user changes a value in the editor and then refreshes, Spyder asks for confirmation:

refresh-dirty.mp4

If the new value can't be displayed in the array editor, for instance because the variable now contains an integer, then Spyder shows an error message and closes the array editor:

refresh-invalid.mp4

The same happens if the variable no longer exists:

refresh-deleted.mp4

Note that in this case, the console displays an exception generated inside spyder-kernels. This is existing behaviour, the same happens in the current master if you open a variable in the Variable Explorer that no longer exists.

In the array editor, the UI changes depending on whether the array is a standard 2d array, a record array, a masked array or a 3d array. Refreshing the editor changes the UI if necessary:

refresh-to-3d.mp4

However, if the variable changes from an array to a dataframe, for instance, then the array editor will not transform into a dataframe editor. Instead, an informative message is displayed and the editor is closed, just as above where the array variable changed to an integer.

As part of this PR, I split the function which sets up the editors in two: one function for setting up the UI and one function for setting the value to be displayed. The first function is called only one, the second function is called when the editor is opened and when it is refreshed. I implemented the refresh functionality by passing another parameter, data_function, to the editors. Calling this function should return the current value of the variable; if data_function is None then the editor can't be refreshed.

The four editors (array, dataframe, collection and object) are all implemented differently, so the details of how to implement the refresh is also different in each case.

This PR removes the options xlabels and ylabels in the Array Editor. These can be used to change the column and row labels in the editor, but they are not currently used in Spyder. These options were added by Pierre in 2011. I could not find where they were used in the past, if they ever were, so I don't know their intended use. I don't know what to do when refreshing means that the size of the array changes, because then the number of labels also needs to change, so I chose to remove the options.

Issue(s) Resolved

Fixes #6325

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

@jitseniesen
Copy link
Member Author

The current plan is to add a toolbar (as suggested in #21315) and then have this be a button in the toolbar. This matches the interface for the Variable Explorer.

@ccordoba12 ccordoba12 changed the title Add Refresh button to array editor PR: Add Refresh button to array editor Sep 8, 2023
@pep8speaks
Copy link

pep8speaks commented Oct 31, 2023

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

Line 1638:1: W293 blank line contains whitespace

Line 245:5: E731 do not assign a lambda expression, use a def
Line 265:5: E731 do not assign a lambda expression, use a def
Line 289:5: E731 do not assign a lambda expression, use a def

Comment last updated at 2023-11-25 14:04:23 UTC

@jitseniesen jitseniesen changed the title PR: Add Refresh button to array editor PR: Add Refresh button to editors from Variable Explorer Oct 31, 2023
@jitseniesen
Copy link
Member Author

I extended this a fair bit. Now it is possible to refresh array editors, dataframe editors, collection editors (for list, set, dict and tuple), and object explorers (for any type that is not handled by another editor). It works for editors opened directly from the variable explorer and also for editors opened from collection and object editors.

The PR conflicts with PR #20546 and PR #21317, which add a toolbar to the dataframe editor and array editor respectively. The plan is to merge the two PRs that add a toolbar first, and then change this PR so that the Refresh button is added to the toolbar.

@ccordoba12
Copy link
Member

I extended this a fair bit. Now it is possible to refresh array editors, dataframe editors, collection editors (for list, set, dict and tuple), and object explorers (for any type that is not handled by another editor).

Great work @jitseniesen! That'd be really nice to have.

The PR conflicts with PR #20546 and PR #21317

Ok, I'll try to review soon those PRs so you can finish this one.

@jitseniesen
Copy link
Member Author

I rebased the PR in order to pick up the addition of the toolbar to the array and dataframe editors (thanks Carlos for the speedy review) and I updated the screenshots in the top comment. This is now ready for review.

@jitseniesen
Copy link
Member Author

I rebased to pick up the CI automatic test fix in PR #21517.

@ccordoba12 ccordoba12 added this to the v6.0alpha3 milestone Nov 13, 2023
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.

Thanks @jitseniesen ! I checked locally this and seems like things are working as expected 👍

Left a single comment regarding the safe_disconnect function definition. Seems like taht function could be worthy to be moved to some utils module for reusage (not only for this PR but seems like there are other places inside the source code from a quick search which could benefit from using it)

Other than that this LGTM 👍

@GitKia1392
Copy link

Can you provide the Videos in mp3 format, MIME format is unstable and unreadable on Windows 11 or Typically Every windows + xp

Appreciated
Kiamehr Esk

@dalthviz
Copy link
Member

Regarding the tests not passing, maybe a rebase on top of the latest master could help @jitseniesen ? Checking the commits history seems like in a recent commit some changes were done to the CI related with test env activation and spyder-kernels: 9c07bc8

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.

Thanks @jitseniesen !

@jitseniesen
Copy link
Member Author

maybe a rebase on top of the latest master could help

@dalthviz Thanks a lot for this suggestion. I had somehow convinced myself that rebasing could not possibly make a difference, so I kept looking for other explanations.

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! I mostly left minor stylistic suggestions and improvements to docstrings for you.

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/plugins/variableexplorer/widgets/arrayeditor.py Outdated Show resolved Hide resolved
spyder/widgets/tests/test_collectioneditor.py Outdated Show resolved Hide resolved
spyder/widgets/tests/test_collectioneditor.py Outdated Show resolved Hide resolved
spyder/widgets/tests/test_collectioneditor.py Outdated Show resolved Hide resolved
spyder/widgets/tests/test_collectioneditor.py Outdated Show resolved Hide resolved
spyder/widgets/tests/test_collectioneditor.py Outdated Show resolved Hide resolved
@ccordoba12
Copy link
Member

Note: This also needs a rebase to get the fixes I did in PR #21553.

This option was added in Spyder 2, but it is not used at the moment
and may never been used. It interferes with the refresh functionality
which is to be added in a following commit.
Make two functions. The first is setup_ui(), which sets up the UI
and should be called only once. The second is set_data_and_check(),
which specifies the data for the editor and can be called repeatedly.
This is to prepare for the refresh functionality in the next commit.
jitseniesen and others added 14 commits November 25, 2023 14:04
* Add a function `CollectionsDelegate.make_data_function()` for retrieving
  the current value when the user refreshes an editor. This function returns
  `None`, indicating that the editor can not be refreshed.
* Override `.make_data_function()` in `RemoteCollectionsDelegate`, so that
  if the function is called in the namespace editor in the variable explorer,
  it retrieves the current value of the variable currently selected in the
  variable explorer.
* When creating a new array editor in `CollectionsDelegate`, pass the
  `data_function` that is created by `make_data_function` to the constructor.
* Add a Refresh button to the toolbar of the array editor. When pressed,
  this calls the new function `.refresh()`.
* This function calls `data_function` that was passed to the constructor and
  displays the return value in the array editor.
* In CollectionDelegate, pass `data_function` when creating a `CollectionEditor`.
  Pass this along to the `CollectionEditorWidget`.
* Add a refresh button to the toolbar of `CollectionEditorWidget`, which raises
  a signal when pressed. Enable this button if `data_function` is set.
* Add a function `CollectionEditor.refresh_editor()` and connect it to the new
  signal. In this function, check whether the user made any edits and if so,
  ask for confirmation first. Call `data_function` and use its return value as
  the new value to be displayed. Handle exceptions thrown by `data_function`.
* Add tests for the new refresh functionality.
* Pass the `data_function` from `CollectionsEditor`, which can retrieve
  new data for the object being edited, via `CollectionsEditorTableView`
  to `CollectionsDelegate`.
* Add `CollectionsDelegate.make_data_function()` for retrieving new data
  for editors opened from the collection editor.
* Catch `IndexError` when calling a data_function, because this can be raised
  if an item in the collection is deleted.
* Add tests for new functionality.
Make two functions. The first is setup_ui(), which sets up the UI
and should be called only once. The second is set_data_and_check(),
which specifies the data for the editor and can be called repeatedly.

This is a refactor to prepare for the implementation of refresh
functionality in the next commit.
* In CollectionDelegate, pass `data_function` when creating a dataframe editor.
  Pass this along to the `DataFrameView`
* Add a refresh action to the `DataFrameView`, which raises a signal when it
  is triggered. Enable this action if `data_function` is set.
* Add a function `DataFrameEditor.refresh_editor()` and connect it to the new
  signal. In this function, check whether the user edited the dataframe and if
  so,  ask for confirmation first. Call `data_function` and use its return value
  as the new value for the dataframe to be displayed. Handle exceptions thrown
  by `data_function`, or when the new value is not a data frame.
* Add tests for the new refresh functionality.
Make new function `ObjectExplorer.set_value()` for setting the object
displayed in the object explorer, and call this from the constructor.
This is a refactor to prepare for implementing the refresh functionality
in the next commit.
* In CollectionDelegate, pass `data_function` when creating an object explorer.
* Add a refresh toolbutton to the object explorer, which calls the new
  function `.refresh_editor()`.
* In the new function, call `data_function` and use its return value as the
  new object to be displayed in the object explorer. Handle exceptions thrown
  by `data_function`.
* Add tests for the new refresh functionality.
* Pass the `data_function` from the object explorer, which can retrieve
  new data for the object being edited, via `ToggleColumnTreeView`
  to `ToggleColumnDelegate`.
* Add `ToggleColumnDelegate.make_data_function()` for retrieving new data
  for editors opened from the object explorer.
* Pass the result of the above function to editors opened from the object
  explorer.
* Add tests for new functionality.
For some reason, this test started to fail on Windows, probably because
of the new menu item `Refresh`. This should fix the test.
* Fix style of refresh button in object explorer
* Mock correct function in test
* Many style fixes

Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>
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 for this terrific improvement!

@ccordoba12 ccordoba12 merged commit 4749cb2 into spyder-ide:master Nov 25, 2023
14 checks passed
@jitseniesen jitseniesen deleted the refresh branch November 25, 2023 15:41
@rpinto73
Copy link

Thank you so much to @jitseniesen and everyone else who worked hard to make this feature a reality!!

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.

Add ability to refresh/update existing Variable Explorer windows to reflect current variable state
6 participants