Allow multiple viewports on Wayland platform #675

psaavedra · 2023-11-24T07:28:53Z

Note: This PR contains 3 commits. This is the more relevant and what includes the actual functionality added by this PR.

Add new functions in the CogShell API for handling multiple Viewports:

cog_shell_viewport_lookup(CogShell *, CogViewportKey)
cog_shell_viewport_new(CogShell *, CogViewportKey)
cog_shell_viewport_remove(CogShell *, CogViewportKey)

Also added the CogShell::create-viewport signal. This signal is emitted when the shell creates a new CogViewport. With handling this signal is possible to customize the required actions to be done during the creation of a new viewport.

This change modifies the cog_shell_get_viewport(). Now it creates a default viewport in order to keep the current behavior. The creation of the default viewport is lazy and it is delayed until the invocation of the cog_shell_get_viewport().

Add a new multiple-stack example program which shows how to attach 2 viewports to the CogShell. Each viewport iterates over a couple of views.

Pre-requisites:

wl: Fix unused variable display warning
wl: Split cog_platform_configure in 2 stages (create and configure)
- Disassociate the configuration respect of the creation of the platform.
wl: Allow platform implementations can provide their own viewport class
- Optionally, platform plug-in implementations can provide their own
  viewport class by overriding the CogPlatform.get_viewport_type method.
wl: Initial empty implementation of CogWlViewport
wl: Move CogWlWindow to the CogWlViewport
wl: Replace the shell and platform CogWlViewport unique reference with a GHashTable
wl: Add default CogWlViewport/CogWlView
wl: Extend CogShell API for add/remove CogViewport

psaavedra · 2023-11-24T07:58:59Z

platform/wayland/cog-platform-wl.c

+    /* get a reference to the hashmap with viewports added to the shell */
+    self->viewports = cog_shell_get_viewports(shell);
+
+    g_signal_connect_swapped(shell, "create-viewport", G_CALLBACK(cog_wl_platform_on_create_viewport), self);


This allows the specific platforms to react to it and run specific steps. Like here https://github.com/Igalia/cog/pull/675/files#diff-7342248ee4ad822cd294d9c8341a52fd10b3d1df4dd2a202ccb926df67d51034R1327 where the WL platform creates the CogWlWindow.

Technically speaking the Wayland plug-in could also track all the viewports by overriding the GObject.constructed and GObject.dispose virtual functions in its CogWlViewport implementation. One reason to do that would be untangling CogPlatform from CogShell, which we will want to do at some point. This being said, I think we can merge this as-is for now and take care of this change later, when we tackle #634.

psaavedra · 2023-11-24T11:20:27Z

examples/viewports.c

@@ -0,0 +1,83 @@
+/*
+ * viewports.c


For a better user experience, I suggest to use this example with Sway or a similar tiling Wayland-based window manager.

aperezdc

I have doubts about using a GHashTable and exposing that to the public CogShell API, because it looks to me that the code could be simpler and more consistent with the rest of the library using an array—please see the comments inline in the review.

If there is some reason I am missing to use the hash table, please let me know and we can chat about it 😺

aperezdc · 2023-11-27T09:34:53Z

platform/wayland/cog-platform-wl.c

+    /* get a reference to the hashmap with viewports added to the shell */
+    self->viewports = cog_shell_get_viewports(shell);
+
+    g_signal_connect_swapped(shell, "create-viewport", G_CALLBACK(cog_wl_platform_on_create_viewport), self);


Technically speaking the Wayland plug-in could also track all the viewports by overriding the GObject.constructed and GObject.dispose virtual functions in its CogWlViewport implementation. One reason to do that would be untangling CogPlatform from CogShell, which we will want to do at some point. This being said, I think we can merge this as-is for now and take care of this change later, when we tackle #634.

aperezdc · 2023-11-27T09:45:16Z

core/cog-shell.c

