From f74c524fff44609d944f737fdae6c8ef671725c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Benno=20F=C3=BCnfst=C3=BCck?= Date: Thu, 17 Jan 2019 14:22:48 +0100 Subject: [PATCH] fix(renderer): Handling of missing root pixmap (#1608) Polybar had issues when there is no background set or set by a tool like imagemagick which doesn't add the root pixmap to the root window properties. There's not much we can do about it, but at least polybar doesn't crash anymore. Fixes #1582 Fixes #1585 * fix(tray_manager): only enable transparency if neccessary Previously, we always enabled transparency * fix(background_manager): avoid needless fetching * fix(renderer): move logging message to correct place * fix(background_manager): handle dummy pixmap (_XSETROOT_ID) right * fix(background_manager): more initialization + don't free on error Freeing on error is incorrect, since we could still be called again later in which case we still need the resources. * fix(background_manager): add more infos to trace logs * fix(background): correct typo (XROOTMAP -> XROOTPMAP) * fix(background_manager): do not report "no background" as error * style(background_manager): use braces for if Co-Authored-By: bennofs * fix(background_manager): better error message for dummy pixmap Co-Authored-By: bennofs * style(background): some more style fixes * fix(connection): initialize pixmap in all cases in root_pixmap() * style(connection): improve readability using early return --- include/x11/atoms.hpp | 2 +- src/components/renderer.cpp | 3 +-- src/x11/atoms.cpp | 4 ++-- src/x11/background_manager.cpp | 31 +++++++++++++++++++++------ src/x11/connection.cpp | 39 ++++++++++++++++++++++------------ src/x11/tray_manager.cpp | 5 +++-- 6 files changed, 56 insertions(+), 28 deletions(-) diff --git a/include/x11/atoms.hpp b/include/x11/atoms.hpp index 94aa806e..10611d84 100644 --- a/include/x11/atoms.hpp +++ b/include/x11/atoms.hpp @@ -40,7 +40,7 @@ extern xcb_atom_t _NET_SYSTEM_TRAY_COLORS; extern xcb_atom_t WM_TAKE_FOCUS; extern xcb_atom_t Backlight; extern xcb_atom_t BACKLIGHT; -extern xcb_atom_t _XROOTMAP_ID; +extern xcb_atom_t _XROOTPMAP_ID; extern xcb_atom_t _XSETROOT_ID; extern xcb_atom_t ESETROOT_PMAP_ID; extern xcb_atom_t _COMPTON_SHADOW; diff --git a/src/components/renderer.cpp b/src/components/renderer.cpp index 4e55b6e2..302b12ab 100644 --- a/src/components/renderer.cpp +++ b/src/components/renderer.cpp @@ -163,12 +163,11 @@ renderer::renderer( m_log.info("Loaded font \"%s\" (name=%s, offset=%i, file=%s)", pattern, font->name(), offset, font->file()); *m_context << move(font); } - - m_log.trace("Activate root background manager"); } m_pseudo_transparency = m_conf.get("settings", "pseudo-transparency", m_pseudo_transparency); if (m_pseudo_transparency) { + m_log.trace("Activate root background manager"); m_background = background.observe(m_bar.outer_area(false), m_window); } diff --git a/src/x11/atoms.cpp b/src/x11/atoms.cpp index 3145783f..fc87c768 100644 --- a/src/x11/atoms.cpp +++ b/src/x11/atoms.cpp @@ -33,7 +33,7 @@ xcb_atom_t _NET_SYSTEM_TRAY_COLORS; xcb_atom_t WM_TAKE_FOCUS; xcb_atom_t Backlight; xcb_atom_t BACKLIGHT; -xcb_atom_t _XROOTMAP_ID; +xcb_atom_t _XROOTPMAP_ID; xcb_atom_t _XSETROOT_ID; xcb_atom_t ESETROOT_PMAP_ID; xcb_atom_t _COMPTON_SHADOW; @@ -72,7 +72,7 @@ cached_atom ATOMS[36] = { {"WM_TAKE_FOCUS", sizeof("WM_TAKE_FOCUS") - 1, &WM_TAKE_FOCUS}, {"Backlight", sizeof("Backlight") - 1, &Backlight}, {"BACKLIGHT", sizeof("BACKLIGHT") - 1, &BACKLIGHT}, - {"_XROOTMAP_ID", sizeof("_XROOTMAP_ID") - 1, &_XROOTMAP_ID}, + {"_XROOTPMAP_ID", sizeof("_XROOTPMAP_ID") - 1, &_XROOTPMAP_ID}, {"_XSETROOT_ID", sizeof("_XSETROOT_ID") - 1, &_XSETROOT_ID}, {"ESETROOT_PMAP_ID", sizeof("ESETROOT_PMAP_ID") - 1, &ESETROOT_PMAP_ID}, {"_COMPTON_SHADOW", sizeof("_COMPTON_SHADOW") - 1, &_COMPTON_SHADOW}, diff --git a/src/x11/background_manager.cpp b/src/x11/background_manager.cpp index bd291443..e8e10642 100644 --- a/src/x11/background_manager.cpp +++ b/src/x11/background_manager.cpp @@ -81,9 +81,14 @@ void background_manager::fetch_root_pixmap() { try { if (!m_connection.root_pixmap(&pixmap, &pixmap_depth, &pixmap_geom)) { - free_resources(); - return m_log.err("background_manager: Failed to get root pixmap for background (realloc=%i)", realloc); + return m_log.warn("background_manager: Failed to get root pixmap, default to black (is there a wallpaper?)"); }; + m_log.trace("background_manager: root pixmap (%d:%d) %dx%d+%d+%d", pixmap, pixmap_depth, + pixmap_geom.width, pixmap_geom.height, pixmap_geom.x, pixmap_geom.y); + + if (pixmap_depth == 1 && pixmap_geom.width == 1 && pixmap_geom.height == 1) { + return m_log.err("background_manager: Cannot find root pixmap, try a different tool to set the desktop background"); + } for (auto it = m_slices.begin(); it != m_slices.end(); ) { auto slice = it->lock(); @@ -98,7 +103,7 @@ void background_manager::fetch_root_pixmap() { auto src_y = math_util::cap(translated->dst_y, pixmap_geom.y, int16_t(pixmap_geom.y + pixmap_geom.height)); auto w = math_util::cap(slice->m_rect.width, uint16_t(0), uint16_t(pixmap_geom.width - (src_x - pixmap_geom.x))); auto h = math_util::cap(slice->m_rect.height, uint16_t(0), uint16_t(pixmap_geom.height - (src_y - pixmap_geom.y))); - m_log.trace("background_manager: Copying from root pixmap (%d) %dx%d+%dx%d", pixmap, w, h, src_x, src_y); + m_log.trace("background_manager: Copying from root pixmap (%d:%d) %dx%d+%d+%d", pixmap, pixmap_depth, w, h, src_x, src_y); m_connection.copy_area_checked(pixmap, slice->m_pixmap, slice->m_gcontext, src_x, src_y, 0, 0, w, h); it++; @@ -119,15 +124,22 @@ void background_manager::fetch_root_pixmap() { void background_manager::handle(const evt::property_notify& evt) { // if there are no slices to observe, don't do anything - if(m_slices.empty()) return; + if(m_slices.empty()) { + return; + } - if (evt->atom == _XROOTMAP_ID || evt->atom == _XSETROOT_ID || evt->atom == ESETROOT_PMAP_ID) { + if (evt->atom == _XROOTPMAP_ID || evt->atom == _XSETROOT_ID || evt->atom == ESETROOT_PMAP_ID) { fetch_root_pixmap(); m_sig.emit(signals::ui::update_background()); } } bool background_manager::on(const signals::ui::update_geometry&) { + // if there are no slices to observe, don't do anything + if(m_slices.empty()) { + return false; + } + fetch_root_pixmap(); m_sig.emit(signals::ui::update_background()); return false; @@ -159,8 +171,9 @@ void bg_slice::allocate_resources(const logger& log, xcb_visualtype_t* visual) { if(m_gcontext == XCB_NONE) { log.trace("background_manager: Allocating graphics context"); - unsigned int mask = XCB_GC_GRAPHICS_EXPOSURES; - unsigned int value_list[1] = {0}; + auto black_pixel = m_connection.screen()->black_pixel; + unsigned int mask = XCB_GC_GRAPHICS_EXPOSURES | XCB_GC_FOREGROUND | XCB_GC_BACKGROUND; + unsigned int value_list[3] = {black_pixel, black_pixel, 0}; m_gcontext = m_connection.generate_id(); m_connection.create_gc(m_gcontext, m_pixmap, mask, value_list); } @@ -169,6 +182,10 @@ void bg_slice::allocate_resources(const logger& log, xcb_visualtype_t* visual) { log.trace("background_manager: Allocating cairo surface"); m_surface = make_unique(m_connection, m_pixmap, visual, m_rect.width, m_rect.height); } + + // fill (to provide a default in the case that fetching the background fails) + xcb_rectangle_t rect{0, 0, m_rect.width, m_rect.height}; + m_connection.poly_fill_rectangle(m_pixmap, m_gcontext, 1, &rect); } void bg_slice::free_resources() { diff --git a/src/x11/connection.cpp b/src/x11/connection.cpp index fc4d57fc..4862ec95 100644 --- a/src/x11/connection.cpp +++ b/src/x11/connection.cpp @@ -182,29 +182,40 @@ xcb_visualtype_t* connection::visual_type_for_id(xcb_screen_t* screen, xcb_visua * Query root window pixmap */ bool connection::root_pixmap(xcb_pixmap_t* pixmap, int* depth, xcb_rectangle_t* rect) { - const xcb_atom_t pixmap_properties[3]{ESETROOT_PMAP_ID, _XROOTMAP_ID, _XSETROOT_ID}; + *pixmap = XCB_NONE; // default value if getting the root pixmap fails + + /* + * We try multiple properties for the root pixmap here because I am not 100% sure + * if all programs set them the same way. We might be able to just use _XSETROOT_ID + * but keeping the other as fallback should not hurt (if it does, feel free to remove). + * + * see https://metacpan.org/pod/X11::Protocol::XSetRoot#ROOT-WINDOW-PROPERTIES for description of the properties + * the properties here are ordered by reliability: + * _XSETROOT_ID: this is usually a dummy 1x1 pixmap only for resource managment, use only as last resort + * ESETROOT_PMAP_ID: according to the link above, this should usually by equal to _XROOTPMAP_ID + * _XROOTPMAP_ID: this appears to be the "correct" property to use? if available, use this + */ + const xcb_atom_t pixmap_properties[3]{_XROOTPMAP_ID, ESETROOT_PMAP_ID, _XSETROOT_ID}; for (auto&& property : pixmap_properties) { try { auto prop = get_property(false, screen()->root, property, XCB_ATOM_PIXMAP, 0L, 1L); if (prop->format == 32 && prop->value_len == 1) { *pixmap = *prop.value().begin(); } - } catch (const exception& err) { - continue; - } - } - if (*pixmap) { - try { - auto geom = get_geometry(*pixmap); - *depth = geom->depth; - rect->width = geom->width; - rect->height = geom->height; - rect->x = geom->x; - rect->y = geom->y; - return true; + + if (*pixmap) { + auto geom = get_geometry(*pixmap); + *depth = geom->depth; + rect->width = geom->width; + rect->height = geom->height; + rect->x = geom->x; + rect->y = geom->y; + return true; + } } catch (const exception& err) { *pixmap = XCB_NONE; *rect = xcb_rectangle_t{0, 0, 0U, 0U}; + continue; } } return false; diff --git a/src/x11/tray_manager.cpp b/src/x11/tray_manager.cpp index e08c1c4a..2932868f 100644 --- a/src/x11/tray_manager.cpp +++ b/src/x11/tray_manager.cpp @@ -133,7 +133,8 @@ void tray_manager::setup(const bar_settings& bar_opts) { m_opts.background = bar_opts.background; } - if (color_util::alpha_channel(m_opts.background) != 1) { + if (color_util::alpha_channel(m_opts.background) != 255) { + m_log.trace("tray: enable transparency"); m_opts.transparent = true; } @@ -996,7 +997,7 @@ void tray_manager::handle(const evt::selection_clear& evt) { void tray_manager::handle(const evt::property_notify& evt) { if (!m_activated) { return; - } else if (evt->atom == _XROOTMAP_ID) { + } else if (evt->atom == _XROOTPMAP_ID) { redraw_window(true); } else if (evt->atom == _XSETROOT_ID) { redraw_window(true);