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

Design WebView state #222

Open
tjcouch-sil opened this issue Jun 7, 2023 · 8 comments
Open

Design WebView state #222

tjcouch-sil opened this issue Jun 7, 2023 · 8 comments

Comments

@tjcouch-sil
Copy link
Member

tjcouch-sil commented Jun 7, 2023

As a user, I want my tabs to stay the same between refreshes and restarts so I can pick up where I left off anytime.

As an extension developer, I want the platform to keep track of a set of data that describes the state of each of my webviews so I can keep the webview the same between sessions and such. I want to be able to set this data when the platform asks for a new webview so I can pull up the webview in whatever state is appropriate for the situation.


Created from #177: Add some way for webviews to keep their own state that papi persists and passes to them when the webview loads. For example, clicking a button can increase a counter, but we want it to persist between refreshes and restarts. This could probably be tied to the webview's id in some way. Maybe a map could be stored in web-view.service.ts.

We could probably make a hook for React webviews that wraps this state in an exact mirror to useState.

This could be similar to VSCode's webview state. They basically just let the webview set state synchronously and send messages in the background to asynchronously persist the updates. We might be able to do it fully synchronously, though, since the renderer shares origin with the iframes.

WebView State design

Settings & State Design Doc

@tjcouch-sil tjcouch-sil converted this from a draft issue Jun 7, 2023
@tjcouch-sil tjcouch-sil added this to the MVP milestone Jun 7, 2023
@tjcouch-sil tjcouch-sil moved this from 🔖 Open to 📋Product Backlog in Paranext Jun 7, 2023
@tjcouch-sil
Copy link
Member Author

One example of webview state would be a list of open texts on a text collection webview

@irahopkinson
Copy link
Contributor

What is the advantage of implementing and using this over local storage or session storage? I guess if the state needs to be saved between devices for a user?

@lyonsil lyonsil modified the milestones: MVP, Tech Demo Jun 9, 2023
@tjcouch-sil
Copy link
Member Author

Honestly I'm not sure localStorage in webviews would persist. They might! Or they might not. If it did persist, I imagine we would need to figure out a way to pass the webview id down to the webview so it can use the id in its localstorage keys (to make sure the different webviews have their own storage).

However, I believe having state be a feature on the webview would allow state to be modified by the web view provider (and, if we so decide, maybe other things as well). The main things I think about here are specifying some state when first loading the web view (e.g. telling the webview which texts to load in its text collection) and making version updates to webview state without forcing the webview itself to support old versions of its state (the provider could transform the data on load to make the code simpler).

@tjcouch-sil tjcouch-sil changed the title Add WebView state Design WebView state Jul 27, 2023
@tjcouch-sil
Copy link
Member Author

"We also want the text in the title bar of the webview to be updated dynamically. Right now I this is just static text." split off to #445

@tjcouch-sil tjcouch-sil modified the milestone: Tech Demo Sep 26, 2023
@tjcouch-sil
Copy link
Member Author

Created simple implementation issue #472. This design is now for expanded functionality if we want it. Like updating the state from the webview provider anytime. Not sure we need that, but this is here.

@tjcouch-sil
Copy link
Member Author

tjcouch-sil commented Oct 12, 2023

As a follow-up to #472 (PR #545), it may be good to rework webview state to be attached directly to the SavedWebViewDefinition. That way, it travels across processes to the web view provider very naturally, and it gets removed immediately when a webview is closed (no need to run a cleanup function).

However, I believe this may require creating a synchronous-access dockLayout variable in the web view service so you can get the dock layout to get the web view state immediately. Should be fine since anytime a web view is accessing its state, the dock layout should exist. But maybe you can get away with doing this without web view service having to touch it.

@lyonsil
Copy link
Member

lyonsil commented Oct 12, 2023

However, I believe this may require creating a synchronous-access dockLayout variable in the web view service so you can get the dock layout to get the web view state immediately. Should be fine since anytime a web view is accessing its state, the dock layout should exist. But maybe you can get away with doing this without web view service having to touch it.

This is the reason I didn't do it this way at least for now. There was a requirement for this API to be synchronous, and the dock layout is stored as a promise with the comment:

Also please do not save this variable out anywhere because it can change, invalidating the old one (see `registerDockLayout`)

We're also a little loose in how we handle some async calls (i.e., we don't always await the results) in this code. For example:

  // Will we ever need to await this? For now, seems like it unnecessarily complicates registering
  // because making this function async would probably be annoying in React
  loadLayout();

In other languages when you start getting loose with multi-threading you start to have weird bugs and problems that are hard to diagnose. I don't have enough experience here to say how well those things translate, but it seemed safer to have a totally separately, synchronous (by design) service to saving/loading state. The code to cleanup old values wasn't that hard to write, and it isn't that long, so it seemed better to go that route than to try to be hacky with async concepts here.

If/when we get to a point where working with the dock layout is safely synchronous, then changing how we store web view state makes sense to me. I would probably not want to change where we store web state until then.

@tjcouch-sil
Copy link
Member Author

Note: I added synchronous access to the dock layout in #445

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 📥 For Consideration
Development

No branches or pull requests

3 participants