From: Paul Mackerras <paulus@samba.org>
Date: Thu, 09 Oct 2008 00:27:51 GMT
Subject: Re: [PATCH 1/7] gitk: Enhance UI popup and accelerator handling.
Message-ID: <18669.20487.88439.584567@cargo.ozlabs.ibm.com>
In-Reply-To: <1223449540-20457-2-git-send-email-angavrilov@gmail.com>

Alexander Gavrilov writes:

> - Popups are supposed to be marked transient, otherwise
>   the WM creates them in strange places. Besides, at
>   least under kwin, transients are automatically kept
>   above their parent.
> - Accelerators for menu items should be listed directly
>   on the items, to make them more discoverable.
> - Some more accelerators are added, e.g. F4 for Edit View.
> - Finally, dialogs can now be accepted or dismissed using
>   the Return and Escape keys.

Thanks for the patch series; a few specific comments follow.

> +    catch { bind . <Shift-Key-XF86_Switch_VT_4> {newview 0} }

What is this key, or why is this line needed?

> @@ -2484,10 +2501,10 @@ proc keys {} {
>      }
>      toplevel $w
>      wm title $w [mc "Gitk key bindings"]
> +    wm transient $w .
>      message $w.m -text "
>  [mc "Gitk key bindings:"]
>  
> -[mc "<%s-Q>		Quit" $M1T]
>  [mc "<Home>		Move to first commit"]
>  [mc "<End>		Move to last commit"]
>  [mc "<Up>, p, i	Move up one commit"]
> @@ -2521,11 +2538,11 @@ proc keys {} {
>  [mc "<%s-plus>	Increase font size" $M1T]
>  [mc "<%s-KP->	Decrease font size" $M1T]
>  [mc "<%s-minus>	Decrease font size" $M1T]
> -[mc "<F5>		Update"]

I think it is useful to have the accelerator keys listed in the key
binding help.

> @@ -7501,6 +7534,7 @@ proc domktag {} {
>      addedtag $id
>      dispneartags 0
>      run refill_reflist
> +    return 1
>  }
>  
>  proc redrawtags {id} {
> @@ -7539,7 +7573,7 @@ proc mktagcan {} {
>  }
>  
>  proc mktaggo {} {
> -    domktag
> +    if {![domktag]} return

You need to change the error returns in domktag to say "return 0",
otherwise this will give a "can't use empty string as operand of "!""
Tcl error.

Paul.