fossasia / visdom

A flexible tool for creating, organizing, and sharing visualizations of live, rich data. Supports Torch and Numpy.
Apache License 2.0
9.93k stars 1.13k forks source link

Replaced Window Update Check by 'versioning' (previously used 'hashing') #901

Closed da-h closed 1 year ago

da-h commented 1 year ago

Description

This PR

Motivation and Context

(See discussion below).

How Has This Been Tested?

(Just the cypress tests).

Types of changes

Checklist:

da-h commented 1 year ago

~That's interesting: the variable seems to be required for react 17 (tests are failing now). However, I've tested it on react 16 successfully without any errors. As 18 gives some very good changes, like automatic batched processing, I'll suggest to look into this PR once/if visdom runs under react 18.~

It seems there is a bug in the ImagePane or its test, introduced by combining #900 and #902.

da-h commented 1 year ago

With #908 in-place, this PR succeeds all tests now & is thus now ready for review.

da-h commented 1 year ago

I see!! I would have not thought about this. Thanks for clarifying. :+1:

I'll update this the next days to your proposed solution.

da-h commented 1 year ago

I've just implemented the window-message versioning, to replace consistent_pane_copy. With the background-information of how this mechanism worked, I now also understood the reason for the setTimeout inside of the _checkWindow (now called updateWindow) function:

Using the idea of specifying the version of each pane state, we can circumvent the timers:

da-h commented 1 year ago

Just to be sure: This PR does change the server-client-api by replacing the requirement of hashing the window contents. Also, this PR removes the win_hash endpoint.

Personally I think, since this will be part of 1.0.0, incompatible api-changes should be ok to occur(?) Especially, as the new implementation ~should work~ works with older saved environments, i.e. backwards-compatibility is at least data-wise ensured.