All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [RFC/PATCH] Force using Tcl/Tk 8.4 on Mac OS X
       [not found] <1283792854-45023-1-git-send-email-lists@haller-berlin.de>
@ 2010-09-06 19:36 ` Daniel A. Steffen
  2010-09-07  7:19   ` Stefan Haller
                     ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Daniel A. Steffen @ 2010-09-06 19:36 UTC (permalink / raw)
  To: Stefan Haller; +Cc: git, Stefan Haller, Shawn O Pearce, Daniel A Steffen

Hi Stefan,

On Sep 6, 2010, at 10:07 AM, Stefan Haller wrote:

> When using Git Gui.app on a Snow Leopard system with Tcl/Tk 8.5,
> there are two problems:
> 
> 1) Menu commands that open a dialog (e.g. "Push" or "Revert changes")
>   don't work when invoked via their keyboard shortcuts. You get a
>   dialog without a title bar, and the application then hangs without
>   further responding to any user input; you need to kill it. Invoking
>   the same commands with the mouse by choosing from the menu works fine.

first time I hear of this (or see this myself, and I use git gui every day on Mac OS X); in part this may be due to the fact that many of the menu shortcuts assigned by git gui conflict with existing OS X shortcuts and don't work anyway e.g. cmd-A (esp if a text widget is in focus)...

personally I would hate for my git gui to be downgraded to Tk 8.4 and Carbon because of this small issue, and I'm unlikely to be alone.

> 2) The colored pane headers (red, green, yellow) are all grey.

this is intentional and triggered by the use of ttk, there is a config setting to turn it off (gui.usettk)

> Obviously, the first problem is much more serious than the second.
> 
> Both problems go away when using Tcl/Tk 8.4 instead of 8.5. The user
> interface looks a little "older" then, but at least it works reliably.
> 
> Until someone figures out how to solve these problems properly with
> 8.5, force using 8.4.

please don't brute-force around the problem in this way, there are many features that the Cocoa Tk in 8.5+ adds that would be lost by such a change. 
Moreover the fact that 8.4 is present in that location is for backwards compatibility only, please don't introduce any new dependencies on it.

An effective small workaround for the problem appears to be to turn off the menu accelerators for the affected commands (the key shortcuts will still work, since the actual key bindings are independent of the menu accelerators), see below.
Is there a complete list of menu shortcuts that bring up a dialog?

Ultimately the right way to address this issue is to fix it in Tk, please report this in the Tk bug tracker if you haven't already, a quick glance in the debugger shows a hang in recursive event loop invocation such as used by tk_dialog when called from a menu accelerator callback.

Thanks!

Cheers,

Daniel


diff --git i/git-gui.sh w/git-gui.sh
index 0d5c5e3..ec7ed7d 100755
--- i/git-gui.sh
+++ w/git-gui.sh
@@ -2723,6 +2723,9 @@ if {[is_enabled multicommit] || [is_enabled singlecommit]} {
                -accelerator $M1T-J
        lappend disable_on_lock \
                [list .mbar.commit entryconf [.mbar.commit index last] -state]
+       if {[is_MacOSX]} {
+               .mbar.commit entryconf last -accelerator {}
+       }
 
        .mbar.commit add separator
 

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [RFC/PATCH] Force using Tcl/Tk 8.4 on Mac OS X
  2010-09-06 19:36 ` [RFC/PATCH] Force using Tcl/Tk 8.4 on Mac OS X Daniel A. Steffen
@ 2010-09-07  7:19   ` Stefan Haller
  2010-09-07 19:11   ` Pat Thoyts
  2010-09-14  5:42   ` [RFC/PATCH] Force using Tcl/Tk 8.4 on " Stefan Haller
  2 siblings, 0 replies; 11+ messages in thread
From: Stefan Haller @ 2010-09-07  7:19 UTC (permalink / raw)
  To: Daniel A. Steffen; +Cc: git, Shawn O Pearce, Daniel A Steffen

Daniel A. Steffen <das@users.sourceforge.net> wrote:

> On Sep 6, 2010, at 10:07 AM, Stefan Haller wrote:
> 
> > When using Git Gui.app on a Snow Leopard system with Tcl/Tk 8.5,
> > there are two problems:
> > 
> > 1) Menu commands that open a dialog (e.g. "Push" or "Revert changes")
> >   don't work when invoked via their keyboard shortcuts. You get a
> >   dialog without a title bar, and the application then hangs without
> >   further responding to any user input; you need to kill it. Invoking
> >   the same commands with the mouse by choosing from the menu works fine.
> 
> first time I hear of this (or see this myself, and I use git gui every day
> on Mac OS X); in part this may be due to the fact that many of the menu
> shortcuts assigned by git gui conflict with existing OS X shortcuts and
> don't work anyway e.g. cmd-A (esp if a text widget is in focus)...
> 
> personally I would hate for my git gui to be downgraded to Tk 8.4 and
> Carbon because of this small issue, and I'm unlikely to be alone.

