comment in managedwin.itb

comment in managedwin.itb

[Tom Tromey]

I see this code in managedwin.itb:

  # I don't understand this next line and no one commented it, so it's gone.
  #focus -force [focus -lastfor $top]
  
  focus $top


I think I probably wrote the commented-out code.  Finding who
commented it out was annoying (most copies of the code in CVS were
made by people who obviously weren't the author), but I think it was
Jim Ingham.

Either I didn't comment this because I thought it was an idiom (I did
document it in my Tcl style guide), or because we were under a lot of
time pressure, or both.

The basic idea here is that `focus $top' sets the focus window to $top
-- but if the focus was previously on a subwindow of $top, then this
information is lost.

The idiom `focus -force [focus -lastfor $top]' causes us to force the
focus to change, but also lets us preserve the focus inside the
window.

Using -force is relatively unfriendly though.  I think we were doing
that everywhere at the time because we were targeting Windows, and
that seems to be the sort of thing one does there.

As it stands now I think the comment and the existing focus command in
that proc could be removed.  Comments on this?

Tom

Re: comment in managedwin.itb

[Mo DeJong]

On 28 Nov 2000, Tom Tromey wrote:

> I see this code in managedwin.itb:
> 
>   # I don't understand this next line and no one commented it, so it's gone.
>   #focus -force [focus -lastfor $top]
>   
>   focus $top

If you are looking for a better way to do that, you
could use this proc. It does the refocus management
with a -force except that it also handles the case
where the focus has not yet been assigned to a
widget in the given toplevel.

proc _refocus { window } {
    set toplevel [winfo toplevel $window]
    set last [focus -lastfor $window]
    if {$toplevel == $last} {
        focus -force $window
    } else {
        focus -force $last
    }
}

Just call it like so:

_refocus $text

Of course, I am assuming that you want to actually snap
the focus to the given widget with -force here.

Mo DeJong
Red Hat Inc

Re: comment in managedwin.itb

[Syd Polk]

I hate commented it out code. I think you should restore the command and 
give a concise comment about why you are not using focus $top.

At 01:57 PM 11/28/00 -0700, Tom Tromey wrote:
>I see this code in managedwin.itb:
>
>   # I don't understand this next line and no one commented it, so it's gone.
>   #focus -force [focus -lastfor $top]
>
>   focus $top
>
>
>I think I probably wrote the commented-out code.  Finding who
>commented it out was annoying (most copies of the code in CVS were
>made by people who obviously weren't the author), but I think it was
>Jim Ingham.
>
>Either I didn't comment this because I thought it was an idiom (I did
>document it in my Tcl style guide), or because we were under a lot of
>time pressure, or both.
>
>The basic idea here is that `focus $top' sets the focus window to $top
>-- but if the focus was previously on a subwindow of $top, then this
>information is lost.
>
>The idiom `focus -force [focus -lastfor $top]' causes us to force the
>focus to change, but also lets us preserve the focus inside the
>window.
>
>Using -force is relatively unfriendly though.  I think we were doing
>that everywhere at the time because we were targeting Windows, and
>that seems to be the sort of thing one does there.
>
>As it stands now I think the comment and the existing focus command in
>that proc could be removed.  Comments on this?
>
>Tom

Syd Polk		spolk@redhat.com
Engineering Manager	+1 415 777 9810 x 241
Red Hat, Inc.

Re: comment in managedwin.itb

[Keith Seitz]

