bug#16097: [PATCH] Treat windows on other spaces as not visible in Cocoa

bug#16097: [PATCH] Treat windows on other spaces as not visible in Cocoa

[David Benjamin]

[-- Attachment #1.1: Type: text/plain, Size: 1123 bytes --]

Here's a patch for something that's been bugging me. This makes Cmd-` cycle
through only the current space's windows, which is consistent with other
programs on OS X. It also matches the behavior of other-frame on X11 for
window managers that implement virtual desktops by mapping and unmapping
windows. The patch is attached.

It's not perfect; if you go into Mission Control or so and move a window
onto another space without switching spaces, it won't notice until you
switch spaces again. I wasn't able to find a notification to listen to;
it'd be nice to replace the visible field in a frame with some virtual call
that, in Cocoa, would just call out to [win isVisible] and [win
isOnActiveSpace], but that'd be a more invasive change. It also doesn't fix
the behavior where closing the last window on a space warps you to a window
on another space. I'm not sure yet what's causing that.

As far as copyright goes, if this is a large enough change to matter (a
decent chunk of the diff is just moving some code around), I'm a Google
employee. I'm told we already have an agreement on file with the FSF.

David Benjamin

[-- Attachment #1.2: Type: text/html, Size: 1293 bytes --]

[-- Attachment #2: 0001-In-nsterm-update-frame-visibility-on-space-change-an.patch --]
[-- Type: application/octet-stream, Size: 5718 bytes --]

From 3482a017935469aa2b6d5d759bfcf0446c56dc1f Mon Sep 17 00:00:00 2001
From: David Benjamin <davidben@google.com>
Date: Sat, 9 Nov 2013 19:01:00 -0500
Subject: [PATCH] In nsterm, update frame visibility on space change and
 application hide/unhide. This makes other-frame cycle through windows
 correctly.

* nsterm.m (updateVisibility:): New function from code in
windowDidMiniaturize and windowDidDeminiaturize.
(windowDidMiniaturize:, windowDidDeminiaturize:): Call updateVisibility.
(initFrameFromEmacs:): Register for notifications of application
hide/unhide and, on Cocoa, space changes.
(dealloc): Clean up notifications.

* nsterm.h (EmacsView): Declaration for updateVisibility.
---
 src/ChangeLog | 11 ++++++++
 src/nsterm.h  |  1 +
 src/nsterm.m  | 90 ++++++++++++++++++++++++++++++++++++++++++++++++-----------
 3 files changed, 85 insertions(+), 17 deletions(-)

diff --git a/src/ChangeLog b/src/ChangeLog
index 17ea85d..e261b7b 100644
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,14 @@
+2013-12-08  David Benjamin  <davidben@google.com>
+
+	* nsterm.m (updateVisibility:): New function from code in
+	windowDidMiniaturize and windowDidDeminiaturize.
+	(windowDidMiniaturize:, windowDidDeminiaturize:): Call
+	updateVisibility.
+	(initFrameFromEmacs:): Register for notifications of application
+	hide/unhide and, on Cocoa, space changes.
+	(dealloc): Clean up notifications.
+	* nsterm.h (EmacsView): Declaration for updateVisibility.
+
 2013-12-08  Jan Djärv  <jan.h.d@swipnet.se>
 
 	* nsterm.m (updateFrameSize:): Fix GNUStep toolbar not updating.
diff --git a/src/nsterm.h b/src/nsterm.h
index 0215f13..da9907f 100644
--- a/src/nsterm.h
+++ b/src/nsterm.h
@@ -173,6 +173,7 @@ typedef float EmacsCGFloat;
 #ifdef HAVE_NATIVE_FS
 - (void) updateCollectionBehaviour;
 #endif
+- (void) updateVisibility: (NSNotification *)notification;
 
 #ifdef NS_IMPL_GNUSTEP
 - (void)windowDidMove: (id)sender;
diff --git a/src/nsterm.m b/src/nsterm.m
index 9c87923..3bbcf08 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -4932,6 +4932,21 @@ not_in_argv (NSString *arg)
 - (void)dealloc
 {
   NSTRACE (EmacsView_dealloc);
+  [[NSNotificationCenter defaultCenter]
+      removeObserver: self
+                name: NSApplicationDidHideNotification
+              object: nil];
+  [[NSNotificationCenter defaultCenter]
+      removeObserver: self
+                name: NSApplicationDidUnhideNotification
+              object: nil];
+#if defined(NS_IMPL_COCOA) && \
+  MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_6
+  [[[NSWorkspace sharedWorkspace] notificationCenter]
+    removeObserver: self
+              name: NSWorkspaceActiveSpaceDidChangeNotification
+            object: nil];
+#endif /* NS_IMPL_COCOA && >= MAC_OS_X_VERSION_10_6 */
   [toolbar release];
   if (fs_state == FULLSCREEN_BOTH)
     [nonfs_window release];
@@ -6037,6 +6052,27 @@ if (cols > 0 && rows > 0)
   [NSApp registerServicesMenuSendTypes: ns_send_types
                            returnTypes: nil];
 
+  /* Update visibility state on application hide and unhide. */
+  [[NSNotificationCenter defaultCenter]
+      addObserver: self
+         selector: @selector (updateVisibility:)
+             name: NSApplicationDidHideNotification
+           object: nil];
+  [[NSNotificationCenter defaultCenter]
+      addObserver: self
+         selector: @selector (updateVisibility:)
+             name: NSApplicationDidUnhideNotification
+           object: nil];
+
+#if defined(NS_IMPL_COCOA) && \
+  MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_6
+  [[[NSWorkspace sharedWorkspace] notificationCenter]
+    addObserver: self
+       selector: @selector (updateVisibility:)
+           name: NSWorkspaceActiveSpaceDidChangeNotification
+         object: nil];
+#endif /* NS_IMPL_COCOA && >= MAC_OS_X_VERSION_10_6 */
+
   ns_window_num++;
   return self;
 }
@@ -6143,18 +6179,7 @@ if (cols > 0 && rows > 0)
 - (void)windowDidDeminiaturize: sender
 {
   NSTRACE (windowDidDeminiaturize);
-  if (!emacsframe->output_data.ns)
-    return;
-
-  SET_FRAME_ICONIFIED (emacsframe, 0);
-  SET_FRAME_VISIBLE (emacsframe, 1);
-  windows_or_buffers_changed = 63;
-
-  if (emacs_event)
-    {
-      emacs_event->kind = DEICONIFY_EVENT;
-      EV_TRAILER ((id)nil);
-    }
+  [self updateVisibility:nil];
 }
 
 
@@ -6175,16 +6200,47 @@ if (cols > 0 && rows > 0)
 - (void)windowDidMiniaturize: sender
 {
   NSTRACE (windowDidMiniaturize);
+  [self updateVisibility:nil];
+}
+
+- (void)updateVisibility: (NSNotification *)notification
+{
+  NSTRACE (updateVisibility);
   if (!emacsframe->output_data.ns)
     return;
 
-  SET_FRAME_ICONIFIED (emacsframe, 1);
-  SET_FRAME_VISIBLE (emacsframe, 0);
+  NSWindow *win = [self window];
+  BOOL on_active_space = YES;
+  if ([win respondsToSelector: @selector (isOnActiveSpace)])
+    on_active_space = [win isOnActiveSpace];
+  if (on_active_space && [win isVisible])
+    {
+      if (FRAME_VISIBLE_P (emacsframe) && !FRAME_ICONIFIED_P (emacsframe))
+        return;
 
-  if (emacs_event)
+      SET_FRAME_ICONIFIED (emacsframe, 0);
+      SET_FRAME_VISIBLE (emacsframe, 1);
+      windows_or_buffers_changed = 63;
+
+      if (emacs_event)
+        {
+          emacs_event->kind = DEICONIFY_EVENT;
+          EV_TRAILER ((id)nil);
+        }
+    }
+  else
     {
-      emacs_event->kind = ICONIFY_EVENT;
-      EV_TRAILER ((id)nil);
+      if (!FRAME_VISIBLE_P (emacsframe) && FRAME_ICONIFIED_P (emacsframe))
+        return;
+
+      SET_FRAME_ICONIFIED (emacsframe, 1);
+      SET_FRAME_VISIBLE (emacsframe, 0);
+
+      if (emacs_event)
+        {
+          emacs_event->kind = ICONIFY_EVENT;
+          EV_TRAILER ((id)nil);
+        }
     }
 }
 
-- 
1.8.3.4 (Apple Git-47)

bug#16097: [PATCH] Treat windows on other spaces as not visible in Cocoa

[Stefan Kangas]

David Benjamin <davidben@google.com> writes:

> Here's a patch for something that's been bugging me. This makes Cmd-` cycle through only the current space's windows, which is
> consistent with other programs on OS X. It also matches the behavior of other-frame on X11 for window managers that implement
> virtual desktops by mapping and unmapping windows. The patch is attached.
>
> It's not perfect; if you go into Mission Control or so and move a window onto another space without switching spaces, it won't notice
> until you switch spaces again. I wasn't able to find a notification to listen to; it'd be nice to replace the visible field in a frame with some
> virtual call that, in Cocoa, would just call out to [win isVisible] and [win isOnActiveSpace], but that'd be a more invasive change. It also
> doesn't fix the behavior where closing the last window on a space warps you to a window on another space. I'm not sure yet what's
> causing that.
>
> As far as copyright goes, if this is a large enough change to matter (a decent chunk of the diff is just moving some code around), I'm a
> Google employee. I'm told we already have an agreement on file with the FSF.

(This was sent 7 years ago but unfortunately never got a reply at the
time.)

The attached patch unfortunately no longer applies.

Is this still an issue on modern versions of Emacs?

Best regards,
Stefan Kangas

bug#16097: [PATCH] Treat windows on other spaces as not visible in Cocoa

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> Is this still an issue on modern versions of Emacs?

More information was requested two months ago, but none were given, so
I'm closing this bug report.  If this is something that's still a
problem, please respond to the debbugs address, and we'll reopen.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#16097: [PATCH] Treat windows on other spaces as not visible in Cocoa

[David Benjamin via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 715 bytes --]

I'm afraid that after seven years, I've long forgotten about this patch and
no longer use Emacs these days. But I did a quick build from source on
macOS, and it appears graphical Emacs still has this issue.

David

On Mon, Oct 12, 2020 at 9:43 PM Lars Ingebrigtsen <larsi@gnus.org> wrote:

> Stefan Kangas <stefan@marxist.se> writes:
>
> > Is this still an issue on modern versions of Emacs?
>
> More information was requested two months ago, but none were given, so
> I'm closing this bug report.  If this is something that's still a
> problem, please respond to the debbugs address, and we'll reopen.
>
> --
> (domestic pets only, the antidote for overdose, milk.)
>    bloggy blog: http://lars.ingebrigtsen.no
>

[-- Attachment #2: Type: text/html, Size: 1241 bytes --]

bug#16097: [PATCH] Treat windows on other spaces as not visible in Cocoa

[Lars Ingebrigtsen]

David Benjamin <davidben@google.com> writes:

> I'm afraid that after seven years, I've long forgotten about this
> patch and no longer use Emacs these days. But I did a quick build from
> source on macOS, and it appears graphical Emacs still has this issue.

Thanks for testing; I'm reopening this bug report.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#16097: [PATCH] Treat windows on other spaces as not visible in Cocoa

[Lars Ingebrigtsen]

David Benjamin <davidben@google.com> writes:

> Here's a patch for something that's been bugging me. This makes Cmd-`
> cycle through only the current space's windows, which is consistent
> with other programs on OS X. It also matches the behavior of
> other-frame on X11 for window managers that implement virtual desktops
> by mapping and unmapping windows. The patch is attached.

Perhaps Alan has a comment here; added to the CCs.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#16097: [PATCH] Treat windows on other spaces as not visible in Cocoa

[Robert Pluim]

[-- Attachment #1: Type: text/plain, Size: 761 bytes --]

>>>>> On Wed, 26 Jan 2022 17:46:15 +0100, Lars Ingebrigtsen <larsi@gnus.org> said:

    Lars> David Benjamin <davidben@google.com> writes:
    >> Here's a patch for something that's been bugging me. This makes Cmd-`
    >> cycle through only the current space's windows, which is consistent
    >> with other programs on OS X. It also matches the behavior of
    >> other-frame on X11 for window managers that implement virtual desktops
    >> by mapping and unmapping windows. The patch is attached.

    Lars> Perhaps Alan has a comment here; added to the CCs.

Please let's apply this. Please. Pretty please. Pretty please with sugar on
top. Please.

Iʼve rebased it to master and attach it here.

Did I mention I like it?

Robert
-- 


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Make-other-frame-cycle-on-current-space-only-nsterm.patch --]
[-- Type: text/x-diff, Size: 6308 bytes --]

From 16269d55a34cbc74335fc3729e92bcea1497842c Mon Sep 17 00:00:00 2001
From: David Benjamin <davidben@google.com>
Date: Thu, 27 Jan 2022 12:00:01 +0100
Subject: [PATCH] Make other-frame cycle on current space only (nsterm)
To: emacs-devel@gnu.org

In nsterm, update frame visibility on space change and application
hide/unhide. This makes other-frame cycle through windows correctly.

* nsterm.m (updateVisibility:): New function from code in
windowDidMiniaturize and windowDidDeminiaturize.
(windowDidMiniaturize:, windowDidDeminiaturize:): Call
updateVisibility.
(initFrameFromEmacs:): Register for notifications of application
hide/unhide and, on Cocoa, space changes.
(dealloc): Clean up notifications.
* nsterm.h (EmacsView): Declaration for updateVisibility.
---
 src/nsterm.h |   1 +
 src/nsterm.m | 113 +++++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 96 insertions(+), 18 deletions(-)

diff --git a/src/nsterm.h b/src/nsterm.h
index f027646123..db4e75641b 100644
--- a/src/nsterm.h
+++ b/src/nsterm.h
@@ -481,6 +481,7 @@ #define NSTRACE_UNSILENCE()
 #if defined (NS_IMPL_COCOA) && MAC_OS_X_VERSION_MAX_ALLOWED >= 1070
 - (void) updateCollectionBehavior;
 #endif
+- (void) updateVisibility: (NSNotification *)notification;
 
 #ifdef NS_IMPL_GNUSTEP
 - (void)windowDidMove: (id)sender;
diff --git a/src/nsterm.m b/src/nsterm.m
index a3c7b55218..cce2ada317 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -5959,6 +5959,22 @@ - (void)dealloc
 {
   NSTRACE ("[EmacsView dealloc]");
 
+  [[NSNotificationCenter defaultCenter]
+      removeObserver: self
+                name: NSApplicationDidHideNotification
+              object: nil];
+  [[NSNotificationCenter defaultCenter]
+      removeObserver: self
+                name: NSApplicationDidUnhideNotification
+              object: nil];
+#if defined(NS_IMPL_COCOA) && \
+  MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_6
+  [[[NSWorkspace sharedWorkspace] notificationCenter]
+    removeObserver: self
+              name: NSWorkspaceActiveSpaceDidChangeNotification
+            object: nil];
+#endif /* NS_IMPL_COCOA && >= MAC_OS_X_VERSION_10_6 */
+
   /* Clear the view resize notification.  */
   [[NSNotificationCenter defaultCenter]
     removeObserver:self
@@ -7066,7 +7082,6 @@ - (void)windowDidBecomeKey: (NSNotification *)notification
   [self windowDidBecomeKey];
 }
 
-
 - (void)windowDidBecomeKey      /* for direct calls */
 {
   struct ns_display_info *dpyinfo = FRAME_DISPLAY_INFO (emacsframe);
@@ -7086,7 +7101,27 @@ - (void)windowDidBecomeKey      /* for direct calls */
   XSETFRAME (event.frame_or_window, emacsframe);
   kbd_buffer_store_event (&event);
 }
+#if 0
+- (void)windowDidBecomeKey      /* for direct calls */
+{
+  struct ns_display_info *dpyinfo = FRAME_DISPLAY_INFO (emacsframe);
+  struct frame *old_focus = dpyinfo->ns_focus_frame;
+  struct input_event event;
+
+  EVENT_INIT (event);
+
+  NSTRACE ("[EmacsView windowDidBecomeKey]");
 
+  if (emacsframe != old_focus)
+    dpyinfo->ns_focus_frame = emacsframe;
+
+  ns_frame_rehighlight (emacsframe);
+
+  event.kind = FOCUS_IN_EVENT;
+  XSETFRAME (event.frame_or_window, emacsframe);
+  kbd_buffer_store_event (&event);
+}
+#endif
 
 - (void)windowDidResignKey: (NSNotification *)notification
 /* cf. x_detect_focus_change(), x_focus_changed(), x_new_focus_frame() */
@@ -7214,6 +7249,27 @@ - (instancetype) initFrameFromEmacs: (struct frame *)f
   [NSApp registerServicesMenuSendTypes: ns_send_types
                            returnTypes: [NSArray array]];
 
+  /* Update visibility state on application hide and unhide. */
+  [[NSNotificationCenter defaultCenter]
+      addObserver: self
+         selector: @selector (updateVisibility:)
+             name: NSApplicationDidHideNotification
+           object: nil];
+  [[NSNotificationCenter defaultCenter]
+      addObserver: self
+         selector: @selector (updateVisibility:)
+             name: NSApplicationDidUnhideNotification
+           object: nil];
+
+#if defined(NS_IMPL_COCOA) && \
+  MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_6
+  [[[NSWorkspace sharedWorkspace] notificationCenter]
+    addObserver: self
+       selector: @selector (updateVisibility:)
+           name: NSWorkspaceActiveSpaceDidChangeNotification
+         object: nil];
+#endif /* NS_IMPL_COCOA && >= MAC_OS_X_VERSION_10_6 */
+
   ns_window_num++;
   return self;
 }
@@ -7383,18 +7439,7 @@ - (NSRect)windowWillUseStandardFrame:(NSWindow *)sender
 - (void)windowDidDeminiaturize: sender
 {
   NSTRACE ("[EmacsView windowDidDeminiaturize:]");
-  if (!emacsframe->output_data.ns)
-    return;
-
-  SET_FRAME_ICONIFIED (emacsframe, 0);
-  SET_FRAME_VISIBLE (emacsframe, 1);
-  windows_or_buffers_changed = 63;
-
-  if (emacs_event)
-    {
-      emacs_event->kind = DEICONIFY_EVENT;
-      EV_TRAILER ((id)nil);
-    }
+  [self updateVisibility:nil];
 }
 
 
@@ -7415,16 +7460,48 @@ - (void)windowDidExpose: sender
 - (void)windowDidMiniaturize: sender
 {
   NSTRACE ("[EmacsView windowDidMiniaturize:]");
+  [self updateVisibility:nil];
+}
+
+- (void)updateVisibility: (NSNotification *)notification
+{
+  NSTRACE (updateVisibility);
+
   if (!emacsframe->output_data.ns)
     return;
 
-  SET_FRAME_ICONIFIED (emacsframe, 1);
-  SET_FRAME_VISIBLE (emacsframe, 0);
+  NSWindow *win = [self window];
+  BOOL on_active_space = YES;
+  if ([win respondsToSelector: @selector (isOnActiveSpace)])
+    on_active_space = [win isOnActiveSpace];
+  if (on_active_space && [win isVisible])
+    {
+      if (FRAME_VISIBLE_P (emacsframe) && !FRAME_ICONIFIED_P (emacsframe))
+        return;
 
-  if (emacs_event)
+      SET_FRAME_ICONIFIED (emacsframe, 0);
+      SET_FRAME_VISIBLE (emacsframe, 1);
+      windows_or_buffers_changed = 63;
+
+      if (emacs_event)
+        {
+          emacs_event->kind = DEICONIFY_EVENT;
+          EV_TRAILER ((id)nil);
+        }
+    }
+  else
     {
-      emacs_event->kind = ICONIFY_EVENT;
-      EV_TRAILER ((id)nil);
+      if (!FRAME_VISIBLE_P (emacsframe) && FRAME_ICONIFIED_P (emacsframe))
+        return;
+
+      SET_FRAME_ICONIFIED (emacsframe, 1);
+      SET_FRAME_VISIBLE (emacsframe, 0);
+
+      if (emacs_event)
+        {
+          emacs_event->kind = ICONIFY_EVENT;
+          EV_TRAILER ((id)nil);
+        }
     }
 }
 
-- 
2.34.0

bug#16097: [PATCH] Treat windows on other spaces as not visible in Cocoa

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Robert Pluim <rpluim@gmail.com> writes:

> Please let's apply this. Please. Pretty please. Pretty please with sugar on
> top. Please.

It breaks the fix for bug#53276 and probably the GNUstep build as well.

I don't use macOS and have no opinion about the feature in general, but
before applying it please give me a heads up, so I can fix bug#53276
again afterwards.

Thanks.

bug#16097: [PATCH] Treat windows on other spaces as not visible in Cocoa

[Robert Pluim]

>>>>> On Thu, 27 Jan 2022 21:59:44 +0800, Po Lu <luangruo@yahoo.com> said:

    Po> Robert Pluim <rpluim@gmail.com> writes:
    >> Please let's apply this. Please. Pretty please. Pretty please with sugar on
    >> top. Please.

    Po> It breaks the fix for bug#53276 and probably the GNUstep build as well.

So it does (and my patch contains an unrelated hunk, I probably
applied to an unclean tree).

    Po> I don't use macOS and have no opinion about the feature in general, but
    Po> before applying it please give me a heads up, so I can fix bug#53276
    Po> again afterwards.

It makes macOS consistent with other platforms. Anything that can
improve macOS' atrocious window handling is welcome.

We can try to retain the fix for 53276. Iʼd appreciate a hint :-)

Robert
-- 

bug#16097: [PATCH] Treat windows on other spaces as not visible in Cocoa

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Robert Pluim <rpluim@gmail.com> writes:

> We can try to retain the fix for 53276. Iʼd appreciate a hint :-)

Essentially, `updateVisibility' should be modified to declare a `struct
input_event', initialize that event, and use kbd_buffer_store_event
instead of the `emacs_event' global.

Thanks.

bug#16097: [PATCH] Treat windows on other spaces as not visible in Cocoa

[Robert Pluim]

[-- Attachment #1: Type: text/plain, Size: 1588 bytes --]

>>>>> On Thu, 27 Jan 2022 20:58:11 +0000, Alan Third <alan@idiocy.org> said:

    Alan> On Thu, Jan 27, 2022 at 12:06:36PM +0100, Robert Pluim wrote:
    >> >>>>> On Wed, 26 Jan 2022 17:46:15 +0100, Lars Ingebrigtsen <larsi@gnus.org> said:
    >> 
    Lars> David Benjamin <davidben@google.com> writes:
    >> >> Here's a patch for something that's been bugging me. This makes Cmd-`
    >> >> cycle through only the current space's windows, which is consistent
    >> >> with other programs on OS X. It also matches the behavior of
    >> >> other-frame on X11 for window managers that implement virtual desktops
    >> >> by mapping and unmapping windows. The patch is attached.
    >> 
    Lars> Perhaps Alan has a comment here; added to the CCs.
    >> 
    >> Please let's apply this. Please. Pretty please. Pretty please with sugar on
    >> top. Please.
    >> 
    >> Iʼve rebased it to master and attach it here.
    >> 
    >> Did I mention I like it?

    Alan> I'm afraid I don't much. It looks like the updateVisibility method
    Alan> doesn't actually do enough to replace the windowDidBecomeKey method.

Thatʼs an artifact of me screwing up the patch, I think. But it
probably needs updating anyway, as Po pointed out.

    Alan> It also needs a good clean. For example there's a hunk that appears to
    Alan> just be removing a single blank line and most of the #ifdef's are
    Alan> redundant. I guess some of the weirdness in it is just because it's so
    Alan> old.

Sorry, I messed up the rebase. The attached should be better.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Make-other-frame-cycle-on-current-space-only-nsterm.patch --]
[-- Type: text/x-diff, Size: 5215 bytes --]

From a7054fc3d66b48161b562a8715730c002994dc4a Mon Sep 17 00:00:00 2001
From: David Benjamin <davidben@google.com>
Date: Fri, 28 Jan 2022 09:11:09 +0100
Subject: [PATCH] Make other-frame cycle on current space only (nsterm)
To: emacs-devel@gnu.org

In nsterm, update frame visibility on space change and application
hide/unhide. This makes other-frame cycle through windows correctly.

* nsterm.m (updateVisibility:): New function from code in
windowDidMiniaturize and windowDidDeminiaturize.
(windowDidMiniaturize:, windowDidDeminiaturize:): Call
updateVisibility.
(initFrameFromEmacs:): Register for notifications of application
hide/unhide and, on Cocoa, space changes.
(dealloc): Clean up notifications.
* nsterm.h (EmacsView): Declaration for updateVisibility.
---
 src/nsterm.h |  1 +
 src/nsterm.m | 92 ++++++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 76 insertions(+), 17 deletions(-)

diff --git a/src/nsterm.h b/src/nsterm.h
index f027646123..db4e75641b 100644
--- a/src/nsterm.h
+++ b/src/nsterm.h
@@ -481,6 +481,7 @@ #define NSTRACE_UNSILENCE()
 #if defined (NS_IMPL_COCOA) && MAC_OS_X_VERSION_MAX_ALLOWED >= 1070
 - (void) updateCollectionBehavior;
 #endif
+- (void) updateVisibility: (NSNotification *)notification;
 
 #ifdef NS_IMPL_GNUSTEP
 - (void)windowDidMove: (id)sender;
diff --git a/src/nsterm.m b/src/nsterm.m
index a3c7b55218..ce933e73eb 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -5959,6 +5959,22 @@ - (void)dealloc
 {
   NSTRACE ("[EmacsView dealloc]");
 
+  [[NSNotificationCenter defaultCenter]
+      removeObserver: self
+                name: NSApplicationDidHideNotification
+              object: nil];
+  [[NSNotificationCenter defaultCenter]
+      removeObserver: self
+                name: NSApplicationDidUnhideNotification
+              object: nil];
+#if defined(NS_IMPL_COCOA) && \
+  MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_6
+  [[[NSWorkspace sharedWorkspace] notificationCenter]
+    removeObserver: self
+              name: NSWorkspaceActiveSpaceDidChangeNotification
+            object: nil];
+#endif /* NS_IMPL_COCOA && >= MAC_OS_X_VERSION_10_6 */
+
   /* Clear the view resize notification.  */
   [[NSNotificationCenter defaultCenter]
     removeObserver:self
@@ -7214,6 +7230,27 @@ - (instancetype) initFrameFromEmacs: (struct frame *)f
   [NSApp registerServicesMenuSendTypes: ns_send_types
                            returnTypes: [NSArray array]];
 
+  /* Update visibility state on application hide and unhide. */
+  [[NSNotificationCenter defaultCenter]
+      addObserver: self
+         selector: @selector (updateVisibility:)
+             name: NSApplicationDidHideNotification
+           object: nil];
+  [[NSNotificationCenter defaultCenter]
+      addObserver: self
+         selector: @selector (updateVisibility:)
+             name: NSApplicationDidUnhideNotification
+           object: nil];
+
+#if defined(NS_IMPL_COCOA) && \
+  MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_6
+  [[[NSWorkspace sharedWorkspace] notificationCenter]
+    addObserver: self
+       selector: @selector (updateVisibility:)
+           name: NSWorkspaceActiveSpaceDidChangeNotification
+         object: nil];
+#endif /* NS_IMPL_COCOA && >= MAC_OS_X_VERSION_10_6 */
+
   ns_window_num++;
   return self;
 }
@@ -7383,18 +7420,7 @@ - (NSRect)windowWillUseStandardFrame:(NSWindow *)sender
 - (void)windowDidDeminiaturize: sender
 {
   NSTRACE ("[EmacsView windowDidDeminiaturize:]");
-  if (!emacsframe->output_data.ns)
-    return;
-
-  SET_FRAME_ICONIFIED (emacsframe, 0);
-  SET_FRAME_VISIBLE (emacsframe, 1);
-  windows_or_buffers_changed = 63;
-
-  if (emacs_event)
-    {
-      emacs_event->kind = DEICONIFY_EVENT;
-      EV_TRAILER ((id)nil);
-    }
+  [self updateVisibility:nil];
 }
 
 
@@ -7415,16 +7441,48 @@ - (void)windowDidExpose: sender
 - (void)windowDidMiniaturize: sender
 {
   NSTRACE ("[EmacsView windowDidMiniaturize:]");
+  [self updateVisibility:nil];
+}
+
+- (void)updateVisibility: (NSNotification *)notification
+{
+  NSTRACE (updateVisibility);
+
   if (!emacsframe->output_data.ns)
     return;
 
-  SET_FRAME_ICONIFIED (emacsframe, 1);
-  SET_FRAME_VISIBLE (emacsframe, 0);
+  NSWindow *win = [self window];
+  BOOL on_active_space = YES;
+  if ([win respondsToSelector: @selector (isOnActiveSpace)])
+    on_active_space = [win isOnActiveSpace];
+  if (on_active_space && [win isVisible])
+    {
+      if (FRAME_VISIBLE_P (emacsframe) && !FRAME_ICONIFIED_P (emacsframe))
+        return;
 
-  if (emacs_event)
+      SET_FRAME_ICONIFIED (emacsframe, 0);
+      SET_FRAME_VISIBLE (emacsframe, 1);
+      windows_or_buffers_changed = 63;
+
+      if (emacs_event)
+        {
+          emacs_event->kind = DEICONIFY_EVENT;
+          EV_TRAILER ((id)nil);
+        }
+    }
+  else
     {
-      emacs_event->kind = ICONIFY_EVENT;
-      EV_TRAILER ((id)nil);
+      if (!FRAME_VISIBLE_P (emacsframe) && FRAME_ICONIFIED_P (emacsframe))
+        return;
+
+      SET_FRAME_ICONIFIED (emacsframe, 1);
+      SET_FRAME_VISIBLE (emacsframe, 0);
+
+      if (emacs_event)
+        {
+          emacs_event->kind = ICONIFY_EVENT;
+          EV_TRAILER ((id)nil);
+        }
     }
 }
 
-- 
2.34.0


[-- Attachment #3: Type: text/plain, Size: 12 bytes --]


Robert
-- 

bug#16097: [PATCH] Treat windows on other spaces as not visible in Cocoa

[Robert Pluim]

>>>>> On Fri, 28 Jan 2022 08:35:10 +0800, Po Lu via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org> said:

    Po> Robert Pluim <rpluim@gmail.com> writes:
    >> We can try to retain the fix for 53276. Iʼd appreciate a hint :-)

    Po> Essentially, `updateVisibility' should be modified to declare a `struct
    Po> input_event', initialize that event, and use kbd_buffer_store_event
    Po> instead of the `emacs_event' global.

I can do that, but blink-cursor-mode seems to be working fine with the
current changes (now that I actually know how itʼs supposed to function).

Robert
-- 

bug#16097: [PATCH] Treat windows on other spaces as not visible in Cocoa

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Robert Pluim <rpluim@gmail.com> writes:

> I can do that, but blink-cursor-mode seems to be working fine with the
> current changes (now that I actually know how itʼs supposed to function).

Does it work fine right after the frame is created (and you never focus
into it afterwards?)

Thanks.

bug#16097: [PATCH] Treat windows on other spaces as not visible in Cocoa

[Robert Pluim]

>>>>> On Fri, 28 Jan 2022 16:47:05 +0800, Po Lu <luangruo@yahoo.com> said:

    Po> Robert Pluim <rpluim@gmail.com> writes:
    >> I can do that, but blink-cursor-mode seems to be working fine with the
    >> current changes (now that I actually know how itʼs supposed to function).

    Po> Does it work fine right after the frame is created (and you never focus
    Po> into it afterwards?)

No, but then again current master doesnʼt blink the cursor then either.

Robert
-- 

bug#16097: [PATCH] Treat windows on other spaces as not visible in Cocoa

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Robert Pluim <rpluim@gmail.com> writes:

>>>>>> On Fri, 28 Jan 2022 16:47:05 +0800, Po Lu <luangruo@yahoo.com> said:
>
>     Po> Robert Pluim <rpluim@gmail.com> writes:
>     >> I can do that, but blink-cursor-mode seems to be working fine with the
>     >> current changes (now that I actually know how itʼs supposed to function).
>
>     Po> Does it work fine right after the frame is created (and you never focus
>     Po> into it afterwards?)
>
> No, but then again current master doesnʼt blink the cursor then either.

Works on GNUstep for me (emacs -Q, obviously).  I also tried on macOS,
and it works there as well.  You have to launch it from GWorkspace or
the Finder for the first frame created to be the key window to test this
properly.

Thanks.

bug#16097: [PATCH] Treat windows on other spaces as not visible in Cocoa

[Robert Pluim]

>>>>> On Fri, 28 Jan 2022 18:02:29 +0800, Po Lu <luangruo@yahoo.com> said:

    Po> Robert Pluim <rpluim@gmail.com> writes:
    >>>>>>> On Fri, 28 Jan 2022 16:47:05 +0800, Po Lu <luangruo@yahoo.com> said:
    >> 
    Po> Robert Pluim <rpluim@gmail.com> writes:
    >> >> I can do that, but blink-cursor-mode seems to be working fine with the
    >> >> current changes (now that I actually know how itʼs supposed to function).
    >> 
    Po> Does it work fine right after the frame is created (and you never focus
    Po> into it afterwards?)
    >> 
    >> No, but then again current master doesnʼt blink the cursor then either.

    Po> Works on GNUstep for me (emacs -Q, obviously).  I also tried on macOS,
    Po> and it works there as well.  You have to launch it from GWorkspace or
    Po> the Finder for the first frame created to be the key window to test this
    Po> properly.

Iʼd forgotten what a pain it is to get Emacs to launch from the Finder
as opposed to the cli. Anyway, the cursor blinks for me when launched
from the finder without a .emacs file

Robert
-- 

bug#16097: [PATCH] Treat windows on other spaces as not visible in Cocoa

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Robert Pluim <rpluim@gmail.com> writes:

> Iʼd forgotten what a pain it is to get Emacs to launch from the Finder
> as opposed to the cli. Anyway, the cursor blinks for me when launched
> from the finder without a .emacs file

Interesting, so maybe something causes emacs_event to be non-NULL when
updateVisibility is called.

Though I still think we shouldn't take that chance and modify
updateVisibility to store events directly into the keyboard buffer.

Thanks.

bug#16097: [PATCH] Treat windows on other spaces as not visible in Cocoa

[Alan Third]

On Fri, Jan 28, 2022 at 09:17:24AM +0100, Robert Pluim wrote:
> +- (void)updateVisibility: (NSNotification *)notification
> +{
> +  NSTRACE (updateVisibility);
> +
>    if (!emacsframe->output_data.ns)
>      return;
>  
> -  SET_FRAME_ICONIFIED (emacsframe, 1);
> -  SET_FRAME_VISIBLE (emacsframe, 0);
> +  NSWindow *win = [self window];
> +  BOOL on_active_space = YES;
> +  if ([win respondsToSelector: @selector (isOnActiveSpace)])
> +    on_active_space = [win isOnActiveSpace];
> +  if (on_active_space && [win isVisible])
> +    {
> +      if (FRAME_VISIBLE_P (emacsframe) && !FRAME_ICONIFIED_P (emacsframe))
> +        return;
>  
> -  if (emacs_event)
> +      SET_FRAME_ICONIFIED (emacsframe, 0);
> +      SET_FRAME_VISIBLE (emacsframe, 1);
> +      windows_or_buffers_changed = 63;
> +
> +      if (emacs_event)
> +        {
> +          emacs_event->kind = DEICONIFY_EVENT;
> +          EV_TRAILER ((id)nil);
> +        }
> +    }
> +  else
>      {
> -      emacs_event->kind = ICONIFY_EVENT;
> -      EV_TRAILER ((id)nil);
> +      if (!FRAME_VISIBLE_P (emacsframe) && FRAME_ICONIFIED_P (emacsframe))
> +        return;
> +
> +      SET_FRAME_ICONIFIED (emacsframe, 1);
> +      SET_FRAME_VISIBLE (emacsframe, 0);
> +
> +      if (emacs_event)
> +        {
> +          emacs_event->kind = ICONIFY_EVENT;
> +          EV_TRAILER ((id)nil);
> +        }
>      }
>  }

Why are we marking all frames not on the current space as iconified?
We're also, as far as I can tell, marking all invisible frames on the
current space as iconified too. We'll be sending these iconification
events to Emacs every time the space is switched, whether their status
has changed or not.

I'm unsure what happens to actually minimised frames here.

I'm still not sure this is the best approach versus, say modifying
candidate_frame in frame.c to check if the frame is on the active
space, similar to how it currently checks if the frames use the same
keyboard (which is a check that's not even relevant on macOS, afaict).

Even if it is the best approach it looks to me like it should really
be more streamlined. I don't think we want to call the same function
for (de)minimising and changing space. They strike me as distinctly
different operations and I think the conflation of the two is making
this code's logic... questionable.
-- 
Alan Third

bug#16097: [PATCH] Treat windows on other spaces as not visible in Cocoa

[Alan Third]

(Apparently this failed to send correctly the first time. I know
Robert already replied to it, but it's not in the mailing list
archive, and I don't see my second follow up to this bug either.)

On Thu, Jan 27, 2022 at 12:06:36PM +0100, Robert Pluim wrote:
> >>>>> On Wed, 26 Jan 2022 17:46:15 +0100, Lars Ingebrigtsen <larsi@gnus.org> said:
> 
>     Lars> David Benjamin <davidben@google.com> writes:
>     >> Here's a patch for something that's been bugging me. This makes Cmd-`
>     >> cycle through only the current space's windows, which is consistent
>     >> with other programs on OS X. It also matches the behavior of
>     >> other-frame on X11 for window managers that implement virtual desktops
>     >> by mapping and unmapping windows. The patch is attached.
> 
>     Lars> Perhaps Alan has a comment here; added to the CCs.
> 
> Please let's apply this. Please. Pretty please. Pretty please with sugar on
> top. Please.
> 
> Iʼve rebased it to master and attach it here.
> 
> Did I mention I like it?

I'm afraid I don't much. It looks like the updateVisibility method
doesn't actually do enough to replace the windowDidBecomeKey method.

It also needs a good clean. For example there's a hunk that appears to
just be removing a single blank line and most of the #ifdef's are
redundant. I guess some of the weirdness in it is just because it's so
old.

Is this really how other terms work, where visibility is tied into
which virtual desktop is "active"?

I'm not against this behaviour, I just don't like the look of this
particular patch very much.

-- 
Alan Third