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

Editor Bug fix: Auto reload after language changing #30363

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

pkpedram
Copy link
Contributor

When language is changed user must reload the page manually.

I know this is not the best solution for this purpose but it fixes the bug for now.

I will work on a better solution later.

Please consult me for the better solution (like should I add a new signal or etc...)

Screencast.from.2025-01-20.12-00-34.webm

Mugen87
Mugen87 previously approved these changes Jan 20, 2025
@Mugen87 Mugen87 added this to the r173 milestone Jan 20, 2025
@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 20, 2025

Please consult me for the better solution (like should I add a new signal or etc...)

I think this approach is totally fine. If we would need to trigger reloads in multiple locations, using a separate signal might be more clean. But for now this is good enough.

@pkpedram
Copy link
Contributor Author

pkpedram commented Jan 20, 2025

Please consult me for the better solution (like should I add a new signal or etc...)

I think this approach is totally fine. If we would need to trigger reloads in multiple locations, using a separate signal might be more clean. But for now this is good enough.

thank you so much ❤️
but I was thinking about a solution to not reload the page but to rerender the texts. like changing the shortcuts in setting works instantly but the language changing action doesn't
I tried this:

editor.config.setKey( 'language', value );
editor.strings = new Strings( editor.config );

it updates the strings class in editor but doesn't update the dom

I think I must dig deeper.

any ideas?

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 20, 2025

I think we would need a new signal that refreshes the entire UI which I don't think is worth implementing. Changing the app's language is normally a one-time operation per workstation so a reload shouldn't be an issue.

@pkpedram
Copy link
Contributor Author

I think we would need a new signal that refreshes the entire UI which I don't think is worth implementing. Changing the app's language is normally a one-time operation per workstation so a reload shouldn't be an issue.

Seems fair. thank you

@ycw
Copy link
Contributor

ycw commented Jan 22, 2025

editor should autosave once so that even if users disabled autosave they won't lose their current editing after full page reload

@pkpedram
Copy link
Contributor Author

editor should autosave once so that even if users disabled autosave they won't lose their current editing after full page reload

good point I will work on it ASAP

@ycw
Copy link
Contributor

ycw commented Jan 22, 2025

This is how editor save to IndexedDB: editor.storage.set( editor.toJSON() ), but it isn't a blocking operation, so we may update Storage#set to accept onSaved callback in the 2nd parameter, and then on UI language changed ... :

// save to IndexedDB before reloading
editor.storage.set( editor.toJSON(), () => window.location.reload() )

@pkpedram
Copy link
Contributor Author

This is how editor save to IndexedDB: editor.storage.set( editor.toJSON() ), but it isn't a blocking operation, so we may update Storage#set to accept onSaved callback in the 2nd parameter, and then on UI language changed ... :

// save to IndexedDB before reloading
editor.storage.set( editor.toJSON(), () => window.location.reload() )

how about i work on that matter and in next step i add it to this bug fix?

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 23, 2025

@ycw Would the following work instead? Just check if autosave is enabled and if so perform a reload. The config settings can be checked via:

editor.config.getKey( 'autosave' ) === true

In this way we avoid the issue mentioned in #30363 (comment) and users can save/reload by themselves.

@ycw
Copy link
Contributor

ycw commented Jan 23, 2025

To best of my knowledge editor saves to IndexedDB is scheduled using setTimeout, where IndexedDB API is not blocking rather using callbacks. If full-page reload e.g. location.reload performs immediately it won't await setTimeout nor callbacks, so I should correct my comment #30363 (comment), no matter config of "autosave" is enabled or not, we should always save once before reloading. FWIW, for a non-simple scene, each autosave action may takes few seconds, most likely editor is still waiting for save upon page-reload in this case.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 24, 2025

Um, maybe we should avoid the reload altogether and force a refresh of the entire UI. I don't like the side effects on the save mechanism of the current approach.

@Mugen87 Mugen87 dismissed their stale review January 24, 2025 09:08

Different approach required.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 24, 2025

Alternatively, how about adding a info message next to the language selection that says changing the language requires a restart of the editor?

@pkpedram
Copy link
Contributor Author

Um, maybe we should avoid the reload altogether and force a refresh of the entire UI. I don't like the side effects on the save mechanism of the current approach.

my idea was that reloading is supposed to be a temp solution and then work on a UI rerender signal. I am working on it.

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

Successfully merging this pull request may close these issues.

3 participants