Synchronizer plugin vertical zoom ignorance fix #1037
Conversation
|
I had quite a look around this as well, see commit e854c3d and earlier, but this is not the right way forward, unfortunately, as this causes other issues. This also reverts fixes for some other problems. |
|
@mirabilos, null checks code I provided there is not the final fix I had come to. I found it causing bugs and reverted it as you did in commit you linked. I added it to the topic just to explain the path and options it took me while investigating. |
|
Hmm hmmm, so the code I merged then reverted was not the final version somehow? But it introduced a regression? OK, then let’s have a look into this again. I think I need to understand the problem first. The description is… a bit confusing if I haven’t seen it myself, probably. Could you make a minimal example to exhibit the problem? I also don’t understand how/why the fix works and why
The question is, does the change break anything else which you don’t happen to have in your project, too? It’s been two years, so I admit I don’t recall the exact problems… |
|
@mirabilos thanks for looking at this, I will try to explain the problem more precisely, and I have prepared examples for testing The behavior I want to achieve is to make synchronizer affect X-zooming only, and make it not touching other graphs when one of them Y-zoomed I found this statement in
It sounds exactly what was needed, so I used it and found some bugs. Although my fix solves them, I can't say that it don't affects other behaviors (though I didn't met any, but I use synchronizer with these options only), and it may be the wrong way to fix it. So here are the details for tests to reproduce the problem. Examples for testing (I just copied synchronizer.js code directly to JavaScript field): TestsEach test should be performed after full reinitialization (hitting orange Test 1: Result: other graphs Y-zooming state resets. Test 2: Result: previous graph y-zooming resets. Test 3: Performing tests in fixed version shows consistent behavior. I can't explain exactly how I come to this solution, I actually wrote it in first post, but it was so big and complex, so I have deleted it. I just left what was the first source of the problem I have found:
But there is another sources, probably.
Fix consists of two changes, one is changing the array we are comparing in So, in general, what we are trying to achieve - tell synchronizer to not touch other graphs on performing Y-zooming of one of them if |
|
One of possible reasons to use xAxisRange instead of dateWindow could be that it is not initialized at first interaction with graph, but xAxisRange already gives correct result. Maybe need same change for using yAxisRange instead of valueRange, but this problem just invisible because synchronizer don't have option to not sync by x axis, and thus this never happens. May be wrong |
This reverts commit 35f2fae.
|
@mirabilos I took a fresh view on this, and I think I have found right way to fix it The problem was that we just have to pass |
src/extras/synchronizer.js
Outdated
| opts.valueRange = me.yAxisRange(); | ||
|
|
||
| var opts = {}; | ||
| opts.dateWindow = me.isZoomed('x') ? me.xAxisRange() : null; |
There was a problem hiding this comment.
Is this null assignment deliberate, or do you mean…
if (me.isZoomed('x'))
opts.dateWindow = me.xAxisRange();
… instead?
There was a problem hiding this comment.
I don't know if this is necessary, it is working in my project without (I have deleted the null assignment line in previous fix to make Test 1 fixed), but now it just keeps previous code:
If graph is not zoomed, it assigns null. And I just added the same for y to make logic for both consistent
var opts = { dateWindow: me.xAxisRange() };
if (!me.isZoomed('x')) opts.dateWindow = null;
This is how it was before, I just combined it to
opts.dateWindow = me.isZoomed('x') ? me.xAxisRange() : null;
There was a problem hiding this comment.
It is just a little optimization actually. It doesn't call xAxisRange() now if it is just going to use null
There was a problem hiding this comment.
Ah right, that was in the original code. Thanks. (It’s late.)
|
I posed a question (if it’s deliberate, may want to add a comment about the way (deliberate reset?), if not, if is better), and I’d prefer to not have the then-statement of an if-block in the same line (and I think you changed the indent of the block as well, Dygraphs mostly uses two spaces for some reason). I’ll be without laptop until the 29th or so and will look at it later. |
|
Fixed coding style |
|
Or should I add |
|
Nah, no curly braces for just one command in a then-block, at least in my book ☻ I actually use KNF, but in this project, a somewhat different style is pre-existing, so I keep to that as I see it. |
|
Oh, right. This should be right then? |
| var opts = { | ||
| dateWindow: me.xAxisRange() | ||
| }; |
There was a problem hiding this comment.
Why this, and why not…
var opts = {};
if (me.isZoomed('x'))
opts.dateWindow = me.xAxisRange();
| var opts = { | ||
| dateWindow: me.xAxisRange() | ||
| }; | ||
| if (syncOpts.range) |
There was a problem hiding this comment.
Similarily, no isZoomed check any more… is this deliberate (and do things still work correctly without them)?
|
@mirabilos I assume you have missed the review I just posted on previous commit? I described all details there. |
|
I admit I have quite a hard time wrapping my head around this one :/ Let’s just trust you on this and merge it and see what breaks, if any, at this point. |
|
@mirabilos thank you! |
|
On Wed, 5 Feb 2025, n-gist wrote:
Ok, thank you. Took a look into #917, it doesn't interferes with
synchronizer. Both PRs makes sense.
Thanks for spending the time to look into it as well.
The project in which I was actively using Dygraphs had
its budget cut, so I’ve not got much testability left,
so more eyes are much welcome.
Thankfully the Firefox issue turned out to be nothing,
so perhaps, after merging these, I could push out a new
minor release, and then tackle the documentation/website
thing (apparently, the version of Twitter-Bootstrap it
uses is so old I got asked by the Debian packagers of it
to stop using it, and I’d rather get rid of Twitter stuff
entirely than upgrade, anyway, but I’m not a “frontend
dev” or CSS expert…).
|
Do you mean to get rid of from When it comes to documentation, I see |
|
On Fri, 7 Feb 2025, n-gist wrote:
Do you mean to get rid of
```
<link rel="stylesheet" type="text/css" href=".jslibs/bootstrap.min.css" />
<script type="text/javascript" src=".jslibs/bootstrap.min.js"></script>
```
from `docs/header.html` and adding CSS code for other htmls which were using it?
Yes, and ideally jQuery as well, though some of the demos(?) use it
still, and they can, unless it’s easy enough to drop.
I am more or less familiar with CSS and I wanted to check it. It
doesn't look like there is a lot to replace, from a first view. But
didn't found how to actually test changes.
There’s a Python3 script that can serve the changed files straight
from the dev directory.
I have installed what is needed to compile dygraphs under Ubuntu using
WSL in Windows, and got `build` and `build-jsonly` npm scripts working.
Installed Chrome in Ubuntu, and it seems like demo htmls from `tests`
folder are working normally.
When it comes to documentation, I see `.jslibs` symbolic links (or what
is it) in VS Code, but when I try to open them I get the error
Yes, these are symlinks to the location of the respective libraries in
a Debian system, when installed system-wide. If you (see DEVELOP.md)
install libjs-bootstrap libjs-jquery libjs-jquery-ui in WSL Debian or
Ubuntu or so, they should work within WSL.
To test the changes, you run docs/ssi_server.py and point the browser
to localhost via “network” (WSL 1, for WSL 2 the IP of the VM, but then,
networking is totally hosed in WSL 2 anyway so you should be using WSL 1
or a real VM if you need networking).
So, it is a bit dark forest for me. I could just copy them from
https://dygraphs.com/.jslibs/ though.
Yes, in theory.
But I wanted to look at their sources, and not minified versions.
There’s /usr/share/javascript/jquery/jquery.js in addition to
/usr/share/javascript/jquery/jquery.min.js (and similar for the
others) in the package, though if you want the actual, split-into-
files, source, do an “apt-get source libjs-jquery” (etc.) (needs
deb-src lines in sources.list) or (needs devscripts installed)
“dget http://deb.debian.org/debian/pool/main/n/node-jquery/node-jquery_3.6.1+dfsg+~3.5.14-1.dsc”
etc. (via https://packages.debian.org/sid/libjs-jquery then the
sources URL).
So, I was thinking to just remove bootstrap ccs and js, and compare how
docs are looking. Find out what looks broken and fix it by copy/mimic
CSS inside `css/dygraph.css` or add it to separate css file.
Possible, but I don’t think they use much of it, so I’d probably
opt for doing a simple enough layout myself. Wouldn’t be the first
time, though I need to check on mobile devices as well these days,
which I haven’t experience with.
Incidentally, I’d take this to get said experience, so, unless
you happen to be an expert in grid layout things and so, which,
from your message, you also don’t seem to be, don’t worry about
it and I’ll do it sometime.
Seems like it needs a configured web server running to be able to open docs.
For the fully built project, yes, but see above for dev testing.
|
|
@mirabilos thanks for so much detailed answer, I was interested to just get everything up to get some experience with things I never did, and this helped me. The problem was that I just forgot about DEVELOP.md file. I saw it, but when I actually decided to try to setup everything, I forgot that I should follow it So, I have configured and got it running, thus I checked for how bootstrap and jQuery can be removed. I did some and will add PR. I was just interested to try and didn't delve into problem areas just in case, so it is ok to close it or continue from it as starting point |
Having several graphs synced, I wanted their vertical zooming to be independent. Setting synchronizer
rangeoption tofalsecurrently does that, but only partially.For the first case I found that the source is
arraysAreEqual(a, b)returningfalsewhile comparing x-axis arrays when both arrays arenull. Since in dygraph null x axis (dateWindow) array means 'fully zoomed out', this should be treaten as equal zooming ranges. Adding a check like thissolved first case, but... [it would take too long to explain, deleted] it was still very buggy.
In short, I figured out that other arrays should be used when comparing. Using
xAxisRange()instead ofgetOption('dateWindow')for that gives consistent expected result, so here's the fix.The only issue I found after, is that it still resets graphs sometimes after x-zooming in and out due to precision error. For example, in my case, graphs had the ranges for x-axis equal to
[1.5e-7, 0.00015]and after x-zooming in and out for one of them it changes to[1.5e-7, 0.00015000000000000001]. Thus, comparing the ranges after that, it founds them are different, resetting graphs as a result. It would be more right to fix this somewhere inside zooming handler in dygraph core.