Factor out switcheroos for the different wayland shell protocols #359

Nasah-Kuma · 2021-09-14T02:32:19Z

No description provided.

philn · 2021-09-14T17:45:59Z

The rename fdo to wl in the source and build files commit was merged in master. Can you rebase please?

Nasah-Kuma · 2021-09-14T21:39:42Z

The rename fdo to wl in the source and build files commit was merged in master. Can you rebase please?

Ok I have

Cwiiis

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!

Cwiiis · 2021-09-16T14:07:18Z

code-style.diff

@@ -0,0 +1,54 @@
+--- cog.c	(before formatting)


I assume this file isn't intended to be in this commit.

Cwiiis · 2021-09-16T14:16:34Z

platform/wayland/cog-platform-wl.c

@@ -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;


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.

Cwiiis · 2021-09-16T14:19:06Z

platform/wayland/cog-platform-wl.c

 {
    g_clear_pointer(&wl_data.event_src, g_source_destroy);
+    win_data.shell_functions->destroy_shell();


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.

Cwiiis · 2021-09-16T14:41:38Z

platform/wayland/cog-platform-wl.c

 #endif

+typedef struct shell_operations {


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.

Cwiiis

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.

Cwiiis · 2021-09-21T15:36:53Z

platform/wayland/cog-platform-wl.c

+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);


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.

Cwiiis · 2021-09-21T15:40:14Z

platform/wayland/cog-platform-wl.c

+                                      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);


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.

Cwiiis · 2021-09-21T15:45:26Z

platform/wayland/cog-platform-wl.c

@@ -373,6 +616,23 @@ wl_src_finalize(GSource *base)
 {
 }

+static void
+default_no_shell(shell_functions *shell_functions)


Not sure what this function is actually intended to do at the moment.

Cwiiis · 2021-09-21T15:49:08Z

platform/wayland/cog-platform-wl.c

+        if (wl_data.shell != NULL) {
+            win_data.shell_functions = &wayland_shell_functions;
+        } else {
+            default_no_shell(&wayland_shell_functions);


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!

Cwiiis

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.

Cwiiis · 2021-09-27T09:04:53Z

platform/wayland/cog-platform-wl.c

+} shell_functions;
+
+typedef struct {
+    shell_functions *    functions;


I don't think there's any reason to make this a pointer.

Cwiiis · 2021-09-27T09:06:23Z

platform/wayland/cog-platform-wl.c

+                                              .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};


If functions wasn't a pointer, you could just declare the functions struct directly here;

.functions = { .enter_fullscreen = ..., ... }

Cwiiis · 2021-09-27T09:08:42Z

platform/wayland/cog-platform-wl.c

+    g_assert_not_reached();
+}
+static void
+maximize_no_shell()


To be consistent with what you've done above, this should be called no_shell_maximize.

platform/wayland/cog-platform-wl.c

Cwiiis · 2021-09-27T09:34:30Z

platform/wayland/cog-platform-wl.c


-    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);


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.

Cwiiis · 2021-09-27T09:36:19Z

platform/wayland/cog-platform-wl.c

-    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);


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.

Cwiiis · 2021-09-27T09:37:33Z

platform/wayland/cog-platform-wl.c

-    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);


Same with this block, you can't access multiple members of shell_context like this, only one is going to be valid.

Cwiiis

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.

Cwiiis · 2021-10-04T12:06:02Z

platform/wayland/cog-platform-wl.c

+    xdg_toplevel_unset_fullscreen(win_data.shell_context->xdg_shell_data.xdg_toplevel);
+}
+static void
+destroy_xdg_shell()


This should be called xdg_shell_destroy_shell, to match the other functions.

Cwiiis · 2021-10-04T12:06:22Z

platform/wayland/cog-platform-wl.c

+    xdg_wm_base_destroy(wl_data.xdg_shell);
+}
+static void
+maximize_xdg_shell_surface()


Same here, xdg_shell_maximize_surface.

Cwiiis · 2021-10-04T12:07:16Z

platform/wayland/cog-platform-wl.c

+};
+
+static void
+create_xdg_shell_window()


xdg_shell_create_window.

Cwiiis · 2021-10-04T12:31:27Z

platform/wayland/cog-platform-wl.c

+    wl_surface_commit(win_data.wl_surface);
+}
+static void
+create_xdg_shell_popup()


xdg_shell_create_popup

Cwiiis · 2021-10-04T12:32:15Z

platform/wayland/cog-platform-wl.c

+}
+
+static void
+destroy_xdg_surfaces()


xdg_shell_surface_destroy

platform/wayland/cog-platform-wl.c

