Fix size hints for x11-ssh-askpass

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Fix size hints for x11-ssh-askpass

Alexander Hall
Hi,

Recently, my window manager (i3) started making the ssh-askpass windows
too small to be really usable. The problem seems to be that the size
hints indicate that it provides a width and height, while those fields
are set to 0. While looking at this, the same seems to be the case for
position (x and y are 0). AFAICS, both of these hints are obsolete.

The diff below (even without the PPosition removal, but I added that
for good measure) fixes this, at least for i3.

Should this rather be taken upstream, if any, if it's not already
there, in which case it might already be fixed?

Anyway, the diff follows. Please test on your favourite window manager.

OK? Comments?

/Alexander


Index: x11-ssh-askpass.c
===================================================================
RCS file: /cvs/xenocara/app/ssh-askpass/x11-ssh-askpass.c,v
retrieving revision 1.6
diff -u -p -r1.6 x11-ssh-askpass.c
--- x11-ssh-askpass.c 24 Apr 2015 02:19:41 -0000 1.6
+++ x11-ssh-askpass.c 27 Dec 2015 18:52:55 -0000
@@ -772,8 +772,6 @@ void createDialogWindow(AppInfo *app)
       outOfMemory(app, __LINE__);
    }
    d->sizeHints->flags = 0;
-   d->sizeHints->flags |= PPosition;
-   d->sizeHints->flags |= PSize;
    d->sizeHints->min_width = d->w3.w.width;
    d->sizeHints->min_height = d->w3.w.height;
    d->sizeHints->flags |= PMinSize;

x11-ssh-askpass_size_hints.diff (659 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Fix size hints for x11-ssh-askpass

Theo Buehler
On Sun, Dec 27, 2015 at 08:28:36PM +0100, Alexander Hall wrote:

> Hi,
>
> Recently, my window manager (i3) started making the ssh-askpass windows
> too small to be really usable. The problem seems to be that the size
> hints indicate that it provides a width and height, while those fields
> are set to 0. While looking at this, the same seems to be the case for
> position (x and y are 0). AFAICS, both of these hints are obsolete.
>
> The diff below (even without the PPosition removal, but I added that
> for good measure) fixes this, at least for i3.
>
> Should this rather be taken upstream, if any, if it's not already
> there, in which case it might already be fixed?
>
> Anyway, the diff follows. Please test on your favourite window manager.
>
> OK? Comments?

I can confirm both the issue and that your patch fixes this for i3.
Thanks

> /Alexander
>
>
> Index: x11-ssh-askpass.c
> ===================================================================
> RCS file: /cvs/xenocara/app/ssh-askpass/x11-ssh-askpass.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 x11-ssh-askpass.c
> --- x11-ssh-askpass.c 24 Apr 2015 02:19:41 -0000 1.6
> +++ x11-ssh-askpass.c 27 Dec 2015 18:52:55 -0000
> @@ -772,8 +772,6 @@ void createDialogWindow(AppInfo *app)
>        outOfMemory(app, __LINE__);
>     }
>     d->sizeHints->flags = 0;
> -   d->sizeHints->flags |= PPosition;
> -   d->sizeHints->flags |= PSize;
>     d->sizeHints->min_width = d->w3.w.width;
>     d->sizeHints->min_height = d->w3.w.height;
>     d->sizeHints->flags |= PMinSize;

> Index: x11-ssh-askpass.c
> ===================================================================
> RCS file: /cvs/xenocara/app/ssh-askpass/x11-ssh-askpass.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 x11-ssh-askpass.c
> --- x11-ssh-askpass.c 24 Apr 2015 02:19:41 -0000 1.6
> +++ x11-ssh-askpass.c 27 Dec 2015 18:52:55 -0000
> @@ -772,8 +772,6 @@ void createDialogWindow(AppInfo *app)
>        outOfMemory(app, __LINE__);
>     }
>     d->sizeHints->flags = 0;
> -   d->sizeHints->flags |= PPosition;
> -   d->sizeHints->flags |= PSize;
>     d->sizeHints->min_width = d->w3.w.width;
>     d->sizeHints->min_height = d->w3.w.height;
>     d->sizeHints->flags |= PMinSize;

Reply | Threaded
Open this post in threaded view
|

Re: Fix size hints for x11-ssh-askpass

Alexander Hall
On Sun, Dec 27, 2015 at 08:44:35PM +0100, Theo Buehler wrote:

> On Sun, Dec 27, 2015 at 08:28:36PM +0100, Alexander Hall wrote:
> > Hi,
> >
> > Recently, my window manager (i3) started making the ssh-askpass windows
> > too small to be really usable. The problem seems to be that the size
> > hints indicate that it provides a width and height, while those fields
> > are set to 0. While looking at this, the same seems to be the case for
> > position (x and y are 0). AFAICS, both of these hints are obsolete.
> >
> > The diff below (even without the PPosition removal, but I added that
> > for good measure) fixes this, at least for i3.
> >
> > Should this rather be taken upstream, if any, if it's not already
> > there, in which case it might already be fixed?
> >
> > Anyway, the diff follows. Please test on your favourite window manager.
> >
> > OK? Comments?
>
> I can confirm both the issue and that your patch fixes this for i3.
> Thanks

I'll add that simple testing reveals no regression for the any of these
window managers:

  cwm
  fvwm
  mwm
  twm

/Alexander

Reply | Threaded
Open this post in threaded view
|

Re: Fix size hints for x11-ssh-askpass

Matthieu Herrb-7
In reply to this post by Alexander Hall
On Sun, Dec 27, 2015 at 08:28:36PM +0100, Alexander Hall wrote:
> Hi,
>
> Recently, my window manager (i3) started making the ssh-askpass windows
> too small to be really usable. The problem seems to be that the size
> hints indicate that it provides a width and height, while those fields
> are set to 0. While looking at this, the same seems to be the case for
> position (x and y are 0). AFAICS, both of these hints are obsolete.

x y width and height are marked obsolete because the values are taken
from the window parameters passed to XCreateWindow and not from here.

But the hints themselve are not obsolete. I would rather investigate
why i3 behaves wrong than just removing the placement and size hints.

It seems to me that it can come from  different reasons (I don't use
i3 so I've not taken the time to look at its code):

- it incorrectly tries to use the obsolete values, rather than the
  window parameters

- it completely dropped the implementation of the "old" ICCCM
  hints and only follows the extened wm hints. Since ssh-askpass
  doesn't handle those it gets random values...

- the computed window parameters are either wrong or interpreted
  wrong. This can be the case with Xinerama or pseudo-xinerama when
  both ssh-ask pass and the window manager are trying to center the
  window on a given monitor rather than having it centered globally
  (where it will end up being split across 2 monitors if they are of
  equal size).

>
> The diff below (even without the PPosition removal, but I added that
> for good measure) fixes this, at least for i3.

I would rather not remove those hints, even if other window manager
seem to behave correctly. Maybe it's needed to pass the correct values
for the obsolete fields. (ihmo it's still an i3 problem in that case,
but it's a more acceptable workaround). ie set them from the d->w3.w
structure.

Also if someone is interested, support for ewmh compliant window
managers could be added to ssh-askpass. It may help with complex
multi-monitor layouts.

>
> Should this rather be taken upstream, if any, if it's not already
> there, in which case it might already be fixed?

Upstream is no longer maintained as far as I know.

>
> Anyway, the diff follows. Please test on your favourite window manager.
>
> OK? Comments?
>
> /Alexander
>
>
> Index: x11-ssh-askpass.c
> ===================================================================
> RCS file: /cvs/xenocara/app/ssh-askpass/x11-ssh-askpass.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 x11-ssh-askpass.c
> --- x11-ssh-askpass.c 24 Apr 2015 02:19:41 -0000 1.6
> +++ x11-ssh-askpass.c 27 Dec 2015 18:52:55 -0000
> @@ -772,8 +772,6 @@ void createDialogWindow(AppInfo *app)
>        outOfMemory(app, __LINE__);
>     }
>     d->sizeHints->flags = 0;
> -   d->sizeHints->flags |= PPosition;
> -   d->sizeHints->flags |= PSize;
>     d->sizeHints->min_width = d->w3.w.width;
>     d->sizeHints->min_height = d->w3.w.height;
>     d->sizeHints->flags |= PMinSize;

> Index: x11-ssh-askpass.c
> ===================================================================
> RCS file: /cvs/xenocara/app/ssh-askpass/x11-ssh-askpass.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 x11-ssh-askpass.c
> --- x11-ssh-askpass.c 24 Apr 2015 02:19:41 -0000 1.6
> +++ x11-ssh-askpass.c 27 Dec 2015 18:52:55 -0000
> @@ -772,8 +772,6 @@ void createDialogWindow(AppInfo *app)
>        outOfMemory(app, __LINE__);
>     }
>     d->sizeHints->flags = 0;
> -   d->sizeHints->flags |= PPosition;
> -   d->sizeHints->flags |= PSize;
>     d->sizeHints->min_width = d->w3.w.width;
>     d->sizeHints->min_height = d->w3.w.height;
>     d->sizeHints->flags |= PMinSize;

--
Matthieu Herrb

signature.asc (476 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Fix size hints for x11-ssh-askpass

Alexander Hall
On Mon, Dec 28, 2015 at 08:10:49AM +0100, Matthieu Herrb wrote:

> On Sun, Dec 27, 2015 at 08:28:36PM +0100, Alexander Hall wrote:
> > Hi,
> >
> > Recently, my window manager (i3) started making the ssh-askpass windows
> > too small to be really usable. The problem seems to be that the size
> > hints indicate that it provides a width and height, while those fields
> > are set to 0. While looking at this, the same seems to be the case for
> > position (x and y are 0). AFAICS, both of these hints are obsolete.
>
> x y width and height are marked obsolete because the values are taken
> from the window parameters passed to XCreateWindow and not from here.
>
> But the hints themselve are not obsolete. I would rather investigate
> why i3 behaves wrong than just removing the placement and size hints.
>
> It seems to me that it can come from  different reasons (I don't use
> i3 so I've not taken the time to look at its code):
>
> - it incorrectly tries to use the obsolete values, rather than the
>   window parameters
>
> - it completely dropped the implementation of the "old" ICCCM
>   hints and only follows the extened wm hints. Since ssh-askpass
>   doesn't handle those it gets random values...
>
> - the computed window parameters are either wrong or interpreted
>   wrong. This can be the case with Xinerama or pseudo-xinerama when
>   both ssh-ask pass and the window manager are trying to center the
>   window on a given monitor rather than having it centered globally
>   (where it will end up being split across 2 monitors if they are of
>   equal size).
>
> >
> > The diff below (even without the PPosition removal, but I added that
> > for good measure) fixes this, at least for i3.
>
> I would rather not remove those hints, even if other window manager
> seem to behave correctly. Maybe it's needed to pass the correct values
> for the obsolete fields. (ihmo it's still an i3 problem in that case,
> but it's a more acceptable workaround). ie set them from the d->w3.w
> structure.

Thanks a lot for the detailed answer. This is obviously not the right
fix. I previously tried adding those fields and that helped too IIRC.
I'll have a look at the i3 sources.

/Alexander

>
> Also if someone is interested, support for ewmh compliant window
> managers could be added to ssh-askpass. It may help with complex
> multi-monitor layouts.
>
> >
> > Should this rather be taken upstream, if any, if it's not already
> > there, in which case it might already be fixed?
>
> Upstream is no longer maintained as far as I know.
>
> >
> > Anyway, the diff follows. Please test on your favourite window manager.
> >
> > OK? Comments?
> >
> > /Alexander
> >
> >
> > Index: x11-ssh-askpass.c
> > ===================================================================
> > RCS file: /cvs/xenocara/app/ssh-askpass/x11-ssh-askpass.c,v
> > retrieving revision 1.6
> > diff -u -p -r1.6 x11-ssh-askpass.c
> > --- x11-ssh-askpass.c 24 Apr 2015 02:19:41 -0000 1.6
> > +++ x11-ssh-askpass.c 27 Dec 2015 18:52:55 -0000
> > @@ -772,8 +772,6 @@ void createDialogWindow(AppInfo *app)
> >        outOfMemory(app, __LINE__);
> >     }
> >     d->sizeHints->flags = 0;
> > -   d->sizeHints->flags |= PPosition;
> > -   d->sizeHints->flags |= PSize;
> >     d->sizeHints->min_width = d->w3.w.width;
> >     d->sizeHints->min_height = d->w3.w.height;
> >     d->sizeHints->flags |= PMinSize;
>
> > Index: x11-ssh-askpass.c
> > ===================================================================
> > RCS file: /cvs/xenocara/app/ssh-askpass/x11-ssh-askpass.c,v
> > retrieving revision 1.6
> > diff -u -p -r1.6 x11-ssh-askpass.c
> > --- x11-ssh-askpass.c 24 Apr 2015 02:19:41 -0000 1.6
> > +++ x11-ssh-askpass.c 27 Dec 2015 18:52:55 -0000
> > @@ -772,8 +772,6 @@ void createDialogWindow(AppInfo *app)
> >        outOfMemory(app, __LINE__);
> >     }
> >     d->sizeHints->flags = 0;
> > -   d->sizeHints->flags |= PPosition;
> > -   d->sizeHints->flags |= PSize;
> >     d->sizeHints->min_width = d->w3.w.width;
> >     d->sizeHints->min_height = d->w3.w.height;
> >     d->sizeHints->flags |= PMinSize;
>
>
> --
> Matthieu Herrb


Reply | Threaded
Open this post in threaded view
|

Re: Fix size hints for x11-ssh-askpass

Alexander Hall
On Mon, Dec 28, 2015 at 11:42:00AM +0100, Alexander Hall wrote:

> On Mon, Dec 28, 2015 at 08:10:49AM +0100, Matthieu Herrb wrote:
> > On Sun, Dec 27, 2015 at 08:28:36PM +0100, Alexander Hall wrote:
> > > Hi,
> > >
> > > Recently, my window manager (i3) started making the ssh-askpass windows
> > > too small to be really usable. The problem seems to be that the size
> > > hints indicate that it provides a width and height, while those fields
> > > are set to 0. While looking at this, the same seems to be the case for
> > > position (x and y are 0). AFAICS, both of these hints are obsolete.
> >
> > x y width and height are marked obsolete because the values are taken
> > from the window parameters passed to XCreateWindow and not from here.
> >
> > But the hints themselve are not obsolete. I would rather investigate
> > why i3 behaves wrong than just removing the placement and size hints.
> >
> > It seems to me that it can come from  different reasons (I don't use
> > i3 so I've not taken the time to look at its code):
> >
> > - it incorrectly tries to use the obsolete values, rather than the
> >   window parameters

Yes that's what it seems like. See below.

> >
> > - it completely dropped the implementation of the "old" ICCCM
> >   hints and only follows the extened wm hints. Since ssh-askpass
> >   doesn't handle those it gets random values...
> >
> > - the computed window parameters are either wrong or interpreted
> >   wrong. This can be the case with Xinerama or pseudo-xinerama when
> >   both ssh-ask pass and the window manager are trying to center the
> >   window on a given monitor rather than having it centered globally
> >   (where it will end up being split across 2 monitors if they are of
> >   equal size).
> >
> > >
> > > The diff below (even without the PPosition removal, but I added that
> > > for good measure) fixes this, at least for i3.
> >
> > I would rather not remove those hints, even if other window manager
> > seem to behave correctly. Maybe it's needed to pass the correct values
> > for the obsolete fields. (ihmo it's still an i3 problem in that case,
> > but it's a more acceptable workaround). ie set them from the d->w3.w
> > structure.
>
> Thanks a lot for the detailed answer. This is obviously not the right
> fix. I previously tried adding those fields and that helped too IIRC.
> I'll have a look at the i3 sources.

Ok, I can see this in the i3 code:

    /* Plasma windows set their geometry in WM_SIZE_HINTS. */
    if ((wm_size_hints.flags & XCB_ICCCM_SIZE_HINT_US_POSITION || wm_size_hints.flags & XCB_ICCCM_SIZE_HINT_P_POSITION) &&
        (wm_size_hints.flags & XCB_ICCCM_SIZE_HINT_US_SIZE || wm_size_hints.flags & XCB_ICCCM_SIZE_HINT_P_SIZE)) {
        DLOG("We are setting geometry according to wm_size_hints x=%d y=%d w=%d h=%d\n",
             wm_size_hints.x, wm_size_hints.y, wm_size_hints.width, wm_size_hints.height);
        geom->x = wm_size_hints.x;
        geom->y = wm_size_hints.y;
        geom->width = wm_size_hints.width;
        geom->height = wm_size_hints.height;
    }  

which indeed indicate it's using the deprecated members, with no intention not to.

However, AFAICT, /usr/X11R6/include/xcb/xcb_icccm.h gives no
alternative, and as I'm not aiming to be an X hacker, ok to commit
the workaround, or is that only counterproductive in terms of i3?

/Alexander


Index: x11-ssh-askpass.c
===================================================================
RCS file: /cvs/xenocara/app/ssh-askpass/x11-ssh-askpass.c,v
retrieving revision 1.6
diff -u -p -r1.6 x11-ssh-askpass.c
--- x11-ssh-askpass.c 24 Apr 2015 02:19:41 -0000 1.6
+++ x11-ssh-askpass.c 28 Dec 2015 12:25:55 -0000
@@ -772,7 +772,11 @@ void createDialogWindow(AppInfo *app)
       outOfMemory(app, __LINE__);
    }
    d->sizeHints->flags = 0;
+   d->sizeHints->x = d->w3.w.x;
+   d->sizeHints->y = d->w3.w.y;
    d->sizeHints->flags |= PPosition;
+   d->sizeHints->width = d->w3.w.width;
+   d->sizeHints->height = d->w3.w.height;
    d->sizeHints->flags |= PSize;
    d->sizeHints->min_width = d->w3.w.width;
    d->sizeHints->min_height = d->w3.w.height;