threads / patch / 56210

patch, 3 partsgitk: macOS improvements

Subject: [PATCH 0/3] gitk: macOS improvements

## tl;dr

6 messages between Aug 4, 2021 and Aug 4, 2021. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Carlo Marcelo Arenas Belón· Aug 4, 2021, 01:08 UTC · lore

The following patches improve the user experience for gitk in macOS by avoiding to abort if the terminal where it is running hasn't been authorized for Automation as reported a few[1,2] times already.

It has been tested in macOS 11.5.1 using both the system tk (8.5) and the latest (8.6.11) from brew, the third[3] patch is needed for using 8.6 and has been included with the gitk version from brew as well.

Carlo Marcelo Arenas Belón (2):
  gitk: skip calling osascript to set frontmost for tk >= 8.6
  gitk: avoid fatal error if `exec osascript` fails

Tobias Pietzsch (1):
  gitk: check main window visibility before waiting for it to show

 gitk | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

[1] https://lore.kernel.org/git/20180724065120.7664-1-sunshine@sunshineco.com/ [2] https://lore.kernel.org/git/20201025175149.11853-1-dev+git@drbeat.li/ [3] https://lore.kernel.org/git/pull.944.git.git.1610234771966.gitgitgadget@gmail.com/

-- 
2.33.0.rc0.433.g9a510e7e11

Carlo Marcelo Arenas Belón· Aug 4, 2021, 01:08 UTC · re: Carlo Marcelo Arenas Belón · lore

[PATCH 1/3] gitk: skip calling osascript to set frontmost for tk >= 8.6

76bf6ff (gitk: On OSX, bring the gitk window to front, 2013-04-24) adds an osascript to set the gitk window to the front to workaround and unfortunate default in older tk versions to add it to the back.

tk 8.6 or newer do not need that workaround so add a conditional check to skip it and while at it update the comment with the new name for the OS.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 gitk | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

Show changes to gitk +8 −6

diff --git a/gitk b/gitk
index 23d9dd1..f68d983 100755
--- a/gitk
+++ b/gitk
@@ -12288,13 +12288,15 @@ if {[catch {package require Tk 8.4} err]} {
     exit 1
 }
 
-# on OSX bring the current Wish process window to front
+# on macOS bring the current Wish process window to front if needed
 if {[tk windowingsystem] eq "aqua"} {
-    exec osascript -e [format {
-        tell application "System Events"
-            set frontmost of processes whose unix id is %d to true
-        end tell
-    } [pid] ]
+    if {$tcl_version < 8.6} {
+        exec osascript -e [format {
+            tell application "System Events"
+                set frontmost of processes whose unix id is %d to true
+            end tell
+        } [pid] ]
+    }
 }
 
 # Unset GIT_TRACE var if set

-- 
2.33.0.rc0.433.g9a510e7e11

Carlo Marcelo Arenas Belón· Aug 4, 2021, 01:09 UTC · re: Carlo Marcelo Arenas Belón · lore

[PATCH 3/3] gitk: check main window visibility before waiting for it to show

From: Tobias Pietzsch <pietzsch@mycroft.speedport.ip>

If the main window is already visible when gitk waits for it to become visible, gitk hangs forever. This commit adds a check whether the window is already visible. See https://wiki.tcl-lang.org/page/tkwait+visibility

Signed-off-by: Tobias Pietzsch <pietzsch@mycroft.speedport.ip>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 gitk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Show changes to gitk +1 −1

diff --git a/gitk b/gitk
index b62c12f..cc587b5 100755
--- a/gitk
+++ b/gitk
@@ -12664,7 +12664,7 @@ catch {
     wm iconphoto . -default gitlogo gitlogo32
 }
 # wait for the window to become visible
-tkwait visibility .
+if {![winfo viewable .]} {tkwait visibility .}
 set_window_title
 update
 readrefs

-- 
2.33.0.rc0.433.g9a510e7e11

Chris Torek· Aug 4, 2021, 06:27 UTC · re: Carlo Marcelo Arenas Belón · lore

Re: [PATCH 3/3] gitk: check main window visibility before waiting for it to show

On Tue, Aug 3, 2021 at 6:51 PM Carlo Marcelo Arenas Belón <carenas@gmail.com> wrote:

Show 27 quoted lines

>
> From: Tobias Pietzsch <pietzsch@mycroft.speedport.ip>
>
> If the main window is already visible when gitk waits for it to
> become visible, gitk hangs forever.
> This commit adds a check whether the window is already visible.
> See https://wiki.tcl-lang.org/page/tkwait+visibility
>
> Signed-off-by: Tobias Pietzsch <pietzsch@mycroft.speedport.ip>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  gitk | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gitk b/gitk
> index b62c12f..cc587b5 100755
> --- a/gitk
> +++ b/gitk
> @@ -12664,7 +12664,7 @@ catch {
>      wm iconphoto . -default gitlogo gitlogo32
>  }
>  # wait for the window to become visible
> -tkwait visibility .
> +if {![winfo viewable .]} {tkwait visibility .}
>  set_window_title
>  update
>  readrefs

I have no objection to this patch (and the other two look fine to me) but I would like to say that this whole idea looks racy in general, and that it would be better to fix this inside Tk itself: the internal visibility-wait should be doing the visibility-check already. Given that it's obviously *not*, this, as a workaround, seems OK.

Chris

Carlo Marcelo Arenas Belón· Aug 4, 2021, 01:08 UTC · re: Carlo Marcelo Arenas Belón · lore

[PATCH 2/3] gitk: avoid fatal error if `exec osascript` fails

starting with macOS 10.14 calling "System Events" requires an "Automation" grant, that will result in a fatal error showing:

  Error in startup script: 66:111: execution error: Not authorized to send Apple events to System Events. (-1743)

instead of aborting catch the error and print a suitable warning.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 gitk | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

Show changes to gitk +9 −5

diff --git a/gitk b/gitk
index f68d983..b62c12f 100755
--- a/gitk
+++ b/gitk
@@ -12291,11 +12291,15 @@ if {[catch {package require Tk 8.4} err]} {
 # on macOS bring the current Wish process window to front if needed
 if {[tk windowingsystem] eq "aqua"} {
     if {$tcl_version < 8.6} {
-        exec osascript -e [format {
-            tell application "System Events"
-                set frontmost of processes whose unix id is %d to true
-            end tell
-        } [pid] ]
+        if {[catch {
+            exec osascript -e [format {
+                tell application "System Events"
+                    set frontmost of processes whose unix id is %d to true
+                end tell
+            } [pid] ]
+        } ]} {
+            puts stderr [mc "Warning: 'System Events' access denied"]
+        }
     }
 }

-- 
2.33.0.rc0.433.g9a510e7e11

Carlo Arenas· Aug 4, 2021, 02:37 UTC · re: Carlo Marcelo Arenas Belón · lore

Re: [PATCH 0/3] gitk: macOS improvements

Tested in Windows (with 8.6) as well not to introduce any regressions

+CC Tobias (gmail instead of the one bouncing in the SoB)

On Tue, Aug 3, 2021 at 6:09 PM Carlo Marcelo Arenas Belón <carenas@gmail.com> wrote:

Show 24 quoted lines

>
> The following patches improve the user experience for gitk in macOS
> by avoiding to abort if the terminal where it is running hasn't been
> authorized for Automation as reported a few[1,2] times already.
>
> It has been tested in macOS 11.5.1 using both the system tk (8.5) and
> the latest (8.6.11) from brew, the third[3] patch is needed for using
> 8.6 and has been included with the gitk version from brew as well.
>
> Carlo Marcelo Arenas Belón (2):
>   gitk: skip calling osascript to set frontmost for tk >= 8.6
>   gitk: avoid fatal error if `exec osascript` fails
>
> Tobias Pietzsch (1):
>   gitk: check main window visibility before waiting for it to show
>
>  gitk | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
>
> [1] https://lore.kernel.org/git/20180724065120.7664-1-sunshine@sunshineco.com/
> [2] https://lore.kernel.org/git/20201025175149.11853-1-dev+git@drbeat.li/
> [3] https://lore.kernel.org/git/pull.944.git.git.1610234771966.gitgitgadget@gmail.com/
> --
> 2.33.0.rc0.433.g9a510e7e11