patchGNUstep - Patches: patch #6286, NSBezierPath encode/decode...

Group
 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #6286: NSBezierPath encode/decode improperly implemented

Submitter:  Christopher Wojno <wojno>
Submitted:  Tue 27 Nov 2007 06:43:28 AM UTC
   
 
Category:  AppKit Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  FredKiefer Open/Closed:  Closed

Jump to the original submission

Tue 22 Jan 2008 09:37:04 PM UTC, comment #6: 

Most of this should be resolved in SVN for some time now. The remaining difference  @encode(enum NSBezierPathElement) should be down to a compiler bug, as discussed on the mailing list.

Fred Kiefer <FredKiefer>
Group Member
Thu 29 Nov 2007 07:37:49 PM UTC, comment #5: 

Ah, now I see what you were doing. You wanted it to fall through so you didn't have to write the encode again. That does not work, however, with the decoder as you saw. The encoder case is correct then. It's still broken without @encode(enum NSBezierPathElement) however.

Yet another decoding bug:
Curved line point decoding is done incorrectly. According to apple's documentation, the end point of a curved line is p[2], not p[0]. The control points take up p[0] and p[1]. See: http://developer.apple.com/documentation/Cocoa/Reference/ApplicationKit/Classes/NSBezierPath_Class/index.html#//apple_ref/doc/uid/TP40004008

Is it idiotic? Yes, completely. Would I have done it (with end points moving around in the array of points) that way? No way.

Change:
[self curveToPoint: pts[0] controlPoint1: pts[1] controlPoint2: pts[2]];

To:
[self curveToPoint: pts[2] controlPoint1: pts[0] controlPoint2: pts[1]];

I'm not sure how you wrote the rest of the class (I'm a bit busy with the end of the semester), but that is crucial for cross platform compatibility.

Glad this is getting looked at. Thanks Fred.

