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

fix(core): sync windows metadata across all windows, closes #5571 #5615

Merged
merged 7 commits into from
Dec 27, 2022

Conversation

amrbashir
Copy link
Member

@amrbashir amrbashir commented Nov 12, 2022

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Docs
  • New Binding issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes, and the changes were approved in issue #___
  • No

Checklist

  • When resolving issues, they are referenced in the PR's title (e.g fix: remove a typo, closes #___, #___)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.
  • I have added a convincing reason for adding this feature, if necessary

Other information

@amrbashir amrbashir requested a review from a team as a code owner November 12, 2022 16:38
FabianLars
FabianLars previously approved these changes Dec 14, 2022
Copy link
Member

@FabianLars FabianLars left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on Windows.

@lucasfernog
Copy link
Member

Unfortunately this does not fix the sync issue completely. The main issue is that we use an initialization script to set the windows metadata (the init script is static and the metadata is dynamic). I started working on a fix for this when the issue was raised, I'll check if it doesn't introduce a breaking change (last time I thought it would).

An easy way to test is run the API example, create a second window, close the main window, and then reload the page of the second window. The metadata will contain the main label.

@lucasfernog
Copy link
Member

Ahh I remembered why I didn't finish my own work, it uses the custom protocol to inject the metadata so it needs the dev server proxy to work on dev :(

@lucasfernog
Copy link
Member

Let's try this solution for now then :(

@FabianLars
Copy link
Member

FabianLars commented Dec 14, 2022

Disabling page reloads is a common request anyway :D

Can't we use the normal IPC instead of a custom protocol?

@lucasfernog
Copy link
Member

We can but if we use it, there'll be a small delay where you'll see a wrong list of labels.

This reverts commit d2c6765.
@lucasfernog
Copy link
Member

Works on Windows, but it doesn't work on Linux. It seems like the first eval() call is lost on Linux, probably because the window isn't loaded yet. (i'll merge this anyway if we can't figure it out, it might be some webkit2gtk race condition).

@lucasfernog
Copy link
Member

Ok it gives me this error: Err(Error { domain: WebKitJavascriptError, code: 699, message: "undefined:1:40: TypeError: undefined is not an object (evaluating 'window.__TAURI_METADATA__.__windows = [\"A\",\"B\"].map(function (label) { return { label: label } })')" }).

It makes sense, but if I change the script to set window.x = 5, the variable isn't there when the app loads.

@lucasfernog
Copy link
Member

I'll push a possible wry fix and let the wry gods decide.

lucasfernog added a commit to tauri-apps/wry that referenced this pull request Dec 14, 2022
When evaluating scripts immediately after creating a webview on Linux, the script runs before the page is ready, so it gets lost or fails to execute. This commit changes it to be similar to other platforms, where the evaluated scripts are executed after the initialization scripts. This issue was raised in tauri-apps/tauri#5615
@lucasfernog
Copy link
Member

lucasfernog commented Dec 14, 2022

Aaand I had to do the same on macOS. Maybe this is more workarounds than we should have? cc @amrbashir @wusyong (see tauri-apps/wry#815).

@amrbashir
Copy link
Member Author

I think it is fine to queue the eval() scripts until the page loads as most users will expect full access to the window object.

wusyong pushed a commit to tauri-apps/wry that referenced this pull request Dec 15, 2022
* fix(linux): race condition on script eval

When evaluating scripts immediately after creating a webview on Linux, the script runs before the page is ready, so it gets lost or fails to execute. This commit changes it to be similar to other platforms, where the evaluated scripts are executed after the initialization scripts. This issue was raised in tauri-apps/tauri#5615

* do the same on macOS
@lucasfernog lucasfernog merged commit 146a794 into dev Dec 27, 2022
@lucasfernog lucasfernog deleted the sync-windows-metadata branch December 27, 2022 15:19
luoffei pushed a commit to luoffei/tauri that referenced this pull request Dec 29, 2022
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