From 6f40b8d0a1462a000ddbe0261577884e4443f5f4 Mon Sep 17 00:00:00 2001 From: Kovid Goyal Date: Sat, 25 Jul 2020 15:31:52 +0530 Subject: [PATCH] Simplify Wayland cursor theme handling Now themes are loaded once per scale and stored centrally not per window. They are not unloaded till application shutdown. Since there is unlikely to be more than two or three scales this is not a big waste of resources. Since cursor lifetime is tied to theme lifetime and cursors are stored per window, destroying a theme requires destroying all cursors win all windows referring to that theme, which is too much work. Should hopefully fix #2810 --- docs/changelog.rst | 3 + glfw/wl_cursors.c | 181 ++++++++++----------------------------------- glfw/wl_cursors.h | 32 +++----- glfw/wl_init.c | 18 ++--- glfw/wl_platform.h | 4 +- glfw/wl_window.c | 17 +---- 6 files changed, 65 insertions(+), 190 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 47e89fce8..19421bd0e 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -64,6 +64,9 @@ To update |kitty|, :doc:`follow the instructions `. - macOS: Make the window id of OS windows available in the ``WINDOWID`` environment variable (:pull:`2877`) +- Wayland: Fix a regression in 0.18.0 that could cause crashes related to mouse + cursors in some rare circumstances (:iss:`2810`) + 0.18.1 [2020-06-23] -------------------- diff --git a/glfw/wl_cursors.c b/glfw/wl_cursors.c index d243eb175..dac6e5633 100644 --- a/glfw/wl_cursors.c +++ b/glfw/wl_cursors.c @@ -1,7 +1,5 @@ // Future devs supporting whatever Wayland protocol stabilizes for cursor selection: see _themeAdd. -#include "wl_cursors.h" - #include "internal.h" #include @@ -10,155 +8,58 @@ #include #include -typedef struct { - struct wl_cursor_theme *theme; - int px; - int refcount; -} _themeData; - -struct _wlCursorThemeManager { - size_t count; - /** Pointer to the head of an unsorted array of themes with no sentinel. - * - * The lack of sort (and thus forcing a linear search) is intentional; - * in most cases, users are likely to have 1-2 different cursor sizes loaded. - * For those cases, we get no benefit from sorting and added constant overheads. - * - * Don't change this to a flexible array member because that complicates growing/shrinking. - */ - _themeData *themes; -}; - -static void -_themeInit(_themeData *dest, const char *name, int px) { - dest->px = px; - dest->refcount = 1; - if (_glfw.wl.shm) { - dest->theme = wl_cursor_theme_load(name, px, _glfw.wl.shm); - if(!dest->theme) { - _glfwInputError(GLFW_PLATFORM_ERROR, - "Wayland: Unable to load cursor theme"); +static int +pixels_from_scale(int scale) { + static bool queried_env = false; + static int factor = 32; + if (!queried_env) { + const char *env = getenv("XCURSOR_SIZE"); + if (env) { + const int retval = atoi(env); + if (retval > 0 && retval < 2048) factor = 32; } - } else { - dest->theme = NULL; + queried_env = true; } + return factor * scale; } -static struct wl_cursor_theme* -_themeAdd(int px, _wlCursorThemeManager *manager) { - ++manager->count; - _themeData *temp = realloc(manager->themes, sizeof(_themeData)*manager->count); - if (!temp) { - _glfwInputError(GLFW_OUT_OF_MEMORY, - "OOM during cursor theme management."); - return NULL; - } else { - manager->themes = temp; - _themeInit(manager->themes + manager->count-1, - getenv("XCURSOR_THEME"), - px); - return manager->themes[manager->count-1].theme; - } -} -//WARNING: No input safety checks. -static inline void _themeInc(_themeData - *theme) { - ++(theme->refcount); -} +struct wl_cursor_theme* +glfw_wlc_theme_for_scale(int scale) { + GLFWWLCursorThemes *t = &_glfw.wl.cursor_themes; + for (size_t i = 0; i < t->count; i++) { + if (t->themes[i].scale == scale) return t->themes[i].theme; + } -// WARNING: No input safety checks. -// In particular, doesn't check if theme is actually managed by the manager. -static void -_themeDec(_themeData *theme, _wlCursorThemeManager *manager) { - if (--(theme->refcount) == 0) { - wl_cursor_theme_destroy(theme->theme); - if (--(manager->count) > 0) { - const _themeData *last_theme = (manager->themes)+(manager->count); - *theme = *last_theme; - _themeData *temp = realloc(manager->themes, (manager->count)*sizeof(_themeData)); - // We're shrinking here, so it's not catastrophic if realloc fails. - if (temp) manager->themes = temp; - } else { - free(manager->themes); - manager->themes = NULL; + if (t->count >= t->capacity) { + t->themes = realloc(t->themes, sizeof(GLFWWLCursorTheme) * (t->count + 16)); + if (!t->themes) { + _glfwInputError(GLFW_PLATFORM_ERROR, "Wayland: Out of memory allocating space for cursor themes"); + return NULL; } + t->capacity = t->count + 16; } -} - -static _wlCursorThemeManager _default = {0}; - -_wlCursorThemeManager* -_wlCursorThemeManagerDefault() { - return &_default; + struct wl_cursor_theme *ans = wl_cursor_theme_load(getenv("XCURSOR_THEME"), pixels_from_scale(scale), _glfw.wl.shm); + if (!ans) { + _glfwInputError( + GLFW_PLATFORM_ERROR, "Wayland: wl_cursor_theme_load failed at scale: %d pixels: %d", + scale, pixels_from_scale(scale) + ); + return NULL; + } + GLFWWLCursorTheme *theme = t->themes + t->count++; + theme->scale = scale; + theme->theme = ans; + return ans; } void -_wlCursorThemeManagerDestroy(_wlCursorThemeManager *manager) { - if (manager) { - for (size_t i = 0; i < manager->count; ++i) { - wl_cursor_theme_destroy(manager->themes[i].theme); - } - free(manager->themes); - } -} +glfw_wlc_destroy(void) { + GLFWWLCursorThemes *t = &_glfw.wl.cursor_themes; -static struct wl_cursor_theme* -_wlCursorThemeManagerGet(_wlCursorThemeManager *manager, int px) { - _themeData *themedata = NULL; - for (size_t i = 0; i < manager->count; ++i) { - _themeData *temp = manager->themes+i; - if (temp->px == px) { - themedata = temp; - break; - } + for (size_t i = 0; i < t->count; i++) { + wl_cursor_theme_destroy(t->themes[i].theme); } - if (themedata != NULL) { - _themeInc(themedata); - return themedata->theme; - } - return _themeAdd(px, manager); -} - -struct wl_cursor_theme* -_wlCursorThemeManage(_wlCursorThemeManager *manager, struct wl_cursor_theme *theme, int px) { - //WARNING: Multiple returns. - if (manager == NULL) { - return NULL; - } - if (theme != NULL) { - // Search for the provided theme in the manager. - _themeData *themedata = NULL; - for (size_t i = 0; i < manager->count; ++i) { - _themeData *temp = manager->themes+i; - if (temp->theme == theme) { - themedata = temp; - break; - } - } - if (themedata != NULL) { - // Search succeeded. Check if we can avoid unnecessary operations. - if (themedata->px == px) return theme; - _themeDec(themedata, manager); - } else { - _glfwInputError(GLFW_PLATFORM_ERROR, - "Wayland internal: managed theme isn't in the provided manager"); - return theme; - //^ This is probably the sanest behavior for this situation: do nothing. - } - } - return px > 0 ? _wlCursorThemeManagerGet(manager, px) : NULL; -} - -int -_wlCursorPxFromScale(int scale) { - const char *envStr = getenv("XCURSOR_SIZE"); - if(envStr != NULL) { - const int retval = atoi(envStr); - //^ atoi here is fine since 0 is an invalid value. - if(retval > 0 && retval <= INT_MAX/scale) { - return retval*scale; - } - } - return 32*scale; + free(t->themes); + t->themes = NULL; t->capacity = 0; t->count = 0; } diff --git a/glfw/wl_cursors.h b/glfw/wl_cursors.h index 3a0a8c8fc..9c1a30967 100644 --- a/glfw/wl_cursors.h +++ b/glfw/wl_cursors.h @@ -2,29 +2,17 @@ #include -typedef struct _wlCursorThemeManager _wlCursorThemeManager; +typedef struct { + struct wl_cursor_theme *theme; + int scale; +} GLFWWLCursorTheme; -/** Returns a pointer to a wlCursorThemeManagerInstance. - * Repeatedly calling this function will return the same instance. - * - * The retrieved instance must be destroyed with _wlCursorThemeManagerDestroy. - */ -_wlCursorThemeManager* _wlCursorThemeManagerDefault(void); -/** Set a wl_cursor_theme pointer variable to a pointer to a managed cursor theme. - * Pass the desired px as the third argument. - * Returns a pointer to a managed theme, or NULL if the desired px is 0 or an error occurs. - * - * The passed theme pointer must either be NULL or a pointer to a theme managed by the passed manager. - * The provided pointer may be invalidated if it's non-NULL. - */ -struct wl_cursor_theme* -_wlCursorThemeManage(_wlCursorThemeManager*, struct wl_cursor_theme*, int); +typedef struct { + GLFWWLCursorTheme *themes; + size_t count, capacity; +} GLFWWLCursorThemes; -void _wlCursorThemeManagerDestroy(_wlCursorThemeManager*); -/** Helper method to determine the appropriate size in pixels for a given scale. - * - * Reads XCURSOR_SIZE if it's set and is valid, else defaults to 32*scale. - */ -int _wlCursorPxFromScale(int); +struct wl_cursor_theme* glfw_wlc_theme_for_scale(int scale); +void glfw_wlc_destroy(void); diff --git a/glfw/wl_init.c b/glfw/wl_init.c index ed06f2d48..b336c5a3b 100644 --- a/glfw/wl_init.c +++ b/glfw/wl_init.c @@ -148,12 +148,9 @@ static void setCursor(GLFWCursorShape shape, _GLFWwindow* window) struct wl_surface* surface = _glfw.wl.cursorSurface; const int scale = window->wl.scale; - window->wl.cursorTheme = _wlCursorThemeManage( - _glfw.wl.cursorThemeManager, - window->wl.cursorTheme, - _wlCursorPxFromScale(scale) - ); - cursor = _glfwLoadCursor(shape, window->wl.cursorTheme); + struct wl_cursor_theme *theme = glfw_wlc_theme_for_scale(scale); + if (!theme) return; + cursor = _glfwLoadCursor(shape, theme); if (!cursor) return; // TODO: handle animated cursors too. image = cursor->images[0]; @@ -780,13 +777,14 @@ int _glfwPlatformInit(void) if (_glfw.wl.shm) { - _glfw.wl.cursorThemeManager = _wlCursorThemeManagerDefault(); _glfw.wl.cursorSurface = wl_compositor_create_surface(_glfw.wl.compositor); } else { - _glfw.wl.cursorThemeManager = NULL; + _glfwInputError(GLFW_PLATFORM_ERROR, + "Wayland: Failed to find Wayland SHM"); + return false; } return true; @@ -812,9 +810,7 @@ void _glfwPlatformTerminate(void) if (_glfw.wl.cursorSurface) wl_surface_destroy(_glfw.wl.cursorSurface); - if (_glfw.wl.cursorThemeManager) { - _wlCursorThemeManagerDestroy(_glfw.wl.cursorThemeManager); - } + glfw_wlc_destroy(); if (_glfw.wl.subcompositor) wl_subcompositor_destroy(_glfw.wl.subcompositor); if (_glfw.wl.compositor) diff --git a/glfw/wl_platform.h b/glfw/wl_platform.h index 7b2191bdf..aed141a3a 100644 --- a/glfw/wl_platform.h +++ b/glfw/wl_platform.h @@ -132,7 +132,6 @@ typedef struct _GLFWwindowWayland _GLFWcursor* currentCursor; double cursorPosX, cursorPosY; - struct wl_cursor_theme* cursorTheme; char* title; char appId[256]; @@ -262,8 +261,7 @@ typedef struct _GLFWlibraryWayland size_t dataOffersCounter; _GLFWWaylandDataOffer dataOffers[8]; char* primarySelectionString; - - _wlCursorThemeManager* cursorThemeManager; + GLFWWLCursorThemes cursor_themes; } _GLFWlibraryWayland; // Wayland-specific per-monitor data diff --git a/glfw/wl_window.c b/glfw/wl_window.c index 13c1ba02e..3b1fd1894 100644 --- a/glfw/wl_window.c +++ b/glfw/wl_window.c @@ -41,11 +41,6 @@ #include #include -static void -load_cursor_theme(_GLFWwindow* window) { - window->wl.cursorTheme = _wlCursorThemeManage( - _glfw.wl.cursorThemeManager, window->wl.cursorTheme, _wlCursorPxFromScale(window->wl.scale)); -} static void setCursorImage(_GLFWwindow* window) @@ -63,8 +58,9 @@ setCursorImage(_GLFWwindow* window) } else { if (cursorWayland->scale != scale) { - if (!window->wl.cursorTheme) load_cursor_theme(window); - struct wl_cursor *newCursor = _glfwLoadCursor(cursorWayland->shape, window->wl.cursorTheme); + struct wl_cursor *newCursor = NULL; + struct wl_cursor_theme *theme = glfw_wlc_theme_for_scale(scale); + if (theme) newCursor = _glfwLoadCursor(cursorWayland->shape, theme); if (newCursor != NULL) { cursorWayland->cursor = newCursor; cursorWayland->scale = scale; @@ -136,7 +132,6 @@ static bool checkScaleChange(_GLFWwindow* window) { window->wl.scale = scale; wl_surface_set_buffer_scale(window->wl.surface, scale); - load_cursor_theme(window); setCursorImage(window); return true; } @@ -945,12 +940,6 @@ int _glfwPlatformCreateWindow(_GLFWwindow* window, void _glfwPlatformDestroyWindow(_GLFWwindow* window) { - if (window->wl.cursorTheme) { - _wlCursorThemeManage(_glfw.wl.cursorThemeManager, - window->wl.cursorTheme, - 0); - window->wl.cursorTheme = NULL; - } if (window == _glfw.wl.pointerFocus) { _glfw.wl.pointerFocus = NULL;