(file #14509)

Christopher Wojno <wojno>
Thu 29 Nov 2007 09:53:24 AM UTC, comment #4: 

First I have to apologize as I didn't find the time to test keyed encoding of NSBezierPath on the Apple. If we really try to support keyed encoding here, it should be compatible.

As for your latest problem, I just don't see the issue. What is wrong? To me it looks like the current code is handling both the cases, LineTo and MoveTo, correctly. But then I wrote the code in the first place and may not be the best person to spot a bug there. Ah now I see the problem, in the initWithCoder: case we are missing a break statement. But the other method still looks correct to me. In both cases we just need to encode one point and we share the code for that.

I will fix the missing break as soon as I know what keyed format Apple is using.

Fred Kiefer <FredKiefer>
Group Member
Thu 29 Nov 2007 09:37:22 AM UTC, comment #3: 

While we're at it: Here's more problems with this code. I've actually experienced corrupt archives because of it. I'm shocked I missed it the first round.

It does not encode MoveTo points, but then tries to decode them. It also decodes improperly, as the case statement would fall through to the LineTo statement. This caused double-plus-un-fun.

Working on: gcc (GCC) 4.1.2 (Gentoo 4.1.2 p1.0.2)

(file #14506)

Christopher Wojno <wojno>
Tue 27 Nov 2007 06:07:25 PM UTC, comment #2: 


> In your error message I can see that an NSKeyedArchiver gets
> used. As far as I can see, your patch doesn't implement
> the missing keyed archiving for NSBezierPath, so how does it
> help you?


NSKeyedArchiver/NSKeyedUnarchiver (and the non-keyed archivers) implicitly rely on participating classes implementing the NSCoding protocol, which is implemented in NSBezierPath in [NSBezierPath -encodeWithCoder:] and [NSBezierPath initWithCoder:]. This is how NSArchivers are able to construct a recursive graph of the instance variables of the NSBezierPath when the archiving is requested. When the archivers are invoked, they will call those two functions (which I have modified).

> In my code NSBezierPathElement is always an enumeration, why
> would the encoding stuff need the additional hint that it
> really is an enum?


I am at a loss for you there. I am using:
gcc (GCC) 3.4.6 [FreeBSD] 20060305
and
gcc (GCC) 4.1.2 (Gentoo 4.1.2 p1.0.2)

and my patch corrects the aforementioned error in both.

> How should that make any difference? Apart from readability of
> course. It may make a difference for keyed encoding though.


NSPoint is a struct. The Exception explained that Obj-C does not know how to encode structs. So I had assumed it would also cause problems. However, in light of the source of the encodePoint (which I did not look at because I have used it before with success and reliability), there is no point to the change other than it, as you said, improves readability and provides tighter typing information. I care only that the exception is no longer thrown. If you wish to excise this portion of the patch, you may do so. It is, after all, ultimately at your discretion. I must say that, given the exception message, I am thoroughly dumbfounded as to why @encode(NSPoint) works but the enum did not.

> It could well be that your patch works around limitations of
> the keyed encoding in base. We better should know about these,
> before we hide them.


I am afraid I do not know what you mean. I have only been working with GNUstep for little more than a week on a project for school.

The @encode(enum ID) is definitely a problem. I hope this fix may be applied to other places as well.

Christopher Wojno <wojno>
Tue 27 Nov 2007 05:43:40 PM UTC, comment #1: 

Thank you for the nice patch, but before applying it I would like to understand some details.

In your error message I can see that an NSKeyedArchiver gets used. As far as I can see, your patch doesn't implement the missing keyed archiving for NSBezierPath, so how does it help you?

In my code NSBezierPathElement is always an enumeration, why would the encoding stuff need the additional hint that it really is an enum? As far as I can see there is no struct called NSBezierPathElement.

The usage of encodePoint: is surely nice, but in NSCoder it is just defined as

- (void) encodePoint: (NSPoint)point
{
  [self encodeValueOfObjCType: @encode(NSPoint) at: &point];
}

How should that make any difference? Apart from readability of course. It may make a difference for keyed encoding though.

It could well be that your patch works around limitations of the keyed encoding in base. We better should know about these, before we hide them.

Fred Kiefer <FredKiefer>
Group Member
Tue 27 Nov 2007 06:43:28 AM UTC, original submission:  

NSBezierPath, as implemnted, will yield the following error message on archive/unarchive:

Exception: Uncaught exception NSInvalidArgumentException, reason: -[NSKeyedArchiver encodeValueOfObjCType:at:]: this archiver cannot e encode structs
Abort (core dumped)

Upon inspection of the source, I discovered @encode(NSBezierPathElement) (which compiles to a struct) was used instead of @encode(enum NSBezierPathElement) (enum is an Obj-C primitive) and made the corrections. While I was there, I fixed the NSPoint elements to use the encode/decodePoint as well. No doubt, these would have also caused the Exception to occur.

Tested very quickly. NSBezierPath can now be archived and unarchived.

Christopher Wojno <wojno>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #14509:  gnustep-gui-nsbezierpath-encode-missing-move-to-fix-and-point-order.patch added by wojno (3KiB - application/octet-stream - Fixed: @encode(enum), missing break statement in decode, point order in decode curveToLine)
file #14506:  gnustep-gui-nsbezierpath-encode-missing-move-to-fix.patch added by wojno (3KiB - application/octet-stream - Fixed Exception and the missing encodes and breaks)
file #14490:  gnustep-gui-nsbezierpath-encode-fix.patch added by wojno (3KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by FredKiefer (Posted a comment)
  • -email is unavailable- added by wojno (Patching goodness)
  • -email is unavailable- added by wojno (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2008-01-22 FredKiefer Open/ClosedOpen Closed
    2008-01-22 FredKiefer StatusNone Done
    2007-11-29 wojno Attached File- Added gnustep-gui-nsbezierpath-encode-missing-move-to-fix-and-point-order.patch, #14509
    2007-11-29 wojno Attached File- Added gnustep-gui-nsbezierpath-encode-missing-move-to-fix.patch, #14506
    2007-11-27 FredKiefer Priority7 - High 5 - Normal
        Assigned toNone FredKiefer
    2007-11-27 wojno Attached File- Added gnustep-gui-nsbezierpath-encode-fix.patch, #14490
        Carbon-Copy- Added -email is unavailable-

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code