It's not a small issue for me, but I agree that downgrading to 8.4 is
the wrong solution.

> An effective small workaround for the problem appears to be to turn off
> the menu accelerators for the affected commands (the key shortcuts will
> still work, since the actual key bindings are independent of the menu
> accelerators), see below.

This does indeed solve the problem, but at a high price; having the menu
accelerators not visible in the menu is not a satisfactory solution.
(For me personally it's ok, but not for new users.)

Is there no other way to work around this in git gui, like maybe
deferring the command invocation with a one-shot timer or something?

> Is there a complete list of menu shortcuts that bring up a dialog?

As far as I can see, it's

    Branch/Create
    Branch/Checkout
    Commit/Revert Changes
    Merge/Local Merge
    Remote/Push

> Ultimately the right way to address this issue is to fix it in Tk, please
> report this in the Tk bug tracker if you haven't already, a quick glance
> in the debugger shows a hang in recursive event loop invocation such as
> used by tk_dialog when called from a menu accelerator callback.

Will do, thanks.

> > 2) The colored pane headers (red, green, yellow) are all grey.
> 
> this is intentional and triggered by the use of ttk, there is a config
> setting to turn it off (gui.usettk)

I'm not sure I understand. On a Windows system with Tk 8.5 and ttk on,
the pane headers are still colored.  Why is it desirable to have them
grey on Mac?

-Stefan


-- 
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC/PATCH] Force using Tcl/Tk 8.4 on Mac OS X
  2010-09-06 19:36 ` [RFC/PATCH] Force using Tcl/Tk 8.4 on Mac OS X Daniel A. Steffen
  2010-09-07  7:19   ` Stefan Haller
@ 2010-09-07 19:11   ` Pat Thoyts
  2010-09-07 19:56     ` Stefan Haller
  2010-09-14  5:42   ` [RFC/PATCH] Force using Tcl/Tk 8.4 on " Stefan Haller
  2 siblings, 1 reply; 11+ messages in thread
From: Pat Thoyts @ 2010-09-07 19:11 UTC (permalink / raw)
  To: Daniel A. Steffen
  Cc: Stefan Haller, git, Stefan Haller, Shawn O Pearce, Daniel A Steffen

"Daniel A. Steffen" <das@users.sourceforge.net> writes:

>Hi Stefan,
>
>On Sep 6, 2010, at 10:07 AM, Stefan Haller wrote:
>
>> When using Git Gui.app on a Snow Leopard system with Tcl/Tk 8.5,
>> there are two problems:
>> 
>> 1) Menu commands that open a dialog (e.g. "Push" or "Revert changes")
>>   don't work when invoked via their keyboard shortcuts. You get a
>>   dialog without a title bar, and the application then hangs without
>>   further responding to any user input; you need to kill it. Invoking
>>   the same commands with the mouse by choosing from the menu works fine.
>
>first time I hear of this (or see this myself, and I use git gui every
>day on Mac OS X); in part this may be due to the fact that many of the
>menu shortcuts assigned by git gui conflict with existing OS X
>shortcuts and don't work anyway e.g. cmd-A (esp if a text widget is in
>focus)...
>
>personally I would hate for my git gui to be downgraded to Tk 8.4 and
>Carbon because of this small issue, and I'm unlikely to be alone.
>

I will not force 8.4 on anyone. If someone wants to force it locally
they can just edit the wish command in their git-gui script file to use
wish8.4.

I'm happy - even eager - to apply patches to improve the MacOSX git gui
experience but I can't test them as I don't have such a system.

>
>diff --git i/git-gui.sh w/git-gui.sh
>index 0d5c5e3..ec7ed7d 100755
>--- i/git-gui.sh
>+++ w/git-gui.sh
>@@ -2723,6 +2723,9 @@ if {[is_enabled multicommit] || [is_enabled singlecommit]} {
>                -accelerator $M1T-J
>        lappend disable_on_lock \
>                [list .mbar.commit entryconf [.mbar.commit index last] -state]
>+       if {[is_MacOSX]} {
>+               .mbar.commit entryconf last -accelerator {}
>+       }
> 
>        .mbar.commit add separator
> 
>

This removes the Cmd-J accelerator from the "Revert Changes" menu
item. I assume that just changing the menu command to 
 {after idle [list do_revert_selection]}
doesn't work either?

Do you want this applied or will you produce a patch that removes more
such inappropriate accelerators?

Also - got any other MacOS specific patches?

-- 
Pat Thoyts                            http://www.patthoyts.tk/
PGP fingerprint 2C 6E 98 07 2C 59 C8 97  10 CE 11 E6 04 E0 B9 DD

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC/PATCH] Force using Tcl/Tk 8.4 on Mac OS X
  2010-09-07 19:11   ` Pat Thoyts
