From kwin Thu Jan 30 12:42:34 2014 From: =?utf-8?q?Marek_Marczykowski-G=C3=B3recki?= Date: Thu, 30 Jan 2014 12:42:34 +0000 To: kwin Subject: Re: Review Request 115396: fix handling of window size hints with PResizeInc Message-Id: <20140130124234.32672.72974 () probe ! kde ! org> X-MARC-Message: https://marc.info/?l=kwin&m=139108576830486 MIME-Version: 1 Content-Type: multipart/mixed; boundary="--===============7883052291759275269==" --===============7883052291759275269== Content-Type: multipart/alternative; boundary="===============7837984771786509249==" --===============7837984771786509249== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit > On Jan. 30, 2014, 11:30 a.m., Martin Gräßlin wrote: > > I tried to create a test application demonstrating the behavior using the values from https://bugzilla.gnome.org/attachment.cgi?id=193285 but with pure xcb, KWin/5 does not show this problem. > > > > It would probably help if I could see the xprop of the affected windows. Unfortunately I don't have any gtk development environment set up, so I cannot just test myself. > > > > For reference the test code I used: > > #include > > #include > > #include > > > > int main(int, char **) > > { > > int screenNumber; > > xcb_connection_t *c = xcb_connect(nullptr, &screenNumber); > > > > auto getScreen = [=]() { > > const xcb_setup_t *setup = xcb_get_setup(c); > > auto it = xcb_setup_roots_iterator (setup); > > for (int i = 0; i < screenNumber; ++i) { > > xcb_screen_next(&it); > > } > > return it.data; > > }; > > xcb_screen_t *screen = getScreen(); > > > > xcb_window_t w = xcb_generate_id(c); > > const uint32_t values[2] = {screen->white_pixel, XCB_EVENT_MASK_BUTTON_PRESS | XCB_EVENT_MASK_BUTTON_RELEASE}; > > > > xcb_create_window(c, 0, w, screen->root, 0, 0, 80, 36, 0, XCB_WINDOW_CLASS_INPUT_OUTPUT, > > screen->root_visual, XCB_CW_BACK_PIXEL | XCB_CW_EVENT_MASK, values); > > > > // set the normal hints > > xcb_size_hints_t hints; > > hints.flags = XCB_ICCCM_SIZE_HINT_P_MIN_SIZE | XCB_ICCCM_SIZE_HINT_BASE_SIZE | XCB_ICCCM_SIZE_HINT_P_RESIZE_INC; > > hints.min_width = 8; > > hints.min_height = 15; > > hints.base_width = 0; > > hints.base_height = 0; > > hints.width_inc = 8; > > hints.height_inc = 15; > > xcb_icccm_set_wm_normal_hints(c, w, &hints); > > > > // and map the window > > xcb_map_window(c, w); > > xcb_flush(c); > > > > while (xcb_generic_event_t *event = xcb_wait_for_event(c)) { > > bool exit = false; > > if ((event->response_type & ~0x80) == XCB_BUTTON_RELEASE) { > > exit = true; > > } > > free(event); > > if (exit) { > > break; > > } > > } > > > > xcb_disconnect(c); > > return 0; > > } > > > > Thomas Lübking wrote: > The Problem is that the gnome terminal class (also powers emacs and gvim - famous bugs ;-) tries to play WM and "fixes" the size set by the WM. > The KWin code is ultra-twisted and the comments do not match exactly what is done. > Also the code seems to stem from fvwm -> KWM -> KWin =) > > Atm, the code seems correct (while reads wrong) > KWin merges base_width into min_width and uses min_width as base_width - this seems due to the different demands on aspect ratio calculation, where the claim is: > > // According to ICCCM 4.1.2.3 PMinSize should be a fallback for PBaseSize for size increments, > // but not for aspect ratio. > > I could not verify this statement, but *that* is what causes the divergence and needs to be sorted out. > > Marek Marczykowski-Górecki wrote: > The problematic case is when application provides both min size and base size - then KDE uses only min size for ResizeInc handling and ignores base size. The code: > if (xSizeHint.flags & PBaseSize) { > // PBaseSize is a fallback for PMinSize according to ICCCM 4.1.2.3 > // The other way around PMinSize is not a complete fallback for PBaseSize, > // so that's not handled here. > if (!(xSizeHint.flags & PMinSize)) { > xSizeHint.min_width = xSizeHint.base_width; > xSizeHint.min_height = xSizeHint.base_height; > } > (...) > int basew_inc = xSizeHint.min_width; // see getWmNormalHints() > int baseh_inc = xSizeHint.min_height; > w = int((w - basew_inc) / width_inc) * width_inc + basew_inc; > h = int((h - baseh_inc) / height_inc) * height_inc + baseh_inc; > > > Aspect ratio code is ok (which uses directly xSizeHint.base_*), only ResizeInc handling is wrong. > > Yes, that gnome terminal "behaviour" is also a bug, but separate one, not the only one. > > BTW hints of affected window: > WM_NORMAL_HINTS(WM_SIZE_HINTS): > program specified minimum size: 356 by 104 > program specified resize increment: 9 by 18 > program specified base size: 15 by 64 > window gravity: NorthWest > > > Martin Gräßlin wrote: > @Thomas: I just verified and the comment is correct > > I just tried with the values from the provided size hints and the problem is not reproducable. Thus the shrinking is related to client "playing WM". Yes, only in combination with clients "playing with WM" (which is separate bug, gnome-terminal this time) it causes window shrink, in all other cases, window simply will gets wrong size. Every sane application should simply accept those sizes, but still - there are inconsistent with hints because of bug, not because of some real need of WM. For example window with: WM_NORMAL_HINTS(WM_SIZE_HINTS): program specified minimum size: 352 by 104 program specified resize increment: 9 by 18 program specified base size: 15 by 64 Can get size 775x320, which is (352 + 47*9) x (104 + 12*18), but not in form of (15 + i*9) x (64 + j*18). To reproduce this bug, simply install and run gnome-terminal under KDE, it doesn't require whole Gnome installed. - Marek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115396/#review48596 ----------------------------------------------------------- On Jan. 30, 2014, 5:56 a.m., Marek Marczykowski-Górecki wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/115396/ > ----------------------------------------------------------- > > (Updated Jan. 30, 2014, 5:56 a.m.) > > > Review request for kwin. > > > Repository: kde-workspace > > > Description > ------- > > Use base_width/base_height if provided by the application and fallback to min_width/min_height only if not (according to ICCCM 4.1.2.3). > Previously always min_width/min_height was used (if provided), even if base_width/base_height was also provided. > > This fixes long standing bug with shrinking gnome-terminal window. Haven't found it in KDE Bugs, but described here: https://bugzilla.redhat.com/show_bug.cgi?id=707664 > > > Diffs > ----- > > kwin/geometry.cpp 6ae9666 > > Diff: https://git.reviewboard.kde.org/r/115396/diff/ > > > Testing > ------- > > Tested patched kde-workspace-4.10.5 (Fedora 18), I don't any significant changes until now. > Patched version allows normal gnome-terminal use, previously it was shrinking to minimal size. > > > Thanks, > > Marek Marczykowski-Górecki > > --===============7837984771786509249== Content-Type: text/html; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit
This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115396/

