Re: Possible patches from upstream source

[Kazunobu Kuriyama]

Adam Fedor wrote:

> See comments below
>
> On Jun 1, 2004, at 12:14 AM, Alex Perez wrote:
>
> > Cheers,
> > Alex Perez
> > --- src/actions.c    Mon Aug 11 15:29:31 2003
> > +++ src/actions.c    Mon Aug 11 15:31:56 2003
> > @@ -144,6 +144,14 @@
> >        old_scr=scr;
> >      old_focused=old_scr->focused_window;
> >
> > +    /*
> > +     * Safeguard: make sure the timestamp is monotonically increasing
> > +     * (very unlikely that this will be needed, still a safeguard)
> > +     */
> > +    if (timestamp <= LastFocusChange)
> > +        timestamp = LastFocusChange + 1;
> > +
> > +
> >      LastFocusChange = timestamp;
> >
> >  /*
> > @@ -150,8 +158,8 @@
> >   * This is a hack, because XSetInputFocus() should have a proper
> >   * timestamp instead of CurrentTime but it seems that some times
> >   * clients will not receive focus properly that way.
> > + */
> >      if (ignoreTimestamp)
> > -*/
> >        timestamp = CurrentTime;
> >
> >      if (old_focused)
>
>
> Seems like these correct aberrant X11 behavior, so they should be fine 
> with GNUstep. 

Since the global ignoreTimestamp is almost always set to zero (the
variable is set to non-zero only when processEvents() is called;
however, it immediately revers to zero before the function returns.
Thus, its value is effectively identical to 0), the local variable
timestamp would never be set to CurrentTime if the patch were applied.

> > --- src/event.c    Mon Aug 11 15:29:31 2003
> > +++ src/event.c    Mon Aug 11 15:32:06 2003
> > @@ -402,7 +402,6 @@
> >  static void
> >  saveTimestamp(XEvent *event)
> >  {
> > -    LastTimestamp = CurrentTime;
> >
> >      switch (event->type) {
> >       case ButtonRelease:
>
>
> Not sure. Seems harmless. 

If this were applied, LastTimestamp would not be updated for the
X events that are not specified in the case branches of the SWITCH
statement.

> > --- src/workspace.c    Mon Aug 11 15:29:33 2003
> > +++ src/workspace.c    Mon Aug 11 15:33:33 2003
> > @@ -574,10 +574,12 @@
> >                                &foo, &foo, &foo, &foo, &mask)) {
> >          tmp = wWindowFor(win);
> >          }
> > -        if (!tmp && wPreferences.focus_mode == WKF_SLOPPY) {
> > -        wSetFocusTo(scr, foc);
> > -        } else {
> > -        wSetFocusTo(scr, tmp);
> > +        if (!tmp) {
> > +        if (wPreferences.focus_mode == WKF_SLOPPY) {
> > +            wSetFocusTo(scr, foc);
> > +            } else {
> > +            wSetFocusTo(scr, tmp);
> > +        }
> >          }
>
>
> Definitely wrong!  This potentially sets focus to a NULL window.

Since wSetFocusTo() is implemented in such a way that it can handle
the case where the second argument is NULL, the conclusion doesn't
necessarily hold true.

What the patches are for?  If you don't know it, it would be better
to ignore them.

Regards,
- Kazunobu Kuriyama