@ 2010-09-07 19:56     ` Stefan Haller
  2010-09-11  7:12       ` Stefan Haller
                         ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Stefan Haller @ 2010-09-07 19:56 UTC (permalink / raw)
  To: Pat Thoyts, Daniel A. Steffen; +Cc: git, Shawn O Pearce, Daniel A Steffen

Pat Thoyts <patthoyts@users.sourceforge.net> wrote:

> This removes the Cmd-J accelerator from the "Revert Changes" menu
> item. I assume that just changing the menu command to 
>  {after idle [list do_revert_selection]}
> doesn't work either?

No, but "after 100" does for me; "after 10" does not, and "after 50"
does some of the time.  I'm not sure if this would be suitable as a
workaround then.

-Stefan


-- 
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC/PATCH] Force using Tcl/Tk 8.4 on Mac OS X
  2010-09-07 19:56     ` Stefan Haller
@ 2010-09-11  7:12       ` Stefan Haller
  2010-09-11  7:31       ` Stefan Haller
                         ` (2 subsequent siblings)
  3 siblings, 0 replies; 11+ messages in thread
From: Stefan Haller @ 2010-09-11  7:12 UTC (permalink / raw)
  To: Pat Thoyts, Daniel A. Steffen; +Cc: git, Daniel A Steffen

Stefan Haller <lists@haller-berlin.de> wrote:

> Pat Thoyts <patthoyts@users.sourceforge.net> wrote:
> 
> > This removes the Cmd-J accelerator from the "Revert Changes" menu
> > item. I assume that just changing the menu command to 
> >  {after idle [list do_revert_selection]}
> > doesn't work either?
> 
> No, but "after 100" does for me; "after 10" does not, and "after 50"
> does some of the time.  I'm not sure if this would be suitable as a
> workaround then.

We seem to be stuck with this right now.  What can I do to move this
forward?  I have little experience with Tcl/Tk, so I'm probably unable
to solve this in a satifactory way myself; but I'd like to do everything
I can to make progress here.

Do people think that the "after 100" hack would be acceptable, if it's
conditional for {[is_MacOSX] && $::have_tk85}?  Would it help if I try
to come up with a patch for that?

-Stefan


-- 
Stefan Haller
Ableton
http://www.ableton.com/

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC/PATCH] Force using Tcl/Tk 8.4 on Mac OS X
  2010-09-07 19:56     ` Stefan Haller
  2010-09-11  7:12       ` Stefan Haller
@ 2010-09-11  7:31       ` Stefan Haller
  2010-09-21  8:26       ` [PATCH] git-gui: Work around freeze problem with dialogs in " Stefan Haller
  2010-09-22 17:46       ` Stefan Haller
  3 siblings, 0 replies; 11+ messages in thread
From: Stefan Haller @ 2010-09-11  7:31 UTC (permalink / raw)
  To: Pat Thoyts, Daniel A. Steffen; +Cc: git, Daniel A Steffen

Stefan Haller <lists@haller-berlin.de> wrote:

> Pat Thoyts <patthoyts@users.sourceforge.net> wrote:
> 
> > This removes the Cmd-J accelerator from the "Revert Changes" menu
> > item. I assume that just changing the menu command to 
> >  {after idle [list do_revert_selection]}
> > doesn't work either?
> 
> No, but "after 100" does for me; "after 10" does not, and "after 50"
> does some of the time.  I'm not sure if this would be suitable as a
> workaround then.

We seem to be stuck with this right now.  What can I do to move this
forward?  I have little experience with Tcl/Tk, so I'm probably unable
to solve this in a satifactory way myself; but I'd like to do everything
I can to help make progress here.

Do people think that the "after 100" hack would be acceptable, if it's
conditional for {[is_MacOSX] && $::have_tk85}?  Would it help if I try
to come up with a patch for that?

-Stefan


(Sorry for the duplicate mail; I used the wrong return address the first
time, so it didn't got to the list.)


-- 
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC/PATCH] Force using Tcl/Tk 8.4 on Mac OS X
  2010-09-06 19:36 ` [RFC/PATCH] Force using Tcl/Tk 8.4 on Mac OS X Daniel A. Steffen
  2010-09-07  7:19   ` Stefan Haller
  2010-09-07 19:11   ` Pat Thoyts