Cwiiis · 2021-10-04T13:08:32Z

platform/wayland/cog-platform-wl.c

+                                     .enter_fullscreen = &f_shell_enter_fullscreen}};
+
+//default executed when no shell exist
+static void


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.

Cwiiis · 2021-10-04T13:11:16Z

platform/wayland/cog-platform-wl.c

-    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);


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

Cwiiis · 2021-10-04T13:19:06Z

platform/wayland/cog-platform-wl.c

-    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();


This is a little confusing to me - perhaps this function should be called popup_surface_destroy to be clearer?

Cwiiis

Looking better, but still some issues to address.

Cwiiis · 2021-10-11T09:15:24Z

platform/wayland/cog-platform-wl.c

+static void
+xdg_shell_create_window()
+{
+    win_data.shell_context->xdg_shell_data.xdg_surface =


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.

Cwiiis · 2021-10-11T09:24:00Z

platform/wayland/cog-platform-wl.c

+
+    const char *  app_id = NULL;
+    GApplication *app = g_application_get_default();
+    if (app) {


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).

Cwiiis · 2021-10-11T09:29:50Z

platform/wayland/cog-platform-wl.c


 static struct {
+    shell_context *    shell_context;


I see this added, but I don't see it set anywhere.

Cwiiis · 2021-10-11T09:32:31Z

platform/wayland/cog-platform-wl.c

+static void
+xdg_shell_create_popup()
+{
+    popup_data.shell_context->xdg_shell_data.xdg_positioner = xdg_wm_base_create_positioner(wl_data.xdg_shell);


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?

…protocols

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.

aperezdc · 2021-11-11T16:04:26Z

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 😸

…protocols

aperezdc · 2022-01-14T18:23:20Z

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):

% WAYLAND_DISPLAY=wayland-1 G_MESSAGES_DEBUG=Cog,Cog-Core,Cog-FDO COG_MODULEDIR=b/modules \
    gdb --args b/cog -P wl ddg.gg
[...]
(cog:755159): Cog-DEBUG: 20:11:18.071: platform_setup: Platform name: wl
[...]
(cog:755159): Cog-FDO-DEBUG: 20:11:18.083: Using 'wl_compositor' interface obtained from the Wayland registry.
(cog:755159): Cog-FDO-DEBUG: 20:11:18.083: Using 'wl_subcompositor' interface obtained from the Wayland registry.
[...]
(cog:755159): Cog-FDO-DEBUG: 20:11:18.083: Using 'xdg_wm_base' interface obtained from the Wayland registry.
[...]
(cog:755159): Cog-FDO-DEBUG: 20:11:18.391: New XDG toplevel configuration: (0, 0)
(cog:755159): Cog-FDO-DEBUG: 20:11:18.391: Resized EGL buffer to: (1024, 768) @1x


Thread 1 "cog" received signal SIGSEGV, Segmentation fault.
0x00007ffff7f96928 in cog_popup_menu_get_buffer (popup_menu=0x0) at /home/aperez/devel/wpe/cog-master/platform/wayland/cog-popup-menu-wl.c:318
318         if (popup_menu->pending_changes) {
(gdb) bt
#0  0x00007ffff7f96928 in cog_popup_menu_get_buffer (popup_menu=0x0) at /home/aperez/devel/wpe/cog-master/platform/wayland/cog-popup-menu-wl.c:318
#1  0x00007ffff7f93a32 in display_popup () at /home/aperez/devel/wpe/cog-master/platform/wayland/cog-platform-wl.c:2515
#2  0x00007ffff0f8cd4a in  () at /usr/lib/libffi.so.8
#3  0x00007ffff0f8c267 in  () at /usr/lib/libffi.so.8
#4  0x00007fffeec6f323 in  () at /usr/lib/libwayland-client.so.0
#5  0x00007fffeec6b5ab in  () at /usr/lib/libwayland-client.so.0
#6  0x00007fffeec6cf4c in wl_display_dispatch_queue_pending () at /usr/lib/libwayland-client.so.0
#7  0x00007ffff7f92531 in wl_src_dispatch (base=0x5555556b50c0, callback=<optimized out>, user_data=<optimized out>)
    at /home/aperez/devel/wpe/cog-master/platform/wayland/cog-platform-wl.c:704
#8  0x00007ffff2c34fd3 in g_main_context_dispatch () at /usr/lib/libglib-2.0.so.0
#9  0x00007ffff2c8b049 in  () at /usr/lib/libglib-2.0.so.0
#10 0x00007ffff2c32545 in g_main_context_iteration () at /usr/lib/libglib-2.0.so.0
#11 0x00007ffff2e526ee in g_application_run () at /usr/lib/libgio-2.0.so.0
#12 0x0000555555559b9f in main (argc=3, argv=0x7fffffffdb68) at /home/aperez/devel/wpe/cog-master/launcher/cog.c:41
(gdb)

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 😄

aperezdc · 2022-01-14T18:30:39Z

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.

Nasah-Kuma force-pushed the factor-out-switcheroos-for-the-different-Wayland-shell-protocols branch 4 times, most recently from 616a173 to 33a8e45 Compare September 14, 2021 21:33

Cwiiis reviewed Sep 16, 2021

View reviewed changes

Nasah-Kuma force-pushed the factor-out-switcheroos-for-the-different-Wayland-shell-protocols branch 2 times, most recently from f230dd8 to e8baaff Compare September 20, 2021 20:32

Cwiiis reviewed Sep 21, 2021

View reviewed changes

Nasah-Kuma force-pushed the factor-out-switcheroos-for-the-different-Wayland-shell-protocols branch 2 times, most recently from 630a199 to 4e3d79f Compare September 26, 2021 19:05

Cwiiis reviewed Sep 27, 2021

View reviewed changes

Nasah-Kuma force-pushed the factor-out-switcheroos-for-the-different-Wayland-shell-protocols branch from 4e3d79f to d86b37a Compare October 4, 2021 10:34

Cwiiis suggested changes Oct 4, 2021

View reviewed changes

Nasah-Kuma force-pushed the factor-out-switcheroos-for-the-different-Wayland-shell-protocols branch from d86b37a to bbcc7e5 Compare October 9, 2021 10:35

Cwiiis reviewed Oct 11, 2021

View reviewed changes

Nasah-Kuma force-pushed the factor-out-switcheroos-for-the-different-Wayland-shell-protocols branch 2 times, most recently from 0db356b to 42ac42a Compare October 19, 2021 20:24

Nasah-Kuma force-pushed the factor-out-switcheroos-for-the-different-Wayland-shell-protocols branch from 42ac42a to 7976a2d Compare November 1, 2021 13:22

Enhancement-Factored out switcheroos for the different wayland shell …

e56bdca

…protocols

Nasah-Kuma force-pushed the factor-out-switcheroos-for-the-different-Wayland-shell-protocols branch from 7976a2d to e56bdca Compare November 2, 2021 15:43

aperezdc added 8 commits November 3, 2021 14:21

Release version 0.11.1

52f804f

docs: Rename platform fdo to wl after Igalia#351

35f7697

Change text mentioning the "fdo" platform plug-in to "wl", which was missing from Igalia#351.

drm: Avoid leaking array of devices during check

60dcf6d

Arrange to call drmFreeDevices() inside check_drm() and init_drm() to avoid leaking the arrays of devices returned by drmGetDevices2().

docs: Fix building documentation with libsoup3

19be4ec

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.

CI: Change file name of cross-ARM toolchain

0f9afa1

CI: Link to page with toolchain info from script

90b5500

CI: Do not hardcode the CPU type when sourcing env script

c8756bd

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.

aperezdc added 4 commits November 8, 2021 19:06

CI: Use system-provided wayland-scanner binary

5b34335

CI: Use stable ARM cross-toolchain

9014551

aperezdc added the enhancement New feature or request label Nov 11, 2021

aperezdc added this to the Cog 0.14 milestone Nov 11, 2021

aperezdc and others added 5 commits November 12, 2021 00:45

Bump version numbers after release from stable branch

80b9013

Enhancement-Factored out switcheroos for the different wayland shell …

c98f6ab

…protocols

more updates

a65ca96

more updates

1088303

more updates

c59ed5d

aperezdc modified the milestones: Cog 0.14, Cog 0.16 Jun 29, 2022

aperezdc modified the milestones: Cog 0.16, Cog 0.18 Sep 29, 2022

aperezdc modified the milestones: Cog 0.18, Cog 0.20 Dec 13, 2023

Factor out switcheroos for the different wayland shell protocols #359

Are you sure you want to change the base?

Factor out switcheroos for the different wayland shell protocols #359

Conversation

Nasah-Kuma commented Sep 14, 2021

philn commented Sep 14, 2021

Nasah-Kuma commented Sep 14, 2021

Cwiiis 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

Cwiiis 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

Cwiiis 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

Cwiiis 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cwiiis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cwiiis Oct 11, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aperezdc commented Nov 11, 2021

aperezdc commented Jan 14, 2022

aperezdc commented Jan 14, 2022

Cwiiis Oct 11, 2021 •

edited