-    CogViewport              *viewport;
+    WebKitSettings   *web_settings;
+    WebKitWebContext *web_context;
+    GHashTable       *viewports; /* (CogViewportKey, CogViewport) */


Is there any reason to use a hash table? I think this could be a GPtrArray, and then have an API similar to the one used to associate view with viewports:

void cog_shell_add_viewport(CogShell*, CogViewport*); void cog_shell_remove_viewport(CogShell*, CogViewport*); gsize cog_shell_get_n_viewports(CogShell*); CogViewport *cog_shell_get_nth_viewport(CogShell*, gsize index); // This would get the viewport at index 0, creating it if needed when the array is empty. CogViewport *cog_shell_get_viewport(CogShell*); // Optional, I would add those when/if they are useful. No need to implement these for now. gboolean cog_shell_contains_viewport(CogShell*, CogViewport*); void cog_shell_foreach_viewport(CogShell*, GFunc func, void *userdata);

I think this makes the API easier to use, and also consistent with other parts of the API (and that is always a plus).

Also, I expect most applications will have a limited amount of viewports, typically one or two for embedded usage. Even in a complete desktop browser, we may have half a dozen windows/viewports, with tens of tabs/views each: arrays and linear lookups will be faster for the typical case, and fast enough for the unlikely “full desktop browser” case.

aperezdc · 2023-11-27T09:52:09Z

examples/viewports.c

+
+    g_set_prgname("view-stacks");
+
+    g_autoptr(CogShell) shell = cog_shell_new(g_get_prgname(), FALSE);


You can pass NULL as the shell name, and cog_shell_init() will use g_get_prgname() automatically:

Suggested change

g_autoptr(CogShell) shell = cog_shell_new(g_get_prgname(), FALSE);

g_autoptr(CogShell) shell = cog_shell_new(NULL, FALSE);

examples/viewports.c

aperezdc · 2023-11-27T10:01:44Z

examples/viewports.c

+
+    for (int i = 1; i < argc; i++) {
+        g_autoptr(CogView) view = cog_view_new(NULL);
+        cog_platform_init_web_view(platform, WEBKIT_WEB_VIEW(view));


Having to call cog_platform_init_web_view() by hand is annoying. I'm sure we can change it in the future, we can add a CogView:viewport property (most of the code is already there) and then view code that needs late initialization when the view is known to have a viewport assigned can monitor the property. Currently what the x11, drm, and gtk4 plug-ins do is that (late initializations) or other configurations which could be moved to the GObject.constructed virtual function.

Anyway, this is not something we need to change now, I am writing my thoughts here so we can add an issue and make a patch later.

Issue created to handle this later: #678 🏗️

aperezdc

We can land this, assuming we will continue improving things and we will get rid of cog_shell_get_viewports() later on.

PTAL at the comment about removing the typedef before merging.

😺

aperezdc · 2023-11-27T14:47:44Z

core/cog-shell.c

+GPtrArray *
+cog_shell_get_viewports(CogShell *shell)


I'm not a fan of this, because in general it's a bad idea to use GPtrArray in public API (it is not introspectable) but provided that we would do #634 later I think we can land this patch as-is.

aperezdc · 2023-11-27T14:49:36Z

core/cog-shell.h

 typedef struct _CogViewport   CogViewport;
 typedef struct _WebKitWebView WebKitWebView;

-#define COG_TYPE_SHELL  (cog_shell_get_type ())
+typedef gpointer CogViewportKey;


We don't need this typedef anymore 😉

Add new functions in the CogShell API for handling multiple Viewports: * GPtrArray *cog_shell_get_viewports(CogShell *shell); * void cog_shell_add_viewport(CogShell *shell, CogViewport *viewport); * void cog_shell_remove_viewport(CogShell *shell, CogViewport *viewport); * gsize cog_shell_get_n_viewports(CogShell *shell); * CogViewport *cog_shell_get_nth_viewport(CogShell *shell, gsize index); Also added the CogShell::create-viewport signal. This signal is emitted when the shell creates a new CogViewport. With handling this signal is possible to customize the required actions to be done during the creation of a new viewport. This change modifies the 'cog_shell_get_viewport()'. Now it creates a default viewport in order to keep the current behavior. The creation of the default viewport is lazy and it is delayed until the invocation of the 'cog_shell_get_viewport()'.

... and reset the reference in the Platform if the destroyed popup is the same.

Add a new multiple viewports example program which shows how to attach 2 viewports to the CogShell. Each viewport iterates over a couple of views.

psaavedra self-assigned this Nov 24, 2023

psaavedra force-pushed the psaavedra/shell_handles_many_viewports branch from d87e437 to 37ea57e Compare November 24, 2023 07:44

psaavedra commented Nov 24, 2023

View reviewed changes

psaavedra requested a review from aperezdc November 24, 2023 07:59

psaavedra added the enhancement New feature or request label Nov 24, 2023

psaavedra mentioned this pull request Nov 24, 2023

Allow multiple viewports on Wayland platform #646

Closed

8 tasks

psaavedra changed the title ~~wl: Shell handles many viewports~~ Allow multiple viewports on Wayland platform Nov 24, 2023

psaavedra force-pushed the psaavedra/shell_handles_many_viewports branch from 37ea57e to 6f4126e Compare November 24, 2023 11:18

psaavedra commented Nov 24, 2023

View reviewed changes

aperezdc requested changes Nov 27, 2023

View reviewed changes

aperezdc mentioned this pull request Nov 27, 2023

Remove cog_platform_init_web_view() from the API #678

Open

psaavedra force-pushed the psaavedra/shell_handles_many_viewports branch 2 times, most recently from 20ce3a1 to 701d9ab Compare November 27, 2023 13:44

psaavedra requested a review from aperezdc November 27, 2023 13:45

psaavedra force-pushed the psaavedra/shell_handles_many_viewports branch from 701d9ab to 9aee57e Compare November 27, 2023 13:57

aperezdc approved these changes Nov 27, 2023

View reviewed changes

psaavedra added 3 commits November 27, 2023 16:06

wl: Ensure destroying the previous popup

6568884

... and reset the reference in the Platform if the destroyed popup is the same.

examples: Add multiple viewports example program

4b04bd6

Add a new multiple viewports example program which shows how to attach 2 viewports to the CogShell. Each viewport iterates over a couple of views.

psaavedra force-pushed the psaavedra/shell_handles_many_viewports branch from 9aee57e to 4b04bd6 Compare November 27, 2023 15:08

psaavedra merged commit d1fbcb7 into master Nov 27, 2023
4 checks passed

psaavedra deleted the psaavedra/shell_handles_many_viewports branch November 27, 2023 15:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow multiple viewports on Wayland platform #675

Allow multiple viewports on Wayland platform #675

psaavedra commented Nov 24, 2023 •

edited

Loading

psaavedra Nov 24, 2023

aperezdc Nov 27, 2023

psaavedra Nov 24, 2023

aperezdc left a comment

aperezdc Nov 27, 2023

aperezdc Nov 27, 2023

aperezdc Nov 27, 2023

aperezdc Nov 27, 2023

aperezdc Nov 27, 2023

aperezdc Nov 27, 2023

aperezdc left a comment

aperezdc Nov 27, 2023

aperezdc Nov 27, 2023


		g_set_prgname("view-stacks");

		g_autoptr(CogShell) shell = cog_shell_new(g_get_prgname(), FALSE);

Allow multiple viewports on Wayland platform #675

Allow multiple viewports on Wayland platform #675

Conversation

psaavedra commented Nov 24, 2023 • edited Loading

Pre-requisites:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aperezdc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aperezdc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psaavedra commented Nov 24, 2023 •

edited

Loading