@ 2010-09-14  5:42   ` Stefan Haller
  2 siblings, 0 replies; 11+ messages in thread
From: Stefan Haller @ 2010-09-14  5:42 UTC (permalink / raw)
  To: Daniel A. Steffen; +Cc: git, Daniel A Steffen

Daniel A. Steffen <das@users.sourceforge.net> wrote:

> Ultimately the right way to address this issue is to fix it in Tk, please
> report this in the Tk bug tracker if you haven't already, a quick glance
> in the debugger shows a hang in recursive event loop invocation such as
> used by tk_dialog when called from a menu accelerator callback.

I finally got around to this now.  It was already reported as issue
3044863; I added a comment to the existing ticket.

<http://sourceforge.net/tracker/?func=detail&aid=3044863&group_id=12997&atid=112997>

I still think we need a work-around in git gui (and maybe gitk) until a
fixed Tk version is widely available. Yesterday I showed git to a
co-worker, and after 20 minutes of randomly playing around with it he
ran into the problem (without me pointing him to it).

-Stefan


-- 
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] git-gui: Work around freeze problem with dialogs in Mac OS X
  2010-09-07 19:56     ` Stefan Haller
  2010-09-11  7:12       ` Stefan Haller
  2010-09-11  7:31       ` Stefan Haller
@ 2010-09-21  8:26       ` Stefan Haller
  2010-09-22 17:46       ` Stefan Haller
  3 siblings, 0 replies; 11+ messages in thread
From: Stefan Haller @ 2010-09-21  8:26 UTC (permalink / raw)
  To: Pat Thoyts, Daniel A. Steffen; +Cc: git, Daniel A Steffen

Tk 8.5 on Mac OS X has a bug whereby a dialog opened from a key
binding will hang; see issue 3044863 in the Tk issue tracker.
<http://sourceforge.net/tracker/?func=detail&aid=3044863&group_id=12997&atid=112997>

To work around this, we perform commands that open a dialog after
a brief delay; 150 ms seems to be a good compromise between short
enough as to be not annoying, and long enough to reliably work
around the issue.

Signed-off-by: Stefan Haller <stefan@haller-berlin.de>
---
 git-gui.sh |   39 +++++++++++++++++++++++++++------------
 1 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/git-gui.sh b/git-gui.sh
index 4617f29..394c2a0 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -3560,6 +3560,21 @@ if {[info exists repo_config(gui.wmstate)]} {
    catch {wm state . $repo_config(gui.wmstate)}
 }
 
+proc mac_freeze_workaround {cmd} {
+   if {[is_MacOSX] && $::have_tk85} {
+       # Tk 8.5 on Mac OS X has a bug whereby a dialog opened from a key
+       # binding will hang; see issue 3044863 in the Tk issue tracker.
+       # <http://sourceforge.net/tracker/?func=detail&aid=3044863&group_id=12997&atid=112997>
+       #
+       # To work around this, we perform commands that open a dialog after a brief
+       # delay; 150 ms seems to be a good compromise between short enough as to be
+       # not annoying, and long enough to reliably work around the issue.
+       after 150 $cmd
+   } else {
+       $cmd
+   }
+}
+
 # -- Key Bindings
 #
 bind $ui_comm <$M1B-Key-Return> {do_commit;break}
@@ -3567,8 +3582,8 @@ bind $ui_comm <$M1B-Key-t> {do_add_selection;break}
 bind $ui_comm <$M1B-Key-T> {do_add_selection;break}
 bind $ui_comm <$M1B-Key-u> {do_unstage_selection;break}
 bind $ui_comm <$M1B-Key-U> {do_unstage_selection;break}
-bind $ui_comm <$M1B-Key-j> {do_revert_selection;break}
-bind $ui_comm <$M1B-Key-J> {do_revert_selection;break}
+bind $ui_comm <$M1B-Key-j> {mac_freeze_workaround do_revert_selection;break}
+bind $ui_comm <$M1B-Key-J> {mac_freeze_workaround do_revert_selection;break}
 bind $ui_comm <$M1B-Key-i> {do_add_all;break}
 bind $ui_comm <$M1B-Key-I> {do_add_all;break}
 bind $ui_comm <$M1B-Key-x> {tk_textCut %W;break}
@@ -3606,16 +3621,16 @@ bind $ui_diff <Control-Key-f> {catch {%W yview scroll  1 pages};break}
 bind $ui_diff <Button-1>   {focus %W}
 
 if {[is_enabled branch]} {
-   bind . <$M1B-Key-n> branch_create::dialog
-   bind . <$M1B-Key-N> branch_create::dialog
-   bind . <$M1B-Key-o> branch_checkout::dialog
-   bind . <$M1B-Key-O> branch_checkout::dialog
-   bind . <$M1B-Key-m> merge::dialog
-   bind . <$M1B-Key-M> merge::dialog
+   bind . <$M1B-Key-n> {mac_freeze_workaround branch_create::dialog}
+   bind . <$M1B-Key-N> {mac_freeze_workaround branch_create::dialog}
+   bind . <$M1B-Key-o> {mac_freeze_workaround branch_checkout::dialog}
+   bind . <$M1B-Key-O> {mac_freeze_workaround branch_checkout::dialog}
+   bind . <$M1B-Key-m> {mac_freeze_workaround merge::dialog}
+   bind . <$M1B-Key-M> {mac_freeze_workaround merge::dialog}
 }
 if {[is_enabled transport]} {
-   bind . <$M1B-Key-p> do_push_anywhere
-   bind . <$M1B-Key-P> do_push_anywhere
+   bind . <$M1B-Key-p> {mac_freeze_workaround do_push_anywhere}
+   bind . <$M1B-Key-P> {mac_freeze_workaround do_push_anywhere}
 }
 
 bind .   <Key-F5>     ui_do_rescan
@@ -3625,8 +3640,8 @@ bind .   <$M1B-Key-s> do_signoff
 bind .   <$M1B-Key-S> do_signoff
 bind .   <$M1B-Key-t> do_add_selection
 bind .   <$M1B-Key-T> do_add_selection
-bind .   <$M1B-Key-j> do_revert_selection
-bind .   <$M1B-Key-J> do_revert_selection
+bind .   <$M1B-Key-j> {mac_freeze_workaround do_revert_selection}
+bind .   <$M1B-Key-J> {mac_freeze_workaround do_revert_selection}
 bind .   <$M1B-Key-i> do_add_all
 bind .   <$M1B-Key-I> do_add_all
 bind .   <$M1B-Key-minus> {show_less_context;break}
