Skip to content

Fix no-show window issue and refactor cocoa webkit#1271

Merged
SteffenL merged 7 commits into
webview:masterfrom
citkane:refactor-cocoa_webkit2
Mar 26, 2025
Merged

Fix no-show window issue and refactor cocoa webkit#1271
SteffenL merged 7 commits into
webview:masterfrom
citkane:refactor-cocoa_webkit2

Conversation

@citkane

@citkane citkane commented Mar 24, 2025

Copy link
Copy Markdown
Contributor

This PR:

  • Fixes an issue where the window was not showing if the user did not set a size
  • Refactors cocoa_webkit to a more standardised backend code flow

@citkane citkane changed the title Fix and refactor cocoa webkit2 Fix no-show window issue and refactor cocoa webkit2 Mar 24, 2025
@citkane citkane changed the title Fix no-show window issue and refactor cocoa webkit2 Fix no-show window issue and refactor cocoa webkit Mar 24, 2025
@SteffenL SteffenL added the type: fix Pull request is a fix label Mar 24, 2025
Comment thread core/include/webview/detail/backends/cocoa_webkit.hh Outdated
Comment thread core/include/webview/detail/backends/cocoa_webkit.hh Outdated
}

set_up_window();
window_init();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In the constructor, I can easily understand that window_init(window) means that we're going to perform window setup/initialization, potentially with an existing window.

What does it mean when window_init() is called again from on_application_did_finish_launching() without a parameter (= nullptr)?

@citkane citkane Mar 25, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It means that by the time on_application_did_finish_launching is called (ie. the delegate is ready), m_window = static_cast<id>(window) was already set, so the window parameter is moot (ie. there is no further reference to window in window_init). Passing nullptr is thus rational.

If the the user DID pass a window into the class constructor -> window_init(window), !m_window will be false on the second call to window_init, so m_window is guarded from being reset.

@SteffenL

Copy link
Copy Markdown
Collaborator

Thanks for the latest commits. The flow is clearer to me.
Do however remember to account for multiple instances of the webview class, so I suggest the following edit:

diff --git a/core/include/webview/detail/backends/cocoa_webkit.hh b/core/include/webview/detail/backends/cocoa_webkit.hh
index 63181e15..28d46feb 100755
--- a/core/include/webview/detail/backends/cocoa_webkit.hh
+++ b/core/include/webview/detail/backends/cocoa_webkit.hh
@@ -605,6 +605,13 @@ private:
       return;
     }
 
+    // Skip application setup if this isn't the first instance of this class
+    // because the launch event is only sent once.
+    if (!get_and_set_is_first_instance()) {
+      window_init_proceed();
+      return;
+    }
+
     m_app_delegate = create_app_delegate();
     objc_setAssociatedObject(m_app_delegate, "webview", (id)this,
                              OBJC_ASSOCIATION_ASSIGN);
@@ -615,11 +622,7 @@ private:
     // loop has started in order to perform further initialization.
     // We need to return from this constructor so this run loop is only
     // temporary.
-    // Skip the main loop if this isn't the first instance of this class
-    // because the launch event is only sent once.
-    if (get_and_set_is_first_instance()) {
-      objc::msg_send<void>(m_app, "run"_sel);
-    }
+    objc::msg_send<void>(m_app, "run"_sel);
   }
   void window_init_proceed() {
     objc::autoreleasepool arp;

@SteffenL

Copy link
Copy Markdown
Collaborator

A note for later: The get_and_set_is_first_instance() could be a good candidate to move into engine_base.

@citkane

citkane commented Mar 26, 2025

Copy link
Copy Markdown
Contributor Author

Thanks, see latest commit for your suggested change.

@SteffenL SteffenL left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This looks good to me now. Thanks for your work!

@SteffenL SteffenL merged commit 05dd816 into webview:master Mar 26, 2025
@citkane citkane deleted the refactor-cocoa_webkit2 branch March 26, 2025 17:19
@citkane

citkane commented Mar 26, 2025

Copy link
Copy Markdown
Contributor Author

This looks good to me now. Thanks for your work!

And thank-you @SteffenL for your work!
EPIC!

See #1272 for a tidy up.

A note for later: The get_and_set_is_first_instance() could be a good candidate to move into engine_base.

Not sure if I agree...
get_and_set_is_first_instance() is completely unique to Mac, and decoupled from anything else in engine_base. I will leave it to your discretion, so I have not included this in #1272

@SteffenL

Copy link
Copy Markdown
Collaborator

Thanks!

Not sure if I agree...

Don't worry, that isn't something we have to decide on now, and certainly isn't something I expect you to take care of. 😄

I can however give you a few concrete examples where we have application-wide things that we likely could skip if we had get_and_set_is_first_instance() in engine_base:

Windows:

  • is_webview2_available()
  • m_com_init = {COINIT_APARTMENTTHREADED}
  • enable_dpi_awareness()

Linux:

  • gtk_compat::init_check()
  • webkit_dmabuf::apply_webkit_dmabuf_workaround()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: fix Pull request is a fix

Development

Successfully merging this pull request may close these issues.

Ability to set initial size of the window

2 participants