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

Enable Live-Edits of any Property of any Plot #837

Merged
merged 6 commits into from
Mar 22, 2022

Conversation

da-h
Copy link
Contributor

@da-h da-h commented Mar 4, 2022

Description

Adding possibility to change all available plotpanes (e.g. axis-limits) on-the-fly.
The feature added can be most easily explained by the short video added below.

Motivation and Context

Pretty often I find myself restarting a script because a specific setting of a plot cannot be adapted easily in visdom.
This feature tries to solve this by adding the option to edit all pane.content variables on-the-fly.

Some more details on the implementation:

  • to make it easier to add this feature, i have reused functionality of PropertiesPane by adding a AbstractPropertiesList Component.
  • The PropertyList is added to the general Pane. I see that one could argue for a more specific implementation, possibly addit it only to PlotPane or so, but I felt that this feature could be expanded to a more general feature, usable by other types of panes as well.
  • To enable this feature on a per-Pane-object basis i have added the option enablePropertyList, and PlotPane is using it to enable the feature in all plots.
  • All simple data (boolean, ints, strings, and nested simple data) are shown in the list when clicking the new editParameters-Button in the top-right corner of every plot. The button turns red when enabled and closes the window again when pressed a second time.

How Has This Been Tested?

I have to admit that I have tried some, but not all parameters used in demo.py.

Screenshots (if appropriate):

record

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
    -> This is just a proposal for a place to mention this feature. I have used the previous video and placed it in the README.md where it fitted best in my opinion. AFAIK upload from PRs cannot be removed and will be accessible even after closing the PR.
  • For JavaScript changes, I have re-generated the minified JavaScript code.

@da-h da-h changed the title Enable Live-Edit of any Property of any Plot Enable Live-Edits of any Property of any Plot Mar 4, 2022
@JackUrb JackUrb self-requested a review March 4, 2022 20:55
Copy link
Contributor

@JackUrb JackUrb left a comment

Choose a reason for hiding this comment

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

This PR is really cool! As with all cool features at this scale though, I have a few questions/concerns to discuss before moving forward with it. Overall I think the choice to reuse the properties list was really clever, but there's a little work to be done for proper state/props management here. (I think in some ways you're being held back by the component-style react that Visdom has been built on, as it can make state maintenance harder to follow, and updates less clear.) In any case we can start the discussion primarily around the direct changes to props.content in Pane.js.

js/AbstractPropertiesList.js Outdated Show resolved Hide resolved
js/AbstractPropertiesList.js Show resolved Hide resolved
js/Pane.js Show resolved Hide resolved
@da-h
Copy link
Contributor Author

da-h commented Mar 21, 2022

One additional thing:

  • the last change switched the properties list to be shown inside the pane (overflow-y: scroll), instead of overlapping the lower border. I found this fits the current project state better and also is needed as in the multiple-windows-case, some windows could overlap the open list of another window.

@da-h da-h marked this pull request as draft March 21, 2022 07:50
@da-h
Copy link
Contributor Author

da-h commented Mar 21, 2022

Final Note:

  • I found some minor bugs due to missing integrity tests of the objects to create property lists from. I have added some minor checks before passing the objects to AbstractPropertiesList in the last push.

@da-h da-h marked this pull request as ready for review March 21, 2022 11:57
@da-h da-h mentioned this pull request Mar 21, 2022
7 tasks
Copy link
Contributor

@JackUrb JackUrb left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes, and for the overall feature here! This is a really cool addition to Visdom and I'm sure it'll see use 👍

js/AbstractPropertiesList.js Show resolved Hide resolved
js/Pane.js Show resolved Hide resolved
@JackUrb JackUrb merged commit 4720769 into fossasia:master Mar 22, 2022
@da-h da-h mentioned this pull request May 4, 2022
6 tasks
@da-h da-h deleted the edit-button branch May 10, 2022 20:27
@da-h da-h mentioned this pull request May 29, 2022
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants