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

Create add_event_listener function [gtk] #659

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

dandeto
Copy link
Collaborator

@dandeto dandeto commented Feb 7, 2022

This is a simple hook into the g_signal_connect function for gtk. The programmer can pass in an event name and a callback to handle a signal emitted from the gtk window, or the webview. Before this commit, the only way to handle an event would be to hack each handler into the gtk_webkit_engine class itself.

Here is a simple example of two callbacks:

void load_changed_callback(WebKitWebView *web_view, WebKitLoadEvent load_event, gpointer user_data) {
  switch (load_event) {
  case WEBKIT_LOAD_STARTED:
      std::cout << "Load Started URI: " << webkit_web_view_get_uri(web_view) << '\n';
      break;
  case WEBKIT_LOAD_FINISHED:
      std::cout << "Get Finished URI: " << webkit_web_view_get_uri(web_view) << '\n';
      break;
  }
}

void destroy_window_callback(GtkWidget* widget, GtkWidget* window) {
  cout << "window destroyed\n";
}

We can add these event handlers like so:

w.add_event_listener<void>("load-changed", load_changed_callback);
w.add_event_listener<void>("destroy", destroy_window_callback);

Different function signatures? No problem! add_event_listener is generic.

All event handlers have to be platform-dependent - I don't see any way around that. The load_changed_callback I wrote as an example will not work for whatever the equivalent event is on MacOS or Windows. The only way to make these event handlers cross-platform is to define a few for the user to choose from, write them for each platform, and call it a day. Since this library does not expose any events for the user to handle at all, I thought this was a good starting point.

If I get positive feedback on this, I will read up on how I can port it to Windows. I'm happy to answer any questions.

This is a simple hook into the g_signal_connect function for gtk. The programmer can pass in an event name and a callback. Now I need to see if I can make this cross-platform...
@dandeto
Copy link
Collaborator Author

dandeto commented Feb 7, 2022

Am I supposed to manually change
T add_event_listener(const char* event, void (*f)(Types...))
to
T add_event_listener(const char *event, void (*f)(Types...))
for this to be accepted?

@justjosias
Copy link
Member

@dandeto Yes, clang-format tests have to pass for it to be accepted, which involved either manually formatting the file, or using clang-format to do it automatically on your computer.

@dandeto
Copy link
Collaborator Author

dandeto commented Feb 8, 2022

@justjosias Thank you for letting me know. I fixed the error, so it should pass the workflow now.

@justjosias
Copy link
Member

I think we want to have as much as possible available cross-platform. That way someone isn't writing platform-specific code and expecting it to work on multiple platforms. I like the idea here (but I don't have the final say). Is there theoretically a way to abstract over it in a way that works consistently for all platforms? I'm not so sure about exposing platform-specific APIs.

@dandeto
Copy link
Collaborator Author

dandeto commented Feb 8, 2022

I'm on the same page as you, here.
If you read through the documentation for each webview in this header, it becomes clear that they have vastly different APIs, and to top it off, we have to write in Objective-C for MacOs! There are many events that gtk-webkit has that are not shared in Chromium or webkit on Mac. That really surprised me. They're all webkit ports, yet they don't have the same api.

Let's take a page load event for example. You can see my initial post for how it looks in gtk-webkit. What is one function in gtk-webkit are broken into four separate functions for mac and five on windows:

gtk:

load_changed_callback(WebKitWebView *web_view, WebKitLoadEvent load_event, gpointer user_data)
// must call gtk-specific function to get the url. load_event tells us what load state we are in

Mac:

func webView(WKWebView, didStartProvisionalNavigation: WKNavigation!)
// This tells delegate that navigation started. Idk how to get the url

Windows webview2:

[](ICoreWebView2* webview, ICoreWebView2NavigationStartingEventArgs * args) -> HRESULT {
      // Use the args pointer to get the uri
      PWSTR uri;
      args->get_Uri(&uri);
      ...
   }).Get();

Each of these APIs are decidedly incompatible. You can't write one callback and expect it to handle the same event on different platforms.

Here is a two-pronged approach of how I think we can best solve this problem:

  1. Write code to wrap each specific event that we want to support.
  2. Expose all events to the user so they can write platform-specific code if they desire.

I've started here with the latter. I think we should create our own APIs to handle events cross platform, but that will take time and quite honestly will probably bloat our library. If you think that's a good idea, I would certainly like to work on it some. In the meantime, we do not expose the event systems to our users at all, which I think is a clear detriment. And even if we had a lot of common events covered, that won't stop someone from needing to write platform-specific code until our library supports literally everything with the same level of granularity. There is a time and place for platform-specific code... though not often :)
What are your thoughts? And thanks for reading this long post.

@paulocoghi
Copy link
Contributor

paulocoghi commented Feb 12, 2022

  1. Write code to wrap each specific event that we want to support.
  2. Expose all events to the user so they can write platform-specific code if they desire.

For simplicity reasons, I agree with Dandeto's approach # 2, that is to expose all specific events to the user so they can write platform-specific code if they desire.

My reasoning is that this can be used as progressive approach to achieve # 1, because it makes a lot easier to write a final agnostic OS wrapper later, after each of the OS dependent wrappers are ready.

Also, it helps receiving contributions from different developers on each different OS platform, again, to achieve # 1.

Finally, the proposed specific events don't need to be published as final, but as "alpha" and susceptible of changes, avoiding the project obligation to respect these functions "contracts" in the future, as well as allowing us to adapt them as needed, until we finally achieve # 1.

@paulocoghi
Copy link
Contributor

paulocoghi commented Feb 12, 2022

For the reasons explained above, I vote to merge this.

Adding tests for the new function on webview_test.cc would be good, but not strictly necessary on this stage, on my point of view.

Naturally, when considered "stable", adding tests for it will be important.

@dandeto dandeto mentioned this pull request Feb 26, 2022
@dandeto dandeto linked an issue Jun 17, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Web Event Callbacks?
3 participants