Changes to NSMenuItem breaks nib loading...

[gcasa]

The loading of nib files is broken on my machine (Debian 12, clang-14):

2024-06-02 01:28:54.333 NSMenuItem_nib_test[167605:167605] Exception occurred while loading model: [NSKeyedUnarchiver +decodeObjectForKey:]: value for key(NSKeyEquivModMask) is '1048576'
2024-06-02 01:28:54.333 NSMenuItem_nib_test[167605:167605] Failed to load Nib
2024-06-02 01:28:54.369 NSMenuItem_nib_test[167605:167605] Cannot load the main model file 'MainMenu'

The test for this that is failing is at https://github.com/gcasa/NSMenuItem_nib_test.

A similar test is for XIBs that is passing is at http://github.com/gcasa/NSMenuItem_test.

[gcasa]

I don't think this part of the change is correct:

      // Set the key mask when it is present; or default to NSCommandKeyMask
      // when not specified
      keyMask = (NSNumber*)[aDecoder decodeObjectForKey: @"NSKeyEquivModMask"];

      if (keyMask != nil)
        {
          [self setKeyEquivalentModifierMask: [keyMask intValue]];
        }
      else
        {
          [self setKeyEquivalentModifierMask: NSCommandKeyMask];
        }

It doesn't make sense to me why we should default to NSCommandKeyMask when no key equivalent is even set.

[gcasa]

Why are we decoding this as an object when doing keyed coding and NOT when doing it as a typedstream?

      [aDecoder decodeValueOfObjCType: @encode(id) at: &_keyEquivalent];
      [aDecoder decodeValueOfObjCType: @encode(NSUInteger) at: &_keyEquivalentModifierMask];
      if (version <= 3)
        {
          _keyEquivalentModifierMask = _keyEquivalentModifierMask << 16;
        }

[gcasa]

gui/NSButtonCell/encoding.m:
Failed test:     (2024-06-02 14:54:36.428 -0400)   encoding.m:63 ... Encoded key mask 0x2000000 matches expected key mask 0x2000000
--- Running tests in gui/NSCell ---
--- Running tests in gui/NSEvent ---
--- Running tests in gui/NSImage ---
--- Running tests in gui/NSMenuItem ---
--- Running tests in gui/NSNibLoading ---

gui/NSNibLoading/basic.m:
Failed test:     (2024-06-02 14:54:40.215 -0400)   basic.m:64 ... .nib file was loaded properly using loadNibNamed:owner:topLevelObjects:
--- Running tests in gui/NSParagraphStyle --

This is on a second Debian machine. So I have reproduced this in two places.

[gcasa]

This issue is in the release. This needs to be corrected.

[qmfrederik]

It doesn't make sense to me why we should default to NSCommandKeyMask when no key equivalent is even set.

That matches the Apple implementation, as can be seen here: https://github.com/qmfrederik/xib-tests/blob/main/DemoTests/DemoTests.m, and discussed here: #263

[gcasa]

  NS_DURING
  {
    [NSApplication sharedApplication];
  }
  NS_HANDLER
  {
    if ([[localException name] isEqualToString: NSInternalInconsistencyException ])
      SKIP("It looks like GNUstep backend is not yet installed")
  }
  NS_ENDHANDLER

In Tests/GSXib5Unarchiver/menu.m this is not being executed because the backend isn't installed on CI.

[gcasa]

This breaks several applications (Chess.app for one) which previously loaded the .nib test NSNibLoading under Test also fails when run locally. The test above does not run because the backend is NOT included when running CI under libs-gui.

The root of the issue is the code is getting an exception in this code under NSMenuItem.m:

      // Set the key mask when it is present; or default to NSCommandKeyMask
      // when not specified
      keyMask = (NSNumber*)[aDecoder decodeObjectForKey: @"NSKeyEquivModMask"];

      if (keyMask != nil)
        {

When trying to run the code in this test locally... https://github.com/gcasa/NSMenuItem_nib_test.

[gcasa]

It doesn't make sense to me why we should default to NSCommandKeyMask when no key equivalent is even set.

That matches the Apple implementation, as can be seen here: https://github.com/qmfrederik/xib-tests/blob/main/DemoTests/DemoTests.m, and discussed here: #263

That's fine if that is the case... my MAIN issue is the CRASH, the non-functional applications, and also the skipped test.

[gcasa]

@qmfrederik This change causes a clear regression in .nib loading and needs to be corrected. I am going to see if I can get the test running without back loaded to illustrate the issue. Please run make check locally before any changes as there may be differences in what is run locally and what is run on github as illustrated by this issue.

[gcasa]

@qmfrederik Another test being skipped that is more relevant to this issue is Tests/NSNibLoading/basic.m. This test attempts to load a nib file, a gorm file, and a xib file. The xib and gorm are passing and the nib is failing. This issue tells me that, whatever the original issue that inspired this change was, the root cause is in GSXib5KeyedUnarchiver, not in the individual initWithCoder: of the NSMenuItem class.

[rfm]

I think this testcase failure Failed test: (2024-06-02 14:54:36.428 -0400) encoding.m:63 ... Encoded key mask 0x2000000 matches expected key mask 0x2000000

May be a red herring (a faulty test which I have cleaned up).

[gcasa]

@rfm the test I cited above is more relevant (Tests/NSNibLoading/basic.m). NIB files are failing to load due to this change. The test you just mentioned I was aware of and is not the one I am concerned about. Also, as a general practice, people should be running make check before doing a commit.

[rfm]

Looking at the code, I think the issue is that MSMenuItem.m has [aCoder encodeInt: _keyEquivalentModifierMask forKey: @"NSKeyEquivModMask"]; but keyMask = (NSNumber*)[aDecoder decodeObjectForKey: @"NSKeyEquivModMask"]; One of those must be wrong, but I don't know which it should be.

[gcasa]

@rfm Well, previously, NSKeyEquivModMask was being decoded as an int, not an object until this change. Also, the file in question (both the nib and xib) were encoded on macOS. So I believe it's a safe bet that the decoding is wrong here.

[rfm]

OK, with that info it should be quite easy to fix. Give me half an hour.

[rfm]

Judging by the comment, I think the code was trying to decode an NSNumber as a test to see if the value was present in the archive (assuming that, if the decoded value is nil, the value is not in the archive). That's wrong because a nil value returned from an attempt to decode an object is not egitimate for a non-object value. So the fix was to revert to decoding an integer, and instead to call the -containsValueForKey: method to determine whether the value is present or not. Please give it a go.

[gcasa]

@rfm @qmfrederik This fixes the issue. @rfm thank you for doing that. I had thought last night that this was the case, but I am glad I got an independent opinion @qmfrederik please make sure that this code still fixes whatever issue you were attempting to correct in the first place and let us know.

As a general rule, please run make check locally after building to see if any of the tests fail locally. I realize that you guys aren't used to building GNUstep outside of the bespoke environment you have set up, but I did write a set of instructions for this on Confluence prior to January. If it helps, please refer to them for instructions on how to build GS as a standalone item. Thx

[gcasa]

Closing this for now, @qmfrederik please re-open a new issue if this breaks your fix

[ethanc8]

I did write a set of instructions for this on Confluence prior to January

Which confluence service is this referring to?

[gcasa]

I did write a set of instructions for this on Confluence prior to January

Which confluence service is this referring to?

The one at Keysight. I should have been more specific.