bug#21862: 24.5; Commit 35cd5181450 breaks nsterm.m on Mac OS X 10.6

bug#21862: 24.5; Commit 35cd5181450 breaks nsterm.m on Mac OS X 10.6

[David Caldwell]

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


I'm getting this when trying to build on Mac OS X 10.6:

    CC       nsterm.o
  nsterm.m: In function '-[EmacsView toggleFullScreen:]':
  nsterm.m:7058: error: 'NSWindowWillEnterFullScreenNotification'
undeclared (first use in this function)
  nsterm.m:7058: error: (Each undeclared identifier is reported only once
  nsterm.m:7058: error: for each function it appears in.)
  nsterm.m:7066: error: 'NSWindowDidEnterFullScreenNotification'
undeclared (first use in this function)
  nsterm.m:7098: error: 'NSWindowWillExitFullScreenNotification'
undeclared (first use in this function)
  nsterm.m:7104: error: 'NSWindowDidExitFullScreenNotification'
undeclared (first use in this function)
  make[1]: *** [nsterm.o] Error 1
  make: *** [src] Error 2
  make failed


Looks like this commit is the culprit:

  commit 35cd51814507987b916c4b4e0a7b45e09e454341
  Author: David Reitter <david.reitter@gmail.com>
  Date:   Sat Nov 7 20:43:29 2015 -0500

      Provide NS notification objects where required to eliminate warnings

      * nsterm.m (windowDidResize:, toggleFullScreen:):
      Call notification functions with notification objects
      as per delegate APIs.


NSWindow.h says:

  APPKIT_EXTERN NSString * const NSWindowWillEnterFullScreenNotification
NS_AVAILABLE_MAC(10_7);
  APPKIT_EXTERN NSString * const NSWindowDidEnterFullScreenNotification
NS_AVAILABLE_MAC(10_7);
  APPKIT_EXTERN NSString * const NSWindowWillExitFullScreenNotification
NS_AVAILABLE_MAC(10_7);
  APPKIT_EXTERN NSString * const NSWindowDidExitFullScreenNotification
NS_AVAILABLE_MAC(10_7);

We could fix this by wrapping NSWindow*FullScreenNotification with
preprocessor guards. On the other hand, that's getting awfully bulky for
creating a bunch of NSNotifications that aren't ever used (follow the
function calls).

Perhaps we can fix the warning a different way?

-David


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4239 bytes --]

bug#21862: Acknowledgement (24.5; Commit 35cd5181450 breaks nsterm.m on Mac OS X 10.6)

[David Caldwell]

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

Hi David, I'm pulling you in to this bug report since it is about your
commit.

I tried reverting the commit in question
(35cd51814507987b916c4b4e0a7b45e09e454341) and I don't see any warning
generated with the latest XCode (Version 7.1.1 (7B1005)). Where were you
seeing these warnings?

-David



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4239 bytes --]

bug#21862: Acknowledgement (24.5; Commit 35cd5181450 breaks nsterm.m on Mac OS X 10.6)

[David Reitter]

Rather than revert the commit, let me just check in a fix.
- David

> On Nov 10, 2015, at 2:37 PM, David Caldwell <david@porkrind.org> wrote:
> 
> Hi David, I'm pulling you in to this bug report since it is about your
> commit.
> 
> I tried reverting the commit in question
> (35cd51814507987b916c4b4e0a7b45e09e454341) and I don't see any warning
> generated with the latest XCode (Version 7.1.1 (7B1005)). Where were you
> seeing these warnings?
> 
> -David
> 
> 

bug#21862: Acknowledgement (24.5; Commit 35cd5181450 breaks nsterm.m on Mac OS X 10.6)

[David Reitter]

Hi David (and Anders),

I have checked in some changes that should allow compilation on 10.6.8.
I do not have a machine to test, so please reopen this bug in case you encounter a problem.

- David


> On Nov 10, 2015, at 2:37 PM, David Caldwell <david@porkrind.org> wrote:
> 
> Hi David, I'm pulling you in to this bug report since it is about your
> commit.
> 
> I tried reverting the commit in question
> (35cd51814507987b916c4b4e0a7b45e09e454341) and I don't see any warning
> generated with the latest XCode (Version 7.1.1 (7B1005)). Where were you
> seeing these warnings?
> 
> -David
> 
> 

