dygraphs icon indicating copy to clipboard operation
dygraphs copied to clipboard

Fix issue #611 in the drawing demo gallery

Open mp035 opened this issue 5 years ago • 1 comments

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.

mp035 avatar Mar 17 '19 05:03 mp035

Coverage Status

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

coveralls avatar Mar 17 '19 05:03 coveralls

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

mirabilos avatar Jan 25 '23 04:01 mirabilos

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 avatar Feb 07 '23 01:02 mp035

@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 avatar Feb 07 '23 09:02 mirabilos

@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.~~

mp035 avatar Feb 22 '23 02:02 mp035

Mark Pointing dixit:

@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

Don’t worry, I understand.

@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. I see

I also think it fixed another bug, crosshairs<->synchroniser interop.

@you have merged it anyway, I just didn't want to leave you hanging.

Yes, thank you!

bye, //mirabilos

Solange man keine schmutzigen Tricks macht, und ich meine wirklich schmutzige Tricks, wie bei einer doppelt verketteten Liste beide Pointer XORen und in nur einem Word speichern, funktioniert Boehm ganz hervorragend. -- Andreas Bogk über boehm-gc in d.a.s.r

mirabilos avatar Feb 22 '23 03:02 mirabilos

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.

mp035 avatar Feb 22 '23 03:02 mp035