-- 
1.7.3.4.g200b9

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH] git-gui: Work around freeze problem with dialogs in Mac OS X
  2010-09-07 19:56     ` Stefan Haller
                         ` (2 preceding siblings ...)
  2010-09-21  8:26       ` [PATCH] git-gui: Work around freeze problem with dialogs in " Stefan Haller
@ 2010-09-22 17:46       ` Stefan Haller
  2010-09-22 19:01         ` Junio C Hamano
  3 siblings, 1 reply; 11+ messages in thread
From: Stefan Haller @ 2010-09-22 17:46 UTC (permalink / raw)
  To: git

Tk 8.5 on Mac OS X has a bug whereby a dialog opened from a key
binding will hang; see issue 3044863 in the Tk issue tracker.
<http://sourceforge.net/tracker/?func=detail&aid=3044863&group_id=12997&atid=112997>

To work around this, we perform commands that open a dialog after
a brief delay; 150 ms seems to be a good compromise between short
enough as to be not annoying, and long enough to reliably work
around the issue.

Signed-off-by: Stefan Haller <stefan@haller-berlin.de>
---
I already sent this two days ago, but it didn't seem to appear on the
list for some reason, so I'm resending it. Apologies if you see this
twice.

 git-gui.sh |   39 +++++++++++++++++++++++++++------------
 1 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/git-gui.sh b/git-gui.sh
index 4617f29..394c2a0 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -3560,6 +3560,21 @@ if {[info exists repo_config(gui.wmstate)]} {
    catch {wm state . $repo_config(gui.wmstate)}
 }
 
+proc mac_freeze_workaround {cmd} {
+   if {[is_MacOSX] && $::have_tk85} {
+       # Tk 8.5 on Mac OS X has a bug whereby a dialog opened from a key
+       # binding will hang; see issue 3044863 in the Tk issue tracker.
+       # <http://sourceforge.net/tracker/?func=detail&aid=3044863&group_id=12997&atid=112997>
+       #
+       # To work around this, we perform commands that open a dialog after a brief
+       # delay; 150 ms seems to be a good compromise between short enough as to be
+       # not annoying, and long enough to reliably work around the issue.
+       after 150 $cmd
+   } else {
+       $cmd
+   }
+}
+
 # -- Key Bindings
 #
 bind $ui_comm <$M1B-Key-Return> {do_commit;break}
@@ -3567,8 +3582,8 @@ bind $ui_comm <$M1B-Key-t> {do_add_selection;break}
 bind $ui_comm <$M1B-Key-T> {do_add_selection;break}
 bind $ui_comm <$M1B-Key-u> {do_unstage_selection;break}
 bind $ui_comm <$M1B-Key-U> {do_unstage_selection;break}
-bind $ui_comm <$M1B-Key-j> {do_revert_selection;break}
-bind $ui_comm <$M1B-Key-J> {do_revert_selection;break}
+bind $ui_comm <$M1B-Key-j> {mac_freeze_workaround do_revert_selection;break}
+bind $ui_comm <$M1B-Key-J> {mac_freeze_workaround do_revert_selection;break}
 bind $ui_comm <$M1B-Key-i> {do_add_all;break}
 bind $ui_comm <$M1B-Key-I> {do_add_all;break}
 bind $ui_comm <$M1B-Key-x> {tk_textCut %W;break}
@@ -3606,16 +3621,16 @@ bind $ui_diff <Control-Key-f> {catch {%W yview scroll  1 pages};break}
 bind $ui_diff <Button-1>   {focus %W}
 
 if {[is_enabled branch]} {
-   bind . <$M1B-Key-n> branch_create::dialog
-   bind . <$M1B-Key-N> branch_create::dialog
-   bind . <$M1B-Key-o> branch_checkout::dialog
-   bind . <$M1B-Key-O> branch_checkout::dialog
-   bind . <$M1B-Key-m> merge::dialog
-   bind . <$M1B-Key-M> merge::dialog
+   bind . <$M1B-Key-n> {mac_freeze_workaround branch_create::dialog}
+   bind . <$M1B-Key-N> {mac_freeze_workaround branch_create::dialog}
+   bind . <$M1B-Key-o> {mac_freeze_workaround branch_checkout::dialog}
+   bind . <$M1B-Key-O> {mac_freeze_workaround branch_checkout::dialog}
+   bind . <$M1B-Key-m> {mac_freeze_workaround merge::dialog}
+   bind . <$M1B-Key-M> {mac_freeze_workaround merge::dialog}
 }
 if {[is_enabled transport]} {
-   bind . <$M1B-Key-p> do_push_anywhere
-   bind . <$M1B-Key-P> do_push_anywhere
+   bind . <$M1B-Key-p> {mac_freeze_workaround do_push_anywhere}
+   bind . <$M1B-Key-P> {mac_freeze_workaround do_push_anywhere}
 }
 
 bind .   <Key-F5>     ui_do_rescan
@@ -3625,8 +3640,8 @@ bind .   <$M1B-Key-s> do_signoff
 bind .   <$M1B-Key-S> do_signoff
 bind .   <$M1B-Key-t> do_add_selection
 bind .   <$M1B-Key-T> do_add_selection
-bind .   <$M1B-Key-j> do_revert_selection
-bind .   <$M1B-Key-J> do_revert_selection
+bind .   <$M1B-Key-j> {mac_freeze_workaround do_revert_selection}
+bind .   <$M1B-Key-J> {mac_freeze_workaround do_revert_selection}
 bind .   <$M1B-Key-i> do_add_all
 bind .   <$M1B-Key-I> do_add_all
 bind .   <$M1B-Key-minus> {show_less_context;break}
-- 
1.7.3.4.g200b9


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] git-gui: Work around freeze problem with dialogs in Mac OS X
  2010-09-22 17:46       ` Stefan Haller
@ 2010-09-22 19:01         ` Junio C Hamano
  2010-09-23  7:39           ` Stefan Haller
  0 siblings, 1 reply; 11+ messages in thread