Syd Polk wrote:
> 
> At 01:57 PM 11/28/00 -0700, Tom Tromey wrote:
> >I see this code in managedwin.itb:
> >
> >   # I don't understand this next line and no one commented it, so it's gone.
> >   #focus -force [focus -lastfor $top]
> >
> >   focus $top
> >
> >
> >I think I probably wrote the commented-out code.  Finding who
> >commented it out was annoying (most copies of the code in CVS were
> >made by people who obviously weren't the author), but I think it was
> >Jim Ingham.

It could have been me, too. I don't recall anymore... In any case, I
thought that this screwed up certain dialogs, TargetSelection in
particular. (Or maybe it did this just on windows??? Or when something
went awry...)

Anyway, I've said something now so it's in the archives. Maybe we'll
find out why this was done later. Perhaps there is some context in the
changelog?

Keith

Re: comment in managedwin.itb

[Fernando Nasser]

Keith Seitz wrote:
> 
> Syd Polk wrote:
> >
> > At 01:57 PM 11/28/00 -0700, Tom Tromey wrote:
> > >I see this code in managedwin.itb:
> > >
> > >   # I don't understand this next line and no one commented it, so it's gone.
> > >   #focus -force [focus -lastfor $top]
> > >
> > >   focus $top
> > >
> > >
> > >I think I probably wrote the commented-out code.  Finding who
> > >commented it out was annoying (most copies of the code in CVS were
> > >made by people who obviously weren't the author), but I think it was
> > >Jim Ingham.
> 
> It could have been me, too. I don't recall anymore... In any case, I
> thought that this screwed up certain dialogs, TargetSelection in
> particular. (Or maybe it did this just on windows??? Or when something
> went awry...)
> 
> Anyway, I've said something now so it's in the archives. Maybe we'll
> find out why this was done later. Perhaps there is some context in the
> changelog?
> 
  
Unfortunately the comment did not mention what was broken and where.

IMO, although it is a very boring task, we should try to research all the possible 
sources before turning this on again (we would do it if the research fails
and re-learn it the hard way -- probably through a regression).  Unless the
current code is causing some other annoyance, in which case we have no choice but
to try different things (it is broken one way or another).




-- 
Fernando Nasser
Red Hat - Toronto                       E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

Re: comment in managedwin.itb

[Tom Tromey]

Syd> I hate commented it out code. I think you should restore the
Syd> command and give a concise comment about why you are not using
Syd> focus $top.

Actually, I think we shouldn't use focus at all.

* Using focus -force is unfriendly.  It steals the focus, which the
  user might not want.
* Using focus is pointless since it just moves the potential focus
  point in that window.  In general we don't want to do that.

How about this?

2000-11-28  Tom Tromey  <tromey@cygnus.com>

	* managedwin.itb (ManagedWin::reveal): Don't set focus.

Tom

Index: managedwin.itb
===================================================================
RCS file: /cvs/src/src/gdb/gdbtk/library/managedwin.itb,v
retrieving revision 1.4
diff -u -r1.4 managedwin.itb
--- managedwin.itb	2000/03/29 03:18:16	1.4
+++ managedwin.itb	2000/11/29 04:16:39
@@ -40,11 +40,6 @@
   set top [winfo toplevel [namespace tail $this]]
   raise $top
   wm deiconify $top
-  
-  # I don't understand this next line and no one commented it, so it's gone.
-  #focus -force [focus -lastfor $top]
-  
-  focus $top
 }
 
 body ManagedWin::restart {} {

Re: comment in managedwin.itb

[Fernando Nasser]

As nobody seems to be 100% sure of how this happened, I suggest you add your
comments (between -----) and comment out the focus command.

Lets see what happens and, if we are all happy with the results, we remove
the code altogether (this is how we have been handling these cases in gdb).
We may even leave your comments permanently so no one in the future try do
add a "focus" in there.

Fernando


Tom Tromey wrote:
> 
> Syd> I hate commented it out code. I think you should restore the
> Syd> command and give a concise comment about why you are not using
> Syd> focus $top.
> 
------
> Actually, I think we shouldn't use focus at all.
> 
> * Using focus -force is unfriendly.  It steals the focus, which the
>   user might not want.
> * Using focus is pointless since it just moves the potential focus
>   point in that window.  In general we don't want to do that.
------
> 
> How about this?
> 
> 2000-11-28  Tom Tromey  <tromey@cygnus.com>
> 
>         * managedwin.itb (ManagedWin::reveal): Don't set focus.
> 
> Tom
> 
> Index: managedwin.itb
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbtk/library/managedwin.itb,v
> retrieving revision 1.4
> diff -u -r1.4 managedwin.itb
> --- managedwin.itb      2000/03/29 03:18:16     1.4
> +++ managedwin.itb      2000/11/29 04:16:39
> @@ -40,11 +40,6 @@
>    set top [winfo toplevel [namespace tail $this]]
>    raise $top
>    wm deiconify $top
> -
> -  # I don't understand this next line and no one commented it, so it's gone.
> -  #focus -force [focus -lastfor $top]
> -
> -  focus $top
>  }
> 
>  body ManagedWin::restart {} {

-- 
Fernando Nasser
Red Hat - Toronto                       E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

Re: comment in managedwin.itb

[Fernando Nasser]

Tom,

I guess you did not take it as approval.  My only suggestion is that you add
your comments (what you say in between the ------ below) in there so that no
one goes and add a "focus" back in that place.

Or do you think having "focus" in there is so absurd that after you remove it
no one will ever think of doing that?

Regards,
Fernando






Fernando Nasser wrote:
> 
> As nobody seems to be 100% sure of how this happened, I suggest you add your
> comments (between -----) and comment out the focus command.
> 
> Lets see what happens and, if we are all happy with the results, we remove
> the code altogether (this is how we have been handling these cases in gdb).
> We may even leave your comments permanently so no one in the future try do
> add a "focus" in there.
> 
> Fernando
> 
> Tom Tromey wrote:
> >
> > Syd> I hate commented it out code. I think you should restore the
> > Syd> command and give a concise comment about why you are not using
> > Syd> focus $top.
> >
> ------
> > Actually, I think we shouldn't use focus at all.
> >
> > * Using focus -force is unfriendly.  It steals the focus, which the
> >   user might not want.
> > * Using focus is pointless since it just moves the potential focus
> >   point in that window.  In general we don't want to do that.
> ------
> >
> > How about this?
> >
> > 2000-11-28  Tom Tromey  <tromey@cygnus.com>
> >
> >         * managedwin.itb (ManagedWin::reveal): Don't set focus.
> >
> > Tom
> >
> > Index: managedwin.itb
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/gdbtk/library/managedwin.itb,v
> > retrieving revision 1.4
> > diff -u -r1.4 managedwin.itb
> > --- managedwin.itb      2000/03/29 03:18:16     1.4
> > +++ managedwin.itb      2000/11/29 04:16:39
> > @@ -40,11 +40,6 @@
> >    set top [winfo toplevel [namespace tail $this]]
> >    raise $top
> >    wm deiconify $top
> > -
> > -  # I don't understand this next line and no one commented it, so it's gone.
> > -  #focus -force [focus -lastfor $top]
> > -
> > -  focus $top
> >  }
> >
> >  body ManagedWin::restart {} {
> 


-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

Re: comment in managedwin.itb

[Tom Tromey]

Fernando> I guess you did not take it as approval.  My only suggestion
Fernando> is that you add your comments (what you say in between the
Fernando> ------ below) in there so that no one goes and add a "focus"
Fernando> back in that place.

Fernando> Or do you think having "focus" in there is so absurd that
Fernando> after you remove it no one will ever think of doing that?


Nope.

I deleted the code and added this comment:

  # There used to be a `focus -force' here, but using -force is
  # unfriendly, so it was removed.  It was then replaced with a simple
  # `focus $top'.  However, this has no useful effect -- it just
  # resets the subwindow of $top which has the `potential' focus.
  # This can actually be confusing to the user.

I'm checking this in.  If you want something more in the comment, tell
me and I will add it.

Tom

Re: comment in managedwin.itb

[Fernando Nasser]

Tom Tromey wrote:
> 
> Fernando> I guess you did not take it as approval.  My only suggestion
> Fernando> is that you add your comments (what you say in between the
> Fernando> ------ below) in there so that no one goes and add a "focus"
> Fernando> back in that place.
> 
> Fernando> Or do you think having "focus" in there is so absurd that
> Fernando> after you remove it no one will ever think of doing that?
> 
> Nope.
> 
> I deleted the code and added this comment:
> 
>   # There used to be a `focus -force' here, but using -force is
>   # unfriendly, so it was removed.  It was then replaced with a simple
>   # `focus $top'.  However, this has no useful effect -- it just
>   # resets the subwindow of $top which has the `potential' focus.
>   # This can actually be confusing to the user.
> 
> I'm checking this in.  If you want something more in the comment, tell
> me and I will add it.
> 

Excelent!  That would prevent regressions.

Thank you.

-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

Re: comment in managedwin.itb

[Jim Ingham]

Keith,

Actually, IIRC this comment came from Martin.  I merged a whole bunch of his changes onto the main line at some point, so I ended up being the cvs annotate author of lots of stuff that was Martin's rather than mine.  But I remember vaguely having some conversation with him on the phone about this, but sadly not in any detail, so I can't really shed light on why we left this out.

Sorry...

Jim

> From: Keith Seitz <kseitz@firetalk.com>
> Date: Tue Nov 28, 2000  01:07:54 PM US/Pacific
> To: tromey@cygnus.com
> Cc: Insight List <insight@sourceware.cygnus.com>
> Subject: Re: comment in managedwin.itb
> 
> Syd Polk wrote:
> > 
> > At 01:57 PM 11/28/00 -0700, Tom Tromey wrote:
> > >I see this code in managedwin.itb:
> > >
> > >   # I don't understand this next line and no one commented it, so it'
> s gone.
> > >   #focus -force [focus -lastfor $top]
> > >
> > >   focus $top
> > >
> > >
> > >I think I probably wrote the commented-out code.  Finding who
> > >commented it out was annoying (most copies of the code in CVS were
> > >made by people who obviously weren't the author), but I think it was
> > >Jim Ingham.
> 
> It could have been me, too. I don't recall anymore... In any case, I
> thought that this screwed up certain dialogs, TargetSelection in
> particular. (Or maybe it did this just on windows??? Or when something
> went awry...)
> 
> Anyway, I've said something now so it's in the archives. Maybe we'll
> find out why this was done later. Perhaps there is some context in the
> changelog?
> 
> Keith

--
Jim Ingham                                   jingham@apple.com
Developer Tools - gdb
Apple Computer