On January 30th, 2014, 11:30 a.m. UTC, Martin Gräßlin wrote:

I tried to create a test application demonstrating the behavior using the values from https://bugzilla.gnome.org/attachment.cgi?id=193285 but with pure xcb, KWin/5 does not show this problem.

It would probably help if I could see the xprop of the affected windows. Unfortunately I don't have any gtk development environment set up, so I cannot just test myself.

For reference the test code I used:
#include <stdlib.h>
#include <xcb/xcb.h>
#include <xcb/xcb_icccm.h>

int main(int, char **)
{
    int screenNumber;
    xcb_connection_t *c = xcb_connect(nullptr, &screenNumber);

    auto getScreen = [=]() {
        const xcb_setup_t *setup = xcb_get_setup(c);
        auto it = xcb_setup_roots_iterator (setup);
        for (int i = 0; i < screenNumber; ++i) {
            xcb_screen_next(&it);
        }
        return it.data;
    };
    xcb_screen_t *screen = getScreen();

    xcb_window_t w = xcb_generate_id(c);
    const uint32_t values[2] = {screen->white_pixel, XCB_EVENT_MASK_BUTTON_PRESS | XCB_EVENT_MASK_BUTTON_RELEASE};

    xcb_create_window(c, 0, w, screen->root, 0, 0, 80, 36, 0, XCB_WINDOW_CLASS_INPUT_OUTPUT,
                      screen->root_visual, XCB_CW_BACK_PIXEL | XCB_CW_EVENT_MASK, values);

    // set the normal hints
    xcb_size_hints_t hints;
    hints.flags = XCB_ICCCM_SIZE_HINT_P_MIN_SIZE | XCB_ICCCM_SIZE_HINT_BASE_SIZE | XCB_ICCCM_SIZE_HINT_P_RESIZE_INC;
    hints.min_width = 8;
    hints.min_height = 15;
    hints.base_width = 0;
    hints.base_height = 0;
    hints.width_inc = 8;
    hints.height_inc = 15;
    xcb_icccm_set_wm_normal_hints(c, w, &hints);

    // and map the window
    xcb_map_window(c, w);
    xcb_flush(c);

    while (xcb_generic_event_t *event = xcb_wait_for_event(c)) {
        bool exit = false;
        if ((event->response_type & ~0x80) == XCB_BUTTON_RELEASE) {
            exit = true;
        }
        free(event);
        if (exit) {
            break;
        }
    }

    xcb_disconnect(c);
    return 0;
}

On January 30th, 2014, 11:49 a.m. UTC, Thomas Lübking wrote:

The Problem is that the gnome terminal class (also powers emacs and gvim - famous bugs ;-) tries to play WM and "fixes" the size set by the WM.
The KWin code is ultra-twisted and the comments do not match exactly what is done.
Also the code seems to stem from fvwm -> KWM -> KWin =)

Atm, the code seems correct (while reads wrong)
KWin merges base_width into min_width and uses min_width as base_width - this seems due to the different demands on aspect ratio calculation, where the claim is:

// According to ICCCM 4.1.2.3 PMinSize should be a fallback for PBaseSize for size increments,
// but not for aspect ratio.

I could not verify this statement, but *that* is what causes the divergence and needs to be sorted out.

On January 30th, 2014, 12:04 p.m. UTC, Marek Marczykowski-Górecki wrote:

The problematic case is when application provides both min size and base size - then KDE uses only min size for ResizeInc handling and ignores base size. The code:
    if (xSizeHint.flags & PBaseSize) {
        // PBaseSize is a fallback for PMinSize according to ICCCM 4.1.2.3
        // The other way around PMinSize is not a complete fallback for PBaseSize,
        // so that's not handled here.
        if (!(xSizeHint.flags & PMinSize)) {
            xSizeHint.min_width = xSizeHint.base_width;
            xSizeHint.min_height = xSizeHint.base_height;
        }
(...)
    int basew_inc = xSizeHint.min_width; // see getWmNormalHints()
    int baseh_inc = xSizeHint.min_height;
    w = int((w - basew_inc) / width_inc) * width_inc + basew_inc;
    h = int((h - baseh_inc) / height_inc) * height_inc + baseh_inc;


Aspect ratio code is ok (which uses directly xSizeHint.base_*), only ResizeInc handling is wrong.

Yes, that gnome terminal "behaviour" is also a bug, but separate one, not the only one. 

BTW hints of affected window:
WM_NORMAL_HINTS(WM_SIZE_HINTS):
		program specified minimum size: 356 by 104
		program specified resize increment: 9 by 18
		program specified base size: 15 by 64
		window gravity: NorthWest

On January 30th, 2014, 12:26 p.m. UTC, Martin Gräßlin wrote:

@Thomas: I just verified and the comment is correct

I just tried with the values from the provided size hints and the problem is not reproducable. Thus the shrinking is related to client "playing WM".
Yes, only in combination with clients "playing with WM" (which is separate bug, gnome-terminal this time) it causes window shrink, in all other cases, window simply will gets wrong size. Every sane application should simply accept those sizes, but still - there are inconsistent with hints because of bug, not because of some real need of WM.

For example window with:
WM_NORMAL_HINTS(WM_SIZE_HINTS):
		program specified minimum size: 352 by 104
		program specified resize increment: 9 by 18
		program specified base size: 15 by 64

Can get size 775x320, which is (352 + 47*9) x (104 + 12*18), but not in form of (15 + i*9) x (64 + j*18).

To reproduce this bug, simply install and run gnome-terminal under KDE, it doesn't require whole Gnome installed.

- Marek


On January 30th, 2014, 5:56 a.m. UTC, Marek Marczykowski-Górecki wrote:

Review request for kwin.
By Marek Marczykowski-Górecki.

Updated Jan. 30, 2014, 5:56 a.m.

Repository: kde-workspace

Description

Use base_width/base_height if provided by the application and fallback to min_width/min_height only if not (according to ICCCM 4.1.2.3).
Previously always min_width/min_height was used (if provided), even if base_width/base_height was also provided.

This fixes long standing bug with shrinking gnome-terminal window. Haven't found it in KDE Bugs, but described here: https://bugzilla.redhat.com/show_bug.cgi?id=707664

Testing

Tested patched kde-workspace-4.10.5 (Fedora 18), I don't any significant changes until now.
Patched version allows normal gnome-terminal use, previously it was shrinking to minimal size.

Diffs

  • kwin/geometry.cpp (6ae9666)

View Diff

--===============7837984771786509249==-- --===============7883052291759275269== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ kwin mailing list kwin@kde.org https://mail.kde.org/mailman/listinfo/kwin --===============7883052291759275269==--