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 issue #611 in the drawing demo gallery #953

Merged
merged 2 commits into from
Feb 7, 2023
Merged

Fix issue #611 in the drawing demo gallery #953

merged 2 commits into from
Feb 7, 2023

Conversation

mp035
Copy link

@mp035 mp035 commented Mar 17, 2019

This pull request fixes the broken zoom functionality and console errors in the drawing demo. It simply adds the willDestroyContextMyself attribute, and removes the now unimplemented functions in the defaultInteractionModel. Comments have also been added to explain to users why calls are missing.

Sorry about the double commit, still getting a handle on pull request functionality. 13887ef corrects a problem where synchronizer.js would block the users drawCallback when updating the zoom of connected charts.

@coveralls
Copy link

coveralls commented Mar 17, 2019

Coverage Status

Coverage decreased (-0.06%) to 90.122% when pulling 13887ef on mp035:master into da2a028 on danvk:master.

@mirabilos
Copy link
Collaborator

Is this (and #611 therefore) still pertinent? It doesn’t throw any console warnings for me…

Copy link
Collaborator

@mirabilos mirabilos left a comment

Choose a reason for hiding this comment

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

is the bug still there?

@mp035
Copy link
Author

mp035 commented Feb 7, 2023

Hi, yep, the bug is still there. Select the zoom functionality in https://dygraphs.com/gallery/#g/drawing with the console open, and try a zoom, or move the mouse over the canvas.

@mp035 mp035 requested a review from mirabilos February 7, 2023 01:07
@mirabilos
Copy link
Collaborator

mirabilos commented Feb 7, 2023

@mp035 thank you for both retesting and the reproducer. I think I get what you’re doing here now.

The unrelated synchroniser change however, do you also have a comment/docs/reproducer/background info on that? I’ve no problem just merging this PR all in one, but I want to make sure I don’t break anything that works, at this point… (Debian is almost frozen, and I want to make a v2.2.1 with all those tiny fixes that arose since v2.2.0 before nothing new can go in any more).

@mirabilos mirabilos dismissed their stale review February 7, 2023 09:00

we’re in dialogue

mirabilos added a commit that referenced this pull request Feb 7, 2023
This pull request fixes the broken zoom functionality (issue #611) and
console errors in the drawing demo. It simply adds the
willDestroyContextMyself attribute, and removes the now unimplemented
functions in the defaultInteractionModel. Comments have also been added
to explain to users why calls are missing.

> Hi, yep, the bug is still there. Select the zoom functionality in
> https://dygraphs.com/gallery/#g/drawing with the console open, and try
> a zoom, or move the mouse over the canvas.
@mirabilos mirabilos merged commit ea4773c into danvk:master Feb 7, 2023
@mp035
Copy link
Author

mp035 commented Feb 22, 2023

@mirabilos , sorry, I missed your comment. It has been nearly 4 years since I worked on this so, I am a little fuzzy on the details. I think the issue was that the end-user could not rely on their callback being called on every redraw because sometimes synchronizer.js would block for its own purposes and never propagate to the next callback.

Looking at my own personal codebase, I have not included it so I would reject 13887ef if I were you.

@mirabilos
Copy link
Collaborator

mirabilos commented Feb 22, 2023 via email

@mp035
Copy link
Author

mp035 commented Feb 22, 2023

Thanks for the update, perhaps that's why I didn't pull 13887ef into my personal codebase. There was a massive amount of cursor customization in my application so I may not have needed it.

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.

None yet

3 participants