-
Notifications
You must be signed in to change notification settings - Fork 35
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
Sync view state between JS and Python #448
Conversation
src/state.ts
Outdated
const debouncedModelSaveViewState = debounce((model: AnyModel) => { | ||
const viewState = model.get("_view_state"); | ||
|
||
// transitionInterpolator is sometimes a key in the view state while panning | ||
// This is a function object and so can't be serialized via JSON. | ||
// | ||
// In the future anywidget may support custom serializers for sending data | ||
// back from JS to Python. Until then, we need to clean the object ourselves. | ||
// Because this is in a debounce it shouldn't often mess with deck's internal | ||
// transition state it expects, because hopefully the transition will have | ||
// finished in the 300ms that the user has stopped panning. | ||
if ("transitionInterpolator" in viewState) { | ||
console.debug("Deleting transitionInterpolator!"); | ||
delete viewState.transitionInterpolator; | ||
model.set("_view_state", viewState); | ||
} | ||
|
||
model.save_changes(); | ||
}, 300); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this is very tied to view state, so maybe we shouldn't name the below function useModelStateDebounced
I started prototyping using an evented dict to hold the view state, but I'm going to punt on that for now. Here was the code I attempted with: def serialize_view_state(data: EventedDict, obj):
return json.dumps(data._dict)
class ViewStateTrait(FixedErrorTraitType):
default_value = EventedDict()
def __init__(
self: TraitType,
*args,
**kwargs: Any,
) -> None:
super().__init__(*args, **kwargs)
self.tag(sync=True, to_json=serialize_view_state)
def validate(self, obj, value):
# TODO: validate view state keys
evented_dict = EventedDict(value)
@evented_dict.events.connect
def send_state():
obj.send_state("view_state")
return evented_dict |
I realized a good way to ensure the user always assigns a new object instead of mutating a dict is to not use a dict! The latest commit coerces the dict to an immutable named tuple, which forces the creation of a new object. I'm happy with this API now. |
Closes #112
In #444 @batpad mentioned
While playing around with that PR, I also saw some view state lagging at low zooms with lots of data rendered. In this screencast I'm smoothly zooming in, but the rendering is very jagged, and sometimes zooms back out slightly. This screencast is of the previous PR
Screen.Recording.2024-04-01.at.11.23.14.AM.mov
My hypothesis of what's happening is that when rendering lots of data there's a slight lag for deck to call the
onViewStateChange
handler. And so deck will occasionally call the handler with "old" data and that's where you sometimes get the "snap back" in the scrolling.I think this is a really bad UX, and it seems not to happen when deck manages the view state internally. So I think potentially the best UX and performance solution is to decouple the view state from the model, while still syncing from deck to Python in the debounced model handler.
This PR tries to implement this.