bug#21862: Acknowledgement (24.5; Commit 35cd5181450 breaks nsterm.m on Mac OS X 10.6)

[David Caldwell]

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

On 11/10/15 1:02 PM, David Reitter wrote:
> Hi David (and Anders),
> 
> I have checked in some changes that should allow compilation on 10.6.8.
> I do not have a machine to test, so please reopen this bug in case you encounter a problem.

For closure, I've tested on my 10.6 machine and your change works great:

builder10-6:src build$ make nsterm.o
  GEN      globals.h
  CC       nsterm.o
builder10-6:src build$

Thanks!

-David



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4239 bytes --]

bug#21862: Acknowledgement (24.5; Commit 35cd5181450 breaks nsterm.m on Mac OS X 10.6)

[Anders Lindgren]

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

Hi,

Unfortunately, when I compile on my 10.6.8 machine I get the following
warnings:

nsterm.m: In function ‘-[EmacsView windowWillEnterFullScreen:]’:
nsterm.m:6875: warning: ‘EmacsView’ may not respond to
‘-windowWillEnterFullScreen’
nsterm.m:6875: warning: (Messages without a matching method signature
nsterm.m:6875: warning: will be assumed to return ‘id’ and accept
nsterm.m:6875: warning: ‘...’ as arguments.)
nsterm.m: In function ‘-[EmacsView windowDidEnterFullScreen]’:
nsterm.m:6887: warning: passing argument 1 of
‘notificationWithName:object:’ from incompatible pointer type
nsterm.m:6887: warning: ‘EmacsView’ may not respond to
‘-windowDidEnterFullScreen:’
nsterm.m: In function ‘-[EmacsView windowWillExitFullScreen:]’:
nsterm.m:6925: warning: ‘EmacsView’ may not respond to
‘-windowWillExitFullScreen’
nsterm.m: In function ‘-[EmacsView windowDidExitFullScreen:]’:
nsterm.m:6942: warning: ‘EmacsView’ may not respond to ‘-windowDid

The attached patch eliminate them. Basically, it's declarations of the new
(and shadowed) functions and the notification name should be a NSString
(i.e. a @ was missing).

    -- Anders



On Tue, Nov 10, 2015 at 10:02 PM, David Reitter <david.reitter@gmail.com>
wrote:

> Hi David (and Anders),
>
> I have checked in some changes that should allow compilation on 10.6.8.
> I do not have a machine to test, so please reopen this bug in case you
> encounter a problem.
>
> - David
>
>
> > On Nov 10, 2015, at 2:37 PM, David Caldwell <david@porkrind.org> wrote:
> >
> > Hi David, I'm pulling you in to this bug report since it is about your
> > commit.
> >
> > I tried reverting the commit in question
> > (35cd51814507987b916c4b4e0a7b45e09e454341) and I don't see any warning
> > generated with the latest XCode (Version 7.1.1 (7B1005)). Where were you
> > seeing these warnings?
> >
> > -David
> >
> >
>
>

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

[-- Attachment #2: osx106.diff --]
[-- Type: text/plain, Size: 952 bytes --]

diff --git a/src/nsterm.h b/src/nsterm.h
index 1b330f0..a2f344e 100644
--- a/src/nsterm.h
+++ b/src/nsterm.h
@@ -412,6 +412,11 @@ void nstrace_leave(int *);
 - (void)windowDidMove: (id)sender;
 #endif
 - (int)fullscreenState;
+- (void)windowWillEnterFullScreen;
+- (void)windowDidEnterFullScreen;
+- (void)windowDidEnterFullScreen:(NSNotification *)notification;
+- (void)windowWillExitFullScreen;
+- (void)windowDidExitFullScreen;
 @end
 
 
diff --git a/src/nsterm.m b/src/nsterm.m
index 5c39d5c..a2b18cc 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -6867,7 +6867,7 @@ not_in_argv (NSString *arg)
 #endif
 
 #if !defined (NS_IMPL_COCOA) || MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7
-#define NSWindowDidEnterFullScreenNotification "NSWindowDidEnterFullScreenNotification"
+#define NSWindowDidEnterFullScreenNotification @"NSWindowDidEnterFullScreenNotification"
 #endif
 
 - (void)windowWillEnterFullScreen:(NSNotification *)notification

bug#21862: Acknowledgement (24.5; Commit 35cd5181450 breaks nsterm.m on Mac OS X 10.6)

[David Reitter]

On Nov 10, 2015, at 4:39 PM, Anders Lindgren <andlind@gmail.com> wrote:
> The attached patch eliminate them.

I don’t think you should unconditionally declare this one:

+- (void)windowDidEnterFullScreen:(NSNotification *)notification;

Only declare it for the 10.6 build, please.

The others are okay.  

> the notification name should be a NSString (i.e. a @ was missing).

OK, thanks, my bad.

- D

bug#21862: Acknowledgement (24.5; Commit 35cd5181450 breaks nsterm.m on Mac OS X 10.6)

[David Caldwell]

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

On 11/10/15 1:39 PM, Anders Lindgren wrote:
> Hi,
> 
> Unfortunately, when I compile on my 10.6.8 machine I get the following
> warnings:
> 
> nsterm.m: In function ‘-[EmacsView windowWillEnterFullScreen:]’:
> nsterm.m:6875: warning: ‘EmacsView’ may not respond to
> ‘-windowWillEnterFullScreen’
> nsterm.m:6875: warning: (Messages without a matching method signature
> nsterm.m:6875: warning: will be assumed to return ‘id’ and accept
> nsterm.m:6875: warning: ‘...’ as arguments.)
> nsterm.m: In function ‘-[EmacsView windowDidEnterFullScreen]’:
> nsterm.m:6887: warning: passing argument 1 of
> ‘notificationWithName:object:’ from incompatible pointer type
> nsterm.m:6887: warning: ‘EmacsView’ may not respond to
> ‘-windowDidEnterFullScreen:’
> nsterm.m: In function ‘-[EmacsView windowWillExitFullScreen:]’:
> nsterm.m:6925: warning: ‘EmacsView’ may not respond to
> ‘-windowWillExitFullScreen’
> nsterm.m: In function ‘-[EmacsView windowDidExitFullScreen:]’:
> nsterm.m:6942: warning: ‘EmacsView’ may not respond to ‘-windowDid
> 
> The attached patch eliminate them. Basically, it's declarations of the
> new (and shadowed) functions and the notification name should be a
> NSString (i.e. a @ was missing).

Hmm. I did *not* see those warnings, so I went back to my directory,
removed nsterm.o and used `make V=1` to see if my options were somehow
different. This time I got the same warnings you did. I have no idea why
I didn't see that the first time.

At any rate, I can confirm your patch removes the warnings on 10.6:

builder10-6:src build$ rm nsterm.o
builder10-6:src build$ make nsterm.o V=1
i686-apple-darwin10-gcc-4.2.1 -mmacosx-version-min=10.6  -std=gnu99 -c
-Demacs  -I. -I. -I../lib -I../lib         -I'/usr/include/libxml2'
     -MMD -MF deps/nsterm.d -MP      -g3 -O2   nsterm.m
builder10-6:src build$ rm nsterm.o
builder10-6:src build$ make nsterm.o V=1
i686-apple-darwin10-gcc-4.2.1 -mmacosx-version-min=10.6  -std=gnu99 -c
-Demacs  -I. -I. -I../lib -I../lib         -I'/usr/include/libxml2'
     -MMD -MF deps/nsterm.d -MP      -g3 -O2   nsterm.m
builder10-6:src build$

I did it twice because now I don't really trust it. :-)

-David



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4239 bytes --]

bug#21862: Acknowledgement (24.5; Commit 35cd5181450 breaks nsterm.m on Mac OS X 10.6)

[Anders Lindgren]

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

>
> I don’t think you should unconditionally declare this one:
>
> +- (void)windowDidEnterFullScreen:(NSNotification *)notification;
>

> Only declare it for the 10.6 build, please.
>

Ah, yes, that seems correct. It is provided from 10.7 onwards.

Alternatively, we can implement a non-notification variant of
windowDidBecomeKey (it doesn't use the notification anyway), that way we
can move the code from windowDidEnterFullScreen from the notification
variant to the non-notification variant, and avoid a lot of the
notification complexity.

    -- Anders

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

bug#21862: Acknowledgement (24.5; Commit 35cd5181450 breaks nsterm.m on Mac OS X 10.6)

[Anders Lindgren]

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

Hi,

here is another, cleaner, patch to the OS X 10.6.8 build problem. It adds a
non-notification version of windowDidBecomeKey, which
windowDidEnterFullScreen now calls. This mean that 1) all functions now use
the same pattern (the notification version calls the non-notification
version) and 2) there is no need for the locally defined symbol
NSWindowDidEnterFullScreenNotification any more.

David (R), if you think this is a good way to solve this, would you like to
commit this patch, or would you prefer me to do it?

    -- Anders

On Wed, Nov 11, 2015 at 7:56 AM, Anders Lindgren <andlind@gmail.com> wrote:

> I don’t think you should unconditionally declare this one:
>>
>> +- (void)windowDidEnterFullScreen:(NSNotification *)notification;
>>
>
>> Only declare it for the 10.6 build, please.
>>
>
> Ah, yes, that seems correct. It is provided from 10.7 onwards.
>
> Alternatively, we can implement a non-notification variant of
> windowDidBecomeKey (it doesn't use the notification anyway), that way we
> can move the code from windowDidEnterFullScreen from the notification
> variant to the non-notification variant, and avoid a lot of the
> notification complexity.
>
>     -- Anders
>
>
>

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

[-- Attachment #2: osx106-2.diff --]
[-- Type: text/plain, Size: 2074 bytes --]

diff --git a/src/nsterm.h b/src/nsterm.h
index 1b330f0..73b3d89 100644
--- a/src/nsterm.h
+++ b/src/nsterm.h
@@ -412,6 +412,11 @@ void nstrace_leave(int *);
 - (void)windowDidMove: (id)sender;
 #endif
 - (int)fullscreenState;
+- (void)windowWillEnterFullScreen;
+- (void)windowDidEnterFullScreen;
+- (void)windowWillExitFullScreen;
+- (void)windowDidExitFullScreen;
+- (void)windowDidBecomeKey;
 @end
 
 
diff --git a/src/nsterm.m b/src/nsterm.m
index 5c39d5c..397ade1 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -6440,6 +6440,12 @@ not_in_argv (NSString *arg)
 - (void)windowDidBecomeKey: (NSNotification *)notification
 /* cf. x_detect_focus_change(), x_focus_changed(), x_new_focus_frame() */
 {
+  [self windowDidBecomeKey];
+}
+
+
+- (void)windowDidBecomeKey      /* for direct calls */
+{
   struct ns_display_info *dpyinfo = FRAME_DISPLAY_INFO (emacsframe);
   struct frame *old_focus = dpyinfo->x_focus_frame;
 
@@ -6866,10 +6872,6 @@ not_in_argv (NSString *arg)
 }
 #endif
 
-#if !defined (NS_IMPL_COCOA) || MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7
-#define NSWindowDidEnterFullScreenNotification "NSWindowDidEnterFullScreenNotification"
-#endif
-
 - (void)windowWillEnterFullScreen:(NSNotification *)notification
 {
   [self windowWillEnterFullScreen];
@@ -6880,19 +6882,18 @@ not_in_argv (NSString *arg)
   fs_before_fs = fs_state;
 }
 
-- (void)windowDidEnterFullScreen /* provided for direct calls */
+- (void)windowDidEnterFullScreen:(NSNotification *)notification
 {
-  [self windowDidEnterFullScreen:
-	      [NSNotification notificationWithName:NSWindowDidEnterFullScreenNotification
-					    object:[self window]]];
+  [self windowDidEnterFullScreen];
 }
-- (void)windowDidEnterFullScreen:(NSNotification *)notification
+
+- (void)windowDidEnterFullScreen /* provided for direct calls */
 {
   NSTRACE ("windowDidEnterFullScreen");
   [self setFSValue: FULLSCREEN_BOTH];
   if (! [self fsIsNative])
     {
-      [self windowDidBecomeKey:notification];
+      [self windowDidBecomeKey];
       [nonfs_window orderOut:self];
     }
   else

bug#21862: Acknowledgement (24.5; Commit 35cd5181450 breaks nsterm.m on Mac OS X 10.6)

[David Reitter]

On Nov 14, 2015, at 3:14 PM, Anders Lindgren <andlind@gmail.com> wrote:

> David (R), if you think this is a good way to solve this, would you like to commit this patch, or would you prefer me to do it?

Yes, defining an extra windowDidBecomeKey function is prettier.
I think that’s okay to check in to master.  

For emacs-25, I think it’s good, too, as it is part of the original compile bug fix and reduces the chance of future problems rather than increasing it.  I defer to John Wiegley (cc’ed) regarding this.

Anders, I do think that the cleaner way would be to use notifications to run these functions, which might reduce extra calls.  However, that requires more testing that I was willing to do (on GNUStep too), because there’s potential for these methods getting called at a later point in time.

- David

bug#21862: Acknowledgement (24.5; Commit 35cd5181450 breaks nsterm.m on Mac OS X 10.6)

[John Wiegley]

>>>>> David Reitter <david.reitter@gmail.com> writes:

> For emacs-25, I think it’s good, too, as it is part of the original compile
> bug fix and reduces the chance of future problems rather than increasing it.
> I defer to John Wiegley (cc’ed) regarding this.

If you feel this improves upon an earlier fix, please commit to emacs-25.
We're only avoiding new functionality at this point, refinements are still OK.

John

bug#21862: Acknowledgement (24.5; Commit 35cd5181450 breaks nsterm.m on Mac OS X 10.6)

[Anders Lindgren]

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

>
> If you feel this improves upon an earlier fix, please commit to emacs-25.
> We're only avoiding new functionality at this point, refinements are still
> OK.
>

Perfect!

While we are mailing, I just as well say "hi" and introduce myself. I have
tried to take pick up the pieces of the OS X port, after Jan Djärvs
resignation. So far, I have fixed a number of bugs, including a fringe
rendering problem and an annoying startup crash.

I'm an somewhat of an Emacs old-timer and author of the "follow-mode" and
"auto-revert-mode" Emacs modules. Apart from that, I have written a number
of font-lock related packages. Professionally, I develop C compilers for
microcontrollers used in the embedded industry.

As I have small children and very limited time, I have tried to keep a low
profile, however feel free to contact me directly regarding OS X-related
issues.

Sincerely,
    Anders Lindgren (Lindydancer)

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

bug#21862: Acknowledgement (24.5; Commit 35cd5181450 breaks nsterm.m on Mac OS X 10.6)

[Anders Lindgren]

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

Hi,

I just pushed the patch on the emacs-25 branch (OK:ed by John). Hopefully,
everything should work properly this time.

    -- Anders

On Sat, Nov 14, 2015 at 9:23 PM, David Reitter <david.reitter@gmail.com>
wrote:

> On Nov 14, 2015, at 3:14 PM, Anders Lindgren <andlind@gmail.com> wrote:
>
> > David (R), if you think this is a good way to solve this, would you like
> to commit this patch, or would you prefer me to do it?
>
> Yes, defining an extra windowDidBecomeKey function is prettier.
> I think that’s okay to check in to master.
>
> For emacs-25, I think it’s good, too, as it is part of the original
> compile bug fix and reduces the chance of future problems rather than
> increasing it.  I defer to John Wiegley (cc’ed) regarding this.
>
> Anders, I do think that the cleaner way would be to use notifications to
> run these functions, which might reduce extra calls.  However, that
> requires more testing that I was willing to do (on GNUStep too), because
> there’s potential for these methods getting called at a later point in time.
>
> - David

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