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

fix: cease autoplay when maidr is deactivated (outside the plot) #526

Merged
merged 3 commits into from
Aug 27, 2024

Conversation

krishnanand5
Copy link
Collaborator

@krishnanand5 krishnanand5 commented Aug 22, 2024

Pull Request

Description

This Pull request aims to address a background issue where autoplay continues to execute even after maidr has been deactivated on the plot by a user click outside the plot area. Changes made in this pull request check for availability of plotData once maidr has been deactivated to handle exceptions thrown by autoplay() gracefully.

Related Issues

This Pull request aims to solve #522

Changes Made

Autoplay() previously did not consider the state of plot.plotData when trying to access the plot components. Should the user click outside the interactive plot area, maidr would be deactivated which would result in destruction of plot object. Hence, Autoplay() would be running in the background and attempt to access the plot object components which were not present and therefore would emit exceptions incessantly.

With the Heatmap, addressing the Autoplay() on inactive maidr is not sufficient. The heatmap itself needs to be prevented from accessing points once autoplay has ended and additionally, its related audio functions need to be handled should the plot data be empty.

Checklist

  • I have read the Contributor Guidelines.
  • I have performed a self-review of my own code and ensured it follows the project's coding standards.
  • I have tested the changes locally following ManualTestingProcess.md, and all tests related to this pull request pass.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation, if applicable.
  • I have added appropriate unit tests, if applicable.

Additional Notes

This PR addresses ghost autoplay issues with Histogram, Scatterplot and Heatmap.

@krishnanand5 krishnanand5 requested a review from ellvix August 22, 2024 02:04
@jooyoungseo
Copy link
Member

@ellvix It looks good to me. Please share your insight. One naive question: Why do we have to apply this fix to each of the plot types? Could we refactor our code to modularize this?

@jooyoungseo jooyoungseo reopened this Aug 22, 2024
@jooyoungseo
Copy link
Member

@krishnanand5 Please go ahead with this solution and apply to the other plots. Will refactor later if needed later.

@jooyoungseo jooyoungseo reopened this Aug 26, 2024
@krishnanand5 krishnanand5 marked this pull request as ready for review August 27, 2024 00:15
@krishnanand5
Copy link
Collaborator Author

Greetings Professor,

I have pushed some changes that help to address the ghost autoplay issue on Scatterplot and Heatmap. Heatmap in particular, required some additional changes which were a result of handling the case of empty plot data on the Autoplay() function. I felt those collateral issues could be handled through this PR instead of creating fresh issues.

Standard barplots do not face this issue and hence no changes were required.

Regards,
Krishna Anandan Ganesan.

@jooyoungseo
Copy link
Member

Thanks, @krishnanand5. Have you tested the boxplot and histogram as well?

Copy link
Member

@jooyoungseo jooyoungseo left a comment

Choose a reason for hiding this comment

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

LGTM! Merge once you are ready.

@krishnanand5
Copy link
Collaborator Author

Greetings Professor,

Histogram was addressed as part of the draft PR. As far as boxplot is concerned, I experienced issues with the in-repo example where the components were not getting highlighted as I was traversing through the plot.

I did not see the exceptions being thrown like how it was for the other plots. Having said that, I would like to investigate boxplot a bit further and address any issues I unearth separately.

Regards,
Krishna Anandan Ganesan.

@jooyoungseo
Copy link
Member

@krishnanand5 sounds like a plan! Thanks for your detailed description 👍

@krishnanand5 krishnanand5 removed the request for review from ellvix August 27, 2024 18:26
@krishnanand5 krishnanand5 merged commit dd564ec into xability:main Aug 27, 2024
1 check passed
github-actions bot pushed a commit that referenced this pull request Aug 28, 2024
## [2.14.6](v2.14.5...v2.14.6) (2024-08-28)

### Bug Fixes

* cease autoplay when maidr is deactivated (outside the plot) ([#526](#526)) ([dd564ec](dd564ec))
Copy link
Contributor

🎉 This PR is included in version 2.14.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants