-
Notifications
You must be signed in to change notification settings - Fork 61
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
Factor out switcheroos for the different wayland shell protocols #359
base: master
Are you sure you want to change the base?
Factor out switcheroos for the different wayland shell protocols #359
Conversation
The |
616a173
to
33a8e45
Compare
Ok I have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good start. The two biggest correctness issues here are that the shell functions pointer is accessed without allocating it (null pointer dereference), as mentioned below, and also that in registry_global, if none of the cases that set shell functions are hit, the shell functions will be undefined (rather than leading to a default set of functions that would print an error message, or some other appropriate action). In practice, both of these issues would result in crashing in the best case.
Once these issues are tackled, the next big thing will be figuring out how to get these into separate files. Separating the data necessary for these functions from win_data will play a large part in that, but let's tackle that when we get there!
code-style.diff
Outdated
@@ -0,0 +1,54 @@ | |||
--- cog.c (before formatting) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this file isn't intended to be in this commit.
platform/wayland/cog-platform-wl.c
Outdated
@@ -757,14 +961,30 @@ registry_global (void *data, | |||
wl_data.subcompositor = wl_registry_bind(registry, name, &wl_subcompositor_interface, version); | |||
} else if (strcmp(interface, wl_shell_interface.name) == 0) { | |||
wl_data.shell = wl_registry_bind(registry, name, &wl_shell_interface, version); | |||
win_data.shell_functions->enter_fullscreen = &wl_shell_enter_fullscreen; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than setting each function, it might be worthwhile just having each set of functions pre-defined, so something like:
static const shell_operations wayland_shell_operations = {
.enter_fullscreen = &wl_shell_enter_fullscreen,
.exit_fullscreen = &wl_shell_exit_fullscreen,
...
};
...
win_data.shell_functions = &wayland_shell_operations
As it is, win_data.shell_functions
is a pointer and this is referencing invalid memory as I don't see anywhere where shell_functions
is set.
platform/wayland/cog-platform-wl.c
Outdated
{ | ||
g_clear_pointer(&wl_data.event_src, g_source_destroy); | ||
win_data.shell_functions->destroy_shell(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it's worth clearing the shell_functions
pointer after calling destroy_shell
- this would make the situation where a shell function is erroneously called after this function easier to debug.
platform/wayland/cog-platform-wl.c
Outdated
#endif | ||
|
||
typedef struct shell_operations { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be good to be more consistent with naming - we have shell_operations * shell_functions
just below here. Also, I notice that two extra xdg-related pointers are added to the win_data
struct. Maybe it would be better to have a structure "shell_context" that encapsulates both the functions and any implementation-specific data necessary, and calling this "shell_protocol". Use of unions here might save some memory and make organisation a bit easier, for example:
typedef struct {
void (*enter_fullscreen)();
void (*exit_fullscreen)();
} ShellFunctions;
struct xdg_shell_protocol = {
ShellFunctions functions;
struct xdg_surface * xdg_surface;
struct xdg_toplevel * xdg_toplevel;
};
struct f_shell_protocol {
ShellFunctions functions;
// ... Any data that fshell needs specifically ...
};
typedef union {
ShellFunctions functions;
struct xdg_shell_protocol xdg_context;
} ShellContext;
static ShellContext xdg_shell = {
.functions = {
&xdg_enter_fullscreen,
&xdg_exit_fullscreen
}
};
(excuse any incorrect syntax here, let's call this pseudo-code :))
So after this, you could have a ShellContext* ctx
in win_data, and accessing functions would work much the same, like win_data.ctx->functions.enter_fullscreen()
and individual data could be accessed like win_data.ctx->xdg_context.xdg_surface
.
f230dd8
to
e8baaff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely moving in the right direction - some of my comments from before still apply and I've added some new ones for this current revision.
static void resize_to_largest_output(); | ||
static void resize_window(); | ||
static void configure_surface_geometry(); | ||
static void xdg_surface_on_configure(void *data, struct xdg_surface *surface, uint32_t serial); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for ease of reading, it'd be nice to put an empty line between the forward declarations above and the shell function definitions here.
int32_t height, | ||
struct wl_array * states); | ||
static void xdg_toplevel_on_close(void *data, struct xdg_toplevel *xdg_toplevel); | ||
static void shell_surface_ping(void *data, struct wl_shell_surface *shell_surface, uint32_t serial); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having the xdg and shell surface/popup listener declarations mixed together like this is a little confusing, better to keep them together and separated by newlines to make reading easier. I would also consider putting them closer to where they're used, rather than just at the very top like this.
platform/wayland/cog-platform-wl.c
Outdated
@@ -373,6 +616,23 @@ wl_src_finalize(GSource *base) | |||
{ | |||
} | |||
|
|||
static void | |||
default_no_shell(shell_functions *shell_functions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what this function is actually intended to do at the moment.
platform/wayland/cog-platform-wl.c
Outdated
if (wl_data.shell != NULL) { | ||
win_data.shell_functions = &wayland_shell_functions; | ||
} else { | ||
default_no_shell(&wayland_shell_functions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess default_no_shell should be setting up default shell functions, but currently it checks whether various function pointers exist in the given shell_functions struct and then calls the default implementation of the first one it finds and returns?
I would suggest that much like you now have wayland_shell_functions, xdg_shell_functions and f_shell_functions, you should also have no_shell_functions (or whatever you'd like to call it) and that you just set that up immediately before this big if/else block. That would mean you no longer need all these else clauses.
I may have misinterpreted the intension of default_no_shell though, please let me know if so!
630a199
to
4e3d79f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's been a misunderstand of how unions work here, as there are multiple parts of the code that access multiple members of it. Only one member of the shell_context union is going to be valid for the lifecycle of the application - only the shell_functions member should be accessed directly by code outside of shell functions (inside of which you can know which member of the union is valid).
Are you testing cog after making these changes? It seems unlikely that you wouldn't see crashing - make sure to test at least a couple of backends as you work, most of these issues would very likely be flagged by basic testing.
platform/wayland/cog-platform-wl.c
Outdated
} shell_functions; | ||
|
||
typedef struct { | ||
shell_functions * functions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's any reason to make this a pointer.
platform/wayland/cog-platform-wl.c
Outdated
.maximize_surface = &maximize_xdg_shell_surface, | ||
.create_window = &create_xdg_shell_window, | ||
.create_popup = &create_xdg_shell_popup}; | ||
xdg_shell_data xdg_data = {.functions = &xdg_shell_functions}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If functions wasn't a pointer, you could just declare the functions struct directly here;
.functions = { .enter_fullscreen = ...,
... }
platform/wayland/cog-platform-wl.c
Outdated
g_assert_not_reached(); | ||
} | ||
static void | ||
maximize_no_shell() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be consistent with what you've done above, this should be called no_shell_maximize
.
platform/wayland/cog-platform-wl.c
Outdated
|
||
win_data.wl_surface = wl_compositor_create_surface (wl_data.compositor); | ||
g_assert (win_data.wl_surface); | ||
win_data.shell_context->wl_shell_data.wl_surface = wl_compositor_create_surface(wl_data.compositor); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why has wl_surface been moved to wl_shell_data? It doesn't seem to be specific to wl_shell and putting it here means that you're going to corrupt data in some way or another when using any shell that isn't wl_shell.
platform/wayland/cog-platform-wl.c
Outdated
g_clear_pointer (&win_data.xdg_surface, xdg_surface_destroy); | ||
g_clear_pointer (&win_data.shell_surface, wl_shell_surface_destroy); | ||
g_clear_pointer (&win_data.wl_surface, wl_surface_destroy); | ||
g_clear_pointer(&win_data.shell_context->xdg_shell_data.xdg_toplevel, xdg_toplevel_destroy); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shell_context
is only going to be one type of shell, accessing it as multiple types like this almost certainly means you're calling one or more of these functions with invalid data. This should be factored out into a surface_destroy function in shell_functions.
platform/wayland/cog-platform-wl.c
Outdated
g_clear_pointer (&popup_data.xdg_positioner, xdg_positioner_destroy); | ||
g_clear_pointer (&popup_data.shell_surface, wl_shell_surface_destroy); | ||
g_clear_pointer (&popup_data.wl_surface, wl_surface_destroy); | ||
g_clear_pointer(&popup_data.shell_context->xdg_shell_data.xdg_popup, xdg_popup_destroy); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with this block, you can't access multiple members of shell_context
like this, only one is going to be valid.
4e3d79f
to
d86b37a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely looking better, but lots of small things to address. I wouldn't mind seeing another commit that addresses these smaller details, but feel free to go ahead with splitting this out into separate file(s) if it makes sense for you.
platform/wayland/cog-platform-wl.c
Outdated
xdg_toplevel_unset_fullscreen(win_data.shell_context->xdg_shell_data.xdg_toplevel); | ||
} | ||
static void | ||
destroy_xdg_shell() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be called xdg_shell_destroy_shell, to match the other functions.
platform/wayland/cog-platform-wl.c
Outdated
xdg_wm_base_destroy(wl_data.xdg_shell); | ||
} | ||
static void | ||
maximize_xdg_shell_surface() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, xdg_shell_maximize_surface.
platform/wayland/cog-platform-wl.c
Outdated
}; | ||
|
||
static void | ||
create_xdg_shell_window() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
xdg_shell_create_window.
platform/wayland/cog-platform-wl.c
Outdated
wl_surface_commit(win_data.wl_surface); | ||
} | ||
static void | ||
create_xdg_shell_popup() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
xdg_shell_create_popup
platform/wayland/cog-platform-wl.c
Outdated
} | ||
|
||
static void | ||
destroy_xdg_surfaces() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
xdg_shell_surface_destroy
platform/wayland/cog-platform-wl.c
Outdated
.enter_fullscreen = &f_shell_enter_fullscreen}}; | ||
|
||
//default executed when no shell exist | ||
static void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would declare these before all the shell implementations, so that when shells are missing capabilities (like with f_shell above), you can reference these default implementations. I also note that there doesn't seem to be a full set of functions or an accompanying shell_functions struct for these.
platform/wayland/cog-platform-wl.c
Outdated
wl_surface_damage(win_data.wl_surface, 0, 0, INT32_MAX, INT32_MAX); | ||
request_frame (); | ||
wl_surface_commit (win_data.wl_surface); | ||
wl_surface_attach(win_data.shell_context->wl_shell_data.wl_surface, buffer->buffer, 0, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be win_data.wl_surface, right? This wouldn't build if HAVE_SHM_EXPORTED_BUFFER is defined.
platform/wayland/cog-platform-wl.c
Outdated
g_clear_pointer (&popup_data.xdg_positioner, xdg_positioner_destroy); | ||
g_clear_pointer (&popup_data.shell_surface, wl_shell_surface_destroy); | ||
g_clear_pointer (&popup_data.wl_surface, wl_surface_destroy); | ||
win_data.shell_context->functions.surface_destroy(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a little confusing to me - perhaps this function should be called popup_surface_destroy to be clearer?
d86b37a
to
bbcc7e5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking better, but still some issues to address.
platform/wayland/cog-platform-wl.c
Outdated
static void | ||
xdg_shell_create_window() | ||
{ | ||
win_data.shell_context->xdg_shell_data.xdg_surface = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make these functions read a bit more easily, it might be worthwhile to have a xdg_shell_data* xdg_data = (xdg_shell_data*)win_data.shell_context;
at the top to stop from having to type win_data.shell_context->xdg_shell_data
so much.
platform/wayland/cog-platform-wl.c
Outdated
|
||
const char * app_id = NULL; | ||
GApplication *app = g_application_get_default(); | ||
if (app) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the documentation for g_application_get_application_id
, this could just be written directly as
const char * app_id = g_application_get_application_id(g_application_get_default());
if (!app_id)
app_id = COG_DEFAULT_APPID;
as g_application_get_application_id
accepts NULL as a parameter (apparently - worth testing before committing to, in case the documentation is incorrect).
|
||
static struct { | ||
shell_context * shell_context; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this added, but I don't see it set anywhere.
platform/wayland/cog-platform-wl.c
Outdated
static void | ||
xdg_shell_create_popup() | ||
{ | ||
popup_data.shell_context->xdg_shell_data.xdg_positioner = xdg_wm_base_create_positioner(wl_data.xdg_shell); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this function uses popup_data.shell_context
, but I don't see that set anywhere? If it was set, given it's a pointer, you would need to copy it to avoid using the same variables as win_data. I think you're going in the right direction, but there's a misunderstanding here of what pointers are - You can't use the same surface pointers for both win_data
and popup_data
. As before, I think if this code was actually run as, it'd very likely crash. Have you tested this?
0db356b
to
42ac42a
Compare
42ac42a
to
7976a2d
Compare
7976a2d
to
e56bdca
Compare
Change text mentioning the "fdo" platform plug-in to "wl", which was missing from Igalia#351.
Arrange to call drmFreeDevices() inside check_drm() and init_drm() to avoid leaking the arrays of devices returned by drmGetDevices2().
Pass the correct pkg-config module name to gi-docgen depending on the version of libsoup in use: WPE WebKit builds with libsoup2 have API version 1.0, and builds with libsoup3 have API version 1.1.
This avoids not finding any environment setup script due to the CPU name and features changing when SDKs are updated: use a wildcard to make sure at least one gets picked.
Use GObjectClass.finalize instead of CogPlatformClass.teardown. Now that CogPlatform derives from GObject, it is possible to reuse its functionality instead of having an ad-hoc vfunc that gets called immediately before destroying an instance. As a bonus, not only the implementations follow a more idiomatic GObject approach, but also it is not possible anymore to forget a call to cog_platform_teardown() before destruction.
Update the check-style script and the CI job to use clang-format 13, which is the first version that supports combining The "PointerAlignment: Right" with "AlignConsecutiveDeclarations: true". This was unimplemented until recently, see https://reviews.llvm.org/D103245 While at it, do not let clang-format choose a different way of doing pointer alignment by setting "DerivePointerAlignment: false".
Retry the curl command up to ten times in case of download failures. This should alleviate build issues like the following, where the server sometimes seems to wrongly close HTTP/2 streams, or when some connection issue results in partial downloads: https://github.com/Igalia/cog/runs/4176134469 https://github.com/Igalia/cog/runs/4176255670 While at it, pass "-C -" to let curl determine from which point to retry the download. As the toolchain packages are big, this should help avoid timeouts. Finally, use HTTP/1.1; because using HTTP/2 seems to be a source of sporadic issues, and for a big, bulky download, the gains from HTTP/2 (like multiple streams and faster connection open) are basically none due to the transfer itself taking most of the time.
I have taken a brief look at this, and it looks like there is not much left to do before it can be merged—thanks everybody involved! Nevertheless, I would like to make today the 0.12 stable branch, so I am going to defer to tag it for 0.14 😸 |
As agreed with @Nasah-Kuma, I have done a rebase and squashed some of the commits together, and added one more commit with a small cleanup—I will push it momentarily. While testing, I have noticed that with the changes applied Cog segfaults when running under wlroots-based compositors (which used to work fine). The traceback is as follows (log edited to show the interesting bits):
I have not yet investigated why this happens, but will try to debug it in the next days. Running under Weston works as expected, so I suppose that was the compositor used for testing 😄 |
Ah, I just realized that of course I cannot push to @Nasah-Kuma's branch in their fork, so I have posted the rebase to the aperezdc/pr359-rebased branch for now. |
No description provided.