From: Junio C Hamano @ 2010-09-22 19:01 UTC (permalink / raw)
  To: Stefan Haller; +Cc: git

lists@haller-berlin.de (Stefan Haller) writes:

> Tk 8.5 on Mac OS X has a bug whereby a dialog opened from a key
> binding will hang; see issue 3044863 in the Tk issue tracker.
> <http://sourceforge.net/tracker/?func=detail&aid=3044863&group_id=12997&atid=112997>
>
> To work around this, we perform commands that open a dialog after
> a brief delay; 150 ms seems to be a good compromise between short
> enough as to be not annoying, and long enough to reliably work
> around the issue.
>
> Signed-off-by: Stefan Haller <stefan@haller-berlin.de>

Is 150ms applicable no matter how fast or slow your Mac is, or is Mac so
monoculture that everybody's machine has more or less the same performance
characteristics?  IOW does this need to be autoadjusted?

I see a lot of wrapping around foo::dialog; without knowing much about
Tcl, I wonder if it would be simpler, less error prone and more future
proof to add the wrapping logic around something commonly used from them,
e.g. class::make_dialog.

> ---
> I already sent this two days ago, but it didn't seem to appear on the
> list for some reason, so I'm resending it. Apologies if you see this
> twice.
>
>  git-gui.sh |   39 +++++++++++++++++++++++++++------------
>  1 files changed, 27 insertions(+), 12 deletions(-)
>
> diff --git a/git-gui.sh b/git-gui.sh
> index 4617f29..394c2a0 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -3560,6 +3560,21 @@ if {[info exists repo_config(gui.wmstate)]} {
>     catch {wm state . $repo_config(gui.wmstate)}
>  }
>  
> +proc mac_freeze_workaround {cmd} {
> +   if {[is_MacOSX] && $::have_tk85} {
> +       # Tk 8.5 on Mac OS X has a bug whereby a dialog opened from a key
> +       # binding will hang; see issue 3044863 in the Tk issue tracker.
> +       # <http://sourceforge.net/tracker/?func=detail&aid=3044863&group_id=12997&atid=112997>
> +       #
> +       # To work around this, we perform commands that open a dialog after a brief
> +       # delay; 150 ms seems to be a good compromise between short enough as to be
> +       # not annoying, and long enough to reliably work around the issue.
> +       after 150 $cmd
> +   } else {
> +       $cmd
> +   }
> +}
> +
>  # -- Key Bindings
>  #
>  bind $ui_comm <$M1B-Key-Return> {do_commit;break}
> @@ -3567,8 +3582,8 @@ bind $ui_comm <$M1B-Key-t> {do_add_selection;break}
>  bind $ui_comm <$M1B-Key-T> {do_add_selection;break}
>  bind $ui_comm <$M1B-Key-u> {do_unstage_selection;break}
>  bind $ui_comm <$M1B-Key-U> {do_unstage_selection;break}
> -bind $ui_comm <$M1B-Key-j> {do_revert_selection;break}
> -bind $ui_comm <$M1B-Key-J> {do_revert_selection;break}
> +bind $ui_comm <$M1B-Key-j> {mac_freeze_workaround do_revert_selection;break}
> +bind $ui_comm <$M1B-Key-J> {mac_freeze_workaround do_revert_selection;break}
>  bind $ui_comm <$M1B-Key-i> {do_add_all;break}
>  bind $ui_comm <$M1B-Key-I> {do_add_all;break}
>  bind $ui_comm <$M1B-Key-x> {tk_textCut %W;break}
> @@ -3606,16 +3621,16 @@ bind $ui_diff <Control-Key-f> {catch {%W yview scroll  1 pages};break}
>  bind $ui_diff <Button-1>   {focus %W}
>  
>  if {[is_enabled branch]} {
> -   bind . <$M1B-Key-n> branch_create::dialog
> -   bind . <$M1B-Key-N> branch_create::dialog
> -   bind . <$M1B-Key-o> branch_checkout::dialog
> -   bind . <$M1B-Key-O> branch_checkout::dialog
> -   bind . <$M1B-Key-m> merge::dialog
> -   bind . <$M1B-Key-M> merge::dialog
> +   bind . <$M1B-Key-n> {mac_freeze_workaround branch_create::dialog}
> +   bind . <$M1B-Key-N> {mac_freeze_workaround branch_create::dialog}
> +   bind . <$M1B-Key-o> {mac_freeze_workaround branch_checkout::dialog}
> +   bind . <$M1B-Key-O> {mac_freeze_workaround branch_checkout::dialog}
> +   bind . <$M1B-Key-m> {mac_freeze_workaround merge::dialog}
> +   bind . <$M1B-Key-M> {mac_freeze_workaround merge::dialog}
>  }
>  if {[is_enabled transport]} {
> -   bind . <$M1B-Key-p> do_push_anywhere
> -   bind . <$M1B-Key-P> do_push_anywhere
> +   bind . <$M1B-Key-p> {mac_freeze_workaround do_push_anywhere}
> +   bind . <$M1B-Key-P> {mac_freeze_workaround do_push_anywhere}
>  }
>  
>  bind .   <Key-F5>     ui_do_rescan
> @@ -3625,8 +3640,8 @@ bind .   <$M1B-Key-s> do_signoff
>  bind .   <$M1B-Key-S> do_signoff
>  bind .   <$M1B-Key-t> do_add_selection
>  bind .   <$M1B-Key-T> do_add_selection
> -bind .   <$M1B-Key-j> do_revert_selection
> -bind .   <$M1B-Key-J> do_revert_selection
> +bind .   <$M1B-Key-j> {mac_freeze_workaround do_revert_selection}
> +bind .   <$M1B-Key-J> {mac_freeze_workaround do_revert_selection}
>  bind .   <$M1B-Key-i> do_add_all
>  bind .   <$M1B-Key-I> do_add_all
>  bind .   <$M1B-Key-minus> {show_less_context;break}
> -- 
> 1.7.3.4.g200b9
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] git-gui: Work around freeze problem with dialogs in Mac OS X
  2010-09-22 19:01         ` Junio C Hamano
@ 2010-09-23  7:39           ` Stefan Haller
  0 siblings, 0 replies; 11+ messages in thread
From: Stefan Haller @ 2010-09-23  7:39 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Daniel A Steffen, Daniel A. Steffen

Junio C Hamano <gitster@pobox.com> wrote:

> lists@haller-berlin.de (Stefan Haller) writes:
> 
> > Tk 8.5 on Mac OS X has a bug whereby a dialog opened from a key
> > binding will hang; see issue 3044863 in the Tk issue tracker.
> > <http://sourceforge.net/tracker/?func=detail&aid=3044863&group_id=12997&atid=112997>
> >
> > To work around this, we perform commands that open a dialog after
> > a brief delay; 150 ms seems to be a good compromise between short
> > enough as to be not annoying, and long enough to reliably work
> > around the issue.
> >
> > Signed-off-by: Stefan Haller <stefan@haller-berlin.de>
> 
> Is 150ms applicable no matter how fast or slow your Mac is, or is Mac so
> monoculture that everybody's machine has more or less the same performance
> characteristics?  IOW does this need to be autoadjusted?

To be honest, I don't know.  I was hoping that Daniel could shed some
light on whether this is dependent on the machine's performance, or why
the delay is needed at all (i.e. why a simple "after idle" won't do, as
one would have expected).

> I see a lot of wrapping around foo::dialog; without knowing much about
> Tcl, I wonder if it would be simpler, less error prone and more future
> proof to add the wrapping logic around something commonly used from them,
> e.g. class::make_dialog.

I need to wrap the top-level entry points of the command handlers,
because we want the delay only when the command is invoked from a key
binding, not when a menu item is selected with the mouse, or a button is
pushed.  Inside the functions, at the place where the dialog is created,
I can't tell from where we were called.


-- 
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2010-09-23  7:39 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <1283792854-45023-1-git-send-email-lists@haller-berlin.de>
2010-09-06 19:36 ` [RFC/PATCH] Force using Tcl/Tk 8.4 on Mac OS X Daniel A. Steffen
2010-09-07  7:19   ` Stefan Haller
2010-09-07 19:11   ` Pat Thoyts
2010-09-07 19:56     ` Stefan Haller
2010-09-11  7:12       ` Stefan Haller
2010-09-11  7:31       ` Stefan Haller
2010-09-21  8:26       ` [PATCH] git-gui: Work around freeze problem with dialogs in " Stefan Haller
2010-09-22 17:46       ` Stefan Haller
2010-09-22 19:01         ` Junio C Hamano
2010-09-23  7:39           ` Stefan Haller
2010-09-14  5:42   ` [RFC/PATCH] Force using Tcl/Tk 8.4 on " Stefan Haller

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.