LemonBoy / bar

A featherweight, lemon-scented, bar based on xcb
MIT License
1.63k stars 191 forks source link

`-g` doesn't work as expected #163

Open vain opened 8 years ago

vain commented 8 years ago

I'm having trouble with -g. To illustrate, I added the following call to fprintf():

diff --git a/lemonbar.c b/lemonbar.c
index cfd4d9b..d5c1d55 100644
--- a/lemonbar.c
+++ b/lemonbar.c
@@ -752,6 +752,8 @@ monitor_new (int x, int y, int width, int height)
     ret->next = ret->prev = NULL;
     ret->window = xcb_generate_id(c);

+    fprintf(stderr, "creating window at %d %d, %d %d\n", ret->x, ret->y, ret->width, bh);
+
     int depth = (visual == scr->root_visual) ? XCB_COPY_FROM_PARENT : 32;
     xcb_create_window(c, depth, ret->window, scr->root,
             ret->x, ret->y, width, bh, 0,

Now, let's say my screen setup is as follows:

HDMI1 connected primary 1024x768+0+0
VGA1 connected 1280x1024+1024+0

It seems to be important that the screen on the right is larger in height than the first screen.

Let's run lemonbar:

$ ./lemonbar -dp -B '#ff0000' -g 1280x1+1024+768
creating window at 1024 768, 1280 1

This works as expected. I get a red line on the second monitor.

Now let's increase the y coordinate by one pixel:

$ ./lemonbar -dp -B '#ff0000' -g 1280x1+1024+769
creating window at 2048 769, 256 1

That's a little strange, isn't it? y and bh are correct, but x and w are wrong.

I can get the bar to show up at the desired coordinates by setting x to 0:

$ ./lemonbar -dp -B '#ff0000' -g 1280x1+0+769
creating window at 1024 769, 1280 1

I'm sorry that I can't provide a patch yet. I don't fully understand what's going on in monitor_create_chain().

Do you have any ideas?

Thank you!

neeasade commented 8 years ago

@vain, I have some scattered notes on this here - https://github.com/LemonBoy/bar/issues/147, was also getting a bit of weirdness, and ended up with https://github.com/LemonBoy/bar/pull/150 to change some scenarios.

possibly not relevant but might be.

vain commented 8 years ago

@neeasade, yeah appears to be related. Maybe it's the same issue.

What bothers/irritates me is that lemonbar still uses xrandr or xinerama and all kinds of magic, even though I specified a complete geometry string. Why not bypass monitor_create_chain() completely? Something among these lines:

diff --git a/lemonbar.c b/lemonbar.c
index cfd4d9b..76f1eba 100644
--- a/lemonbar.c
+++ b/lemonbar.c
@@ -92,6 +92,7 @@ static int font_index = -1;
 static uint32_t attrs = 0;
 static bool dock = false;
 static bool topbar = true;
+static bool fixed_geom = false;
 static int bw = -1, bh = -1, bx = 0, by = 0;
 static int bu = 1; // Underline height
 static rgba_t fgc, bgc, ugc;
@@ -747,14 +748,17 @@ monitor_new (int x, int y, int width, int height)
     }

     ret->x = x;
-    ret->y = (topbar ? by : height - bh - by) + y;
+    if (fixed_geom)
+        ret->y = y;
+    else
+        ret->y = (topbar ? by : height - bh - by) + y;
     ret->width = width;
     ret->next = ret->prev = NULL;
     ret->window = xcb_generate_id(c);

     int depth = (visual == scr->root_visual) ? XCB_COPY_FROM_PARENT : 32;
     xcb_create_window(c, depth, ret->window, scr->root,
-            ret->x, ret->y, width, bh, 0,
+            ret->x, ret->y, width, (fixed_geom ? height : bh), 0,
             XCB_WINDOW_CLASS_INPUT_OUTPUT, visual,
             XCB_CW_BACK_PIXEL | XCB_CW_BORDER_PIXEL | XCB_CW_OVERRIDE_REDIRECT | XCB_CW_EVENT_MASK | XCB_CW_COLORMAP,
             (const uint32_t []){ bgc.v, bgc.v, dock, XCB_EVENT_MASK_EXPOSURE | XCB_EVENT_MASK_BUTTON_PRESS, colormap });
@@ -1012,7 +1016,7 @@ bool
 parse_geometry_string (char *str, int *tmp)
 {
     char *p = str;
-    int i = 0, j;
+    int i = 0, j, k = 0;

     if (!str || !str[0])
         return false;
@@ -1053,8 +1057,15 @@ parse_geometry_string (char *str, int *tmp)
             return false;
         }
         tmp[i] = j;
+        k++;
     }

+    // All four fields have been specified by the user, thus inherit
+    // xinerama/xrandr and just create one window at the given
+    // coordinates.
+    if (k == 4)
+        fixed_geom = true;
+
     return true;
 }

@@ -1104,23 +1115,27 @@ init (char *wm_name)
     // Initialize monitor list head and tail
     monhead = montail = NULL;

-    // Check if RandR is present
-    qe_reply = xcb_get_extension_data(c, &xcb_randr_id);
-
-    if (qe_reply && qe_reply->present) {
-        get_randr_monitors();
+    if (fixed_geom) {
+        monitor_add(monitor_new(bx, by, bw, bh));
     } else {
-        qe_reply = xcb_get_extension_data(c, &xcb_xinerama_id);
+        // Check if RandR is present
+        qe_reply = xcb_get_extension_data(c, &xcb_randr_id);

-        // Check if Xinerama extension is present and active
         if (qe_reply && qe_reply->present) {
-            xcb_xinerama_is_active_reply_t *xia_reply;
-            xia_reply = xcb_xinerama_is_active_reply(c, xcb_xinerama_is_active(c), NULL);
+            get_randr_monitors();
+        } else {
+            qe_reply = xcb_get_extension_data(c, &xcb_xinerama_id);
+
+            // Check if Xinerama extension is present and active
+            if (qe_reply && qe_reply->present) {
+                xcb_xinerama_is_active_reply_t *xia_reply;
+                xia_reply = xcb_xinerama_is_active_reply(c, xcb_xinerama_is_active(c), NULL);

-            if (xia_reply && xia_reply->state)
-                get_xinerama_monitors();
+                if (xia_reply && xia_reply->state)
+                    get_xinerama_monitors();

-            free(xia_reply);
+                free(xia_reply);
+            }
         }
     }

Now, when the user specifies all four fields of a geometry string, lemonbar creates exactly one window at those exact coordinates (relative to the root window). This is what I would expect to happen.

What do you think? Am I missing something?

neeasade commented 8 years ago

lemonbar creates exactly one window at those exact coordinates (relative to the root window). This is what I would expect to happen.

This is the behavior I expected and wasn't seeing and was trying to fix (and half fixed), but I'm not confident in my C/savviness here.

maybe the multihead stuff is needed for default lemonbar position on uneven monitor setup? currently on launch it will be across all monitors regardless of placement if the -g flag is not set, which is nice -- but you're right, if the -g flag is used and it's getting in the way, ¯_(ツ)_/¯

I will test your patch above this evening.

pockata commented 8 years ago

+1 for absolute coordinates.

The tools I use return coordinates that are relative to the root window. It took me a while to double (and triple) check my coordinate calculation before I found out that lemonbar uses elaborate guesswork on where to show the bar instead of the coordinates exactly as they are specified. If you haven't looked at the source code first, this behavior is somewhat confusing (it's not documented in the readme, or at least I couldn't find it).

I will test the patch and send feedback soon.

LemonBoy commented 7 years ago

The Xinerama/Xrandr magic is used because the bar is replicated on all the monitors and each monitor gets its own window. The -g command tries to break down the desired geometry into monitor-sized bites and place them in the (hopefully) right place with the obvious caveat that there are so many layouts in the wild that the code doesn't handle (patches are more than welcome).

vendion commented 7 years ago

I think I'm running into this as well, I jump between a single display, dual display, and triple display setup depending on where I'm at (I have different monitor setups at work and home) but regardless of what the active setup I have I want my bar to always be on my laptop's internal display. I'm able to return display geometry from my window manager including x and y offsets like the following:

~/.config/herbstluftwm ❱ hc list_monitors
0: 3840x2160+2520+0 with tag "Netz" <- laptop display and where I want my bar
1: 2520x1575+0+585 with tag "Befehlszeile" [FOCUS] <- left most monitor so this is where lemonbar gets displayed
2: 2520x1575+6360+585 with tag "Entwicklung"

Grabbing the x offset of 2520 for my internal display I use it with the -g flag of lemonbar like

lemonbar -dp -g 3840x40x2520x0 -B#CFCFD0 -F#0E0E0E -fRoboto Slab:size=10 -fFontAwesome:size=10 -n bar

but it's almost as if the x offset value is being ignored.

pockata commented 4 years ago

I have a few different monitor configurations that I'm using with my laptop (home and work configs) and lemonbar doesn't handle them all too graciously even though I'm supplying all four fields of the geometry string.

 Variant 1 (work)     Variant 2 (home)     Variant 3 (home + eGPU)
+--------------+     +--------------+     +--------------+ +-------+
| 1920 x 1080* |     | 1920 x 1080* |     | 1920 x 1080* | | 1080  |
|              |     |              |     |              | |  x    |
+--------------+     +--------------+     +--------------+ | 1920  |
   +--------+        +--------------+     +--------------+ |       |
   | 1680 x |        | 1920 x 1080  |     | 1920 x 1080  | +-------+
   | 1050   |        |              |     |              | 
   +--------+        +--------------+     +--------------+ 

Only the top left monitor (marked with *) in each configuration shows lemonbar in the correct place. All other monitors have lemonbar shown in the wrong place. On some of them it's even positioned outside of the monitors (not visible).

@vain's patch works perfectly. In all my confugurations lemonbar is in the correct place. I think this should be the default behavior when supplying the full geometry parameter string to the bar.

For convenience's sake I've updated the patch to work with the current master branch (the original patch doesn't apply due to recent changes in master).

Click here to see patch ```diff diff --git i/lemonbar.c w/lemonbar.c index 9220af0..7cde5ff 100644 --- i/lemonbar.c +++ w/lemonbar.c @@ -95,6 +95,7 @@ static int font_index = -1; static uint32_t attrs = 0; static bool dock = false; static bool topbar = true; +static bool fixed_geom = false; static int bw = -1, bh = -1, bx = 0, by = 0; static int bu = 1; // Underline height static rgba_t fgc, bgc, ugc; @@ -784,14 +785,17 @@ monitor_new (int x, int y, int width, int height) } ret->x = x; - ret->y = (topbar ? by : height - bh - by) + y; + if (fixed_geom) + ret->y = y; + else + ret->y = (topbar ? by : height - bh - by) + y; ret->width = width; ret->next = ret->prev = NULL; ret->window = xcb_generate_id(c); int depth = (visual == scr->root_visual) ? XCB_COPY_FROM_PARENT : 32; xcb_create_window(c, depth, ret->window, scr->root, - ret->x, ret->y, width, bh, 0, + ret->x, ret->y, width, (fixed_geom ? height : bh), 0, XCB_WINDOW_CLASS_INPUT_OUTPUT, visual, XCB_CW_BACK_PIXEL | XCB_CW_BORDER_PIXEL | XCB_CW_OVERRIDE_REDIRECT | XCB_CW_EVENT_MASK | XCB_CW_COLORMAP, (const uint32_t []){ bgc.v, bgc.v, dock, XCB_EVENT_MASK_EXPOSURE | XCB_EVENT_MASK_BUTTON_PRESS, colormap }); @@ -1051,7 +1055,7 @@ bool parse_geometry_string (char *str, int *tmp) { char *p = str; - int i = 0, j; + int i = 0, j, k = 0; if (!str || !str[0]) return false; @@ -1092,8 +1096,15 @@ parse_geometry_string (char *str, int *tmp) return false; } tmp[i] = j; + k++; } + // All four fields have been specified by the user, thus inherit + // xinerama/xrandr and just create one window at the given + // coordinates. + if (k == 4) + fixed_geom = true; + return true; } @@ -1143,28 +1154,33 @@ init (char *wm_name) // Initialize monitor list head and tail monhead = montail = NULL; - // Check if RandR is present - qe_reply = xcb_get_extension_data(c, &xcb_randr_id); - - if (qe_reply && qe_reply->present) { - get_randr_monitors(); + if (fixed_geom) { + monitor_add(monitor_new(bx, by, bw, bh)); } -#if WITH_XINERAMA else { - qe_reply = xcb_get_extension_data(c, &xcb_xinerama_id); + // Check if RandR is present + qe_reply = xcb_get_extension_data(c, &xcb_randr_id); - // Check if Xinerama extension is present and active if (qe_reply && qe_reply->present) { - xcb_xinerama_is_active_reply_t *xia_reply; - xia_reply = xcb_xinerama_is_active_reply(c, xcb_xinerama_is_active(c), NULL); + get_randr_monitors(); + } +#if WITH_XINERAMA + else { + qe_reply = xcb_get_extension_data(c, &xcb_xinerama_id); + + // Check if Xinerama extension is present and active + if (qe_reply && qe_reply->present) { + xcb_xinerama_is_active_reply_t *xia_reply; + xia_reply = xcb_xinerama_is_active_reply(c, xcb_xinerama_is_active(c), NULL); - if (xia_reply && xia_reply->state) - get_xinerama_monitors(); + if (xia_reply && xia_reply->state) + get_xinerama_monitors(); - free(xia_reply); + free(xia_reply); + } } - } #endif + } if (!monhead) { // If I fits I sits ```

I've also provided a patch for krypt-n/bar if someone's interested.

Click here to see patch ```diff diff --git i/lemonbar.c w/lemonbar.c index b5af2f3..468385d 100644 --- i/lemonbar.c +++ w/lemonbar.c @@ -115,6 +115,7 @@ static int offset_y_index = 0; static uint32_t attrs = 0; static bool dock = false; static bool topbar = true; +static bool fixed_geom = false; static int bw = -1, bh = -1, bx = 0, by = 0; static int bu = 1; // Underline height static rgba_t fgc, bgc, ugc; @@ -911,13 +912,16 @@ monitor_new (int x, int y, int width, int height) } ret->x = x; - ret->y = (topbar ? by : height - bh - by) + y; + if (fixed_geom) + ret->y = y; + else + ret->y = (topbar ? by : height - bh - by) + y; ret->width = width; ret->next = ret->prev = NULL; ret->window = xcb_generate_id(c); int depth = (visual == scr->root_visual) ? XCB_COPY_FROM_PARENT : 32; xcb_create_window(c, depth, ret->window, scr->root, - ret->x, ret->y, width, bh, 0, + ret->x, ret->y, width, (fixed_geom ? height : bh), 0, XCB_WINDOW_CLASS_INPUT_OUTPUT, visual, XCB_CW_BACK_PIXEL | XCB_CW_BORDER_PIXEL | XCB_CW_OVERRIDE_REDIRECT | XCB_CW_EVENT_MASK | XCB_CW_COLORMAP, (const uint32_t []) { @@ -1177,7 +1181,7 @@ bool parse_geometry_string (char *str, int *tmp) { char *p = str; - int i = 0, j; + int i = 0, j, k = 0; if (!str || !str[0]) return false; @@ -1218,8 +1222,15 @@ parse_geometry_string (char *str, int *tmp) return false; } tmp[i] = j; + k++; } + // All four fields have been specified by the user, thus inherit + // xinerama/xrandr and just create one window at the given + // coordinates. + if (k == 4) + fixed_geom = true; + return true; } @@ -1277,28 +1288,33 @@ init (char *wm_name, char *wm_instance) // Initialize monitor list head and tail monhead = montail = NULL; - // Check if RandR is present - qe_reply = xcb_get_extension_data(c, &xcb_randr_id); - - if (qe_reply && qe_reply->present) { - get_randr_monitors(); + if (fixed_geom) { + monitor_add(monitor_new(bx, by, bw, bh)); } -#if WITH_XINERAMA else { - qe_reply = xcb_get_extension_data(c, &xcb_xinerama_id); + // Check if RandR is present + qe_reply = xcb_get_extension_data(c, &xcb_randr_id); - // Check if Xinerama extension is present and active if (qe_reply && qe_reply->present) { - xcb_xinerama_is_active_reply_t *xia_reply; - xia_reply = xcb_xinerama_is_active_reply(c, xcb_xinerama_is_active(c), NULL); + get_randr_monitors(); + } +#if WITH_XINERAMA + else { + qe_reply = xcb_get_extension_data(c, &xcb_xinerama_id); + + // Check if Xinerama extension is present and active + if (qe_reply && qe_reply->present) { + xcb_xinerama_is_active_reply_t *xia_reply; + xia_reply = xcb_xinerama_is_active_reply(c, xcb_xinerama_is_active(c), NULL); - if (xia_reply && xia_reply->state) - get_xinerama_monitors(); + if (xia_reply && xia_reply->state) + get_xinerama_monitors(); - free(xia_reply); + free(xia_reply); + } } - } #endif + } if (!monhead) { // If I fits I sits ```