Re: [PATCH (revised)] -setTarget: and -setAction of NSImageView

[Kazunobu Kuriyama]

Hi,

In a word, your comment is just "broader point of view" I've
been seeking for since this thread began.  Now I'm in favor
of the idea that NSImageCell should be a direct subclass of
NSCell.  I can't be against it any more.

In particular, what is written in the header file is definitely
decisive. (I was so stupid that I failed to ask about it.
If I asked it from the beginning, it would make the discussion
fairly shorter. :)

It is header files that neither programmers nor compilers
can't be against!

So, attached is an integrated patch which was made against the -gui
directory in the latest CVS.  Of course, modification was done
along with the idea mentioned above.

Could someone please check it and judge whether it is acceptable
or not?

Thanks a lot.

- Kazunobu Kuriyama


Gregory John Casamento wrote:

> Kazunobu,
>
> See below...
>
> --- Kazunobu Kuriyama <kazunobu.kuriyama@nifty.com> wrote:
>
> > Hi,
> >
> > Gregory John Casamento wrote:
>
> <... my assertion from previous email about maintaining compatibility ...>
>
> > In addition to the comment above, if someone could give another
> > one on how an implementation using NSActionCell is technically
> > inconvenient, I think it would be definitely decisive.
>
> It's inconvenient for a number of reasons:
> ------------------------------------------
> 1) It is in conflict with the documented hierarchy for this class (in both the
> docs and in the header NSImageCell.h).  
>
> 2) It will complicate decoding of previous versions of this class from .gorm
> files since you will need to call the initWithCoder for NSCell instead of the
> one for NSActionCell for all older versions of the class.   It is *NOT*
> acceptable at all to simply allow the older .gorm files to break.   
>
> As the maintainer of Gorm, the encoding of the classes in both gui and base are
> critical to .gorm files and are under my purview. 
>
>
> > The difference of the responsibilities between NSCell and
> > NSActionCell is so obvious that Apple's engineers should
> > know it. Nonetheless, they decided to make NSImageCell a
> > subclass of NSCell (assuming the present document is still
> > correct.). Because the decision appears utterly illogical
> > to me, I think it is possible that the present document is
> > incorrect.  If I knew the techical reason of the decision,
> > I could easily withdraw my first view on the issue, i.e.
> > use of NSActionCell.
>
> I don't know why they did it that way either, but lacking that I don't think
> that not knowing the reason is sufficient cause for changing the parent class
> especially when it's documented this way and it's in the headers.
>
>
> > > Before applying any patch, we should also verify the behavior of MOSX
> > against
> >
> > > GNUstep.
> > >
> > > Two questions should be answered before we proceed:
> > > 1) Does NSImageCell need to have an implementation for the target/action
> > > behaviour? (i.e. is the currently situation acceptable?)
> > As pointed out by Alexander Malmberg, this is clear from
> > the documentation found at
> > http://developer.apple.com/documentation/Cocoa/Conceptual/ImageView/. 
> > For your convenience, I quote part of the article here:
> >
> > <quote>
> > Using an Image View to Specify an Image
> > ----------------------------------------
> > You can use an image view as an image well, which lets a user specify
> > an image by dragging an image to it. Just use setEditable: with an
> > argument of YES.  When the user drags an image to the image view,
> > the image view replaces its old image and sends its action to its
> > target.
> > <end of quote>
> >
> > However, according to Fred Kiefer's comment, this is not included
> > in the OpenStep specs.
>
> True, NSImageCell was not present under the OpenStep spec.  GNUstep isn't,
> however, limited to the OpenStep spec and should not take liberties with the
> classes which aren't specified there.
>
> As I said before, we should follow the MOSX Cocoa Documentation.
>
>
> > > 2) Do we know what the behaviour is on MOSX?
> > Unfortunately, I have no access to MOSX.  If there's someone having
> > access to MOSX, I would appreciate it very much if she/he could try
> > a sample program to see what happens on the MOSX with NSImageView
> > having target/action.
>
> I do have access to MOSX. :)  It is in the MOSX documentation for Objective-C
> and Java with NSCell as the superclass.  It is also in the header as a subclass
> of NSCell.  The idea that it's a "mistake in the docs" is soundly defeated
> here.
>
>
> > The sample program is in CocoaProgramming-20021010/Chapter 9/Image Viewer 2
> > in a freely downloadable compressed file.  You can download it from
> > http://www.cocoaprogramming.net.  -setTarget: and -setAction methods
> > used with an NSImageView can be found in an init method of MYDocument.m.
> >
> > Needless to say, if the program is linked against GNUstep, it terminates
> > immediately after it begins running, due to an exception raised
> > from either of the methods.
>
> Well, that sounds like a problem which needs to be fixed, but changing the
> class hierarchy in a way which is incompatible with MOSX isn't the answer.
>
>
> > Thanks.
> >
> > - Kazunobu Kuriyama
>
> Later, GJC
>
> =====
> Gregory John Casamento -- CEO/President Open Logic Corp.
> -- bheron on #gnustep, #linuxstep, & #gormtalk ---------------- 
> Please sign the petition against software patents at: 
> http://www.petitiononline.com/pasp01/petition.html 
> -- Main Developer of Gorm (featured in April Linux Journal) ---
>
> __________________________________
> Do you Yahoo!?
> New Yahoo! Photos - easier uploading and sharing.
> http://photos.yahoo.com/
>
>  
2003-12-29 Kazunobu Kuriyama <kazunobu.kuriyama@nifty.com>

        * Headers/AppKit/NSImageCell.h: Add new ivars: _tag, _target,
        _action, and _control_view.  Redeclare methods relevant to
        target/action.
        * Source/NSImageCell.m (+initialize): Set version to 2 from 1.
        (-drawInteriorWithFrame:inView:): Set _control_view to the second
        parameter.
        (-action): Overriden.
        (-setAction:): Overriden.
        (-target): Overridden.
        (-setTarget:): Overridden.
        (-tag): Overridden.
        (-setTag:): Overridden.
        (-controlView): Overridden.
        (-encodeWithCoder:): Modified so that it can deals with new ivars.
        (-initWithCoder:): Modified so that it can deals with new ivars.
        * Source/NSImageView.m (-performDragOperation:): Let target invoke
        action when new image is set.  Add a check so that new image is set
        only if the view is editable.

diff -Naru gui/Headers/AppKit/NSImageCell.h gui.rev/Headers/AppKit/NSImageCell.h
--- gui/Headers/AppKit/NSImageCell.h    2003-12-29 13:57:42.000000000 +0900
+++ gui.rev/Headers/AppKit/NSImageCell.h        2003-12-29 14:10:59.000000000 
+0900
@@ -64,6 +64,12 @@
   NSImageFrameStyle _frameStyle;
   NSImageScaling _imageScaling;
   NSSize _original_image_size;
+
+  // [NSImageCell version] >= 2
+  int _tag;
+  id _target;
+  SEL _action;
+  id _control_view;
 }
 
 //
@@ -80,6 +86,20 @@
 - (NSImageFrameStyle) imageFrameStyle;
 - (void) setImageFrameStyle: (NSImageFrameStyle)aFrameStyle;
 
+//
+// Target and Action
+//
+// N.B: These methods are overridden so that the instances doesn't raise an
+//      exception when they receive a target or action message.
+//
+- (SEL) action;
+- (void) setAction: (SEL)aSelector;
+- (id) target;
+- (void) setTarget: (id)anObject;
+- (int) tag;
+- (void) setTag: (int)anInt;
+- (NSView *) controlView;
+
 @end
 
 #endif // _GNUstep_H_NSImageCell
diff -Naru gui/Source/NSImageCell.m gui.rev/Source/NSImageCell.m
--- gui/Source/NSImageCell.m    2003-12-29 13:57:43.000000000 +0900
+++ gui.rev/Source/NSImageCell.m        2003-12-29 14:01:32.000000000 +0900
@@ -41,7 +41,7 @@
 {
   if (self == [NSImageCell class])
     {
-      [self setVersion: 1];
+      [self setVersion: 2];
     }
 }
 
@@ -300,6 +300,9 @@
   // draw!
   [_cell_image compositeToPoint: position operation: NSCompositeSourceOver];
 
+  if (_control_view != controlView)
+    _control_view = controlView;
+
   if (_cell.shows_first_responder)
     NSDottedFrameRect(cellFrame);
 }
@@ -360,6 +363,44 @@
 }
 
 //
+//  Target and Action
+//
+- (SEL) action
+{
+  return _action;
+}
+
+- (void) setAction: (SEL)aSelector
+{
+  _action = aSelector;
+}
+
+- (id) target
+{
+  return _target;
+}
+
+- (void) setTarget: (id)anObject
+{
+  _target = anObject;
+}
+
+- (int) tag
+{
+  return _tag;
+}
+
+- (void) setTag: (int)anInt
+{
+  _tag = anInt;
+}
+
+- (NSView *) controlView
+{
+  return _control_view;
+}
+
+//
 // NSCoding protocol
 //
 - (void) encodeWithCoder: (NSCoder *)aCoder
@@ -370,6 +411,11 @@
   [aCoder encodeValueOfObjCType: @encode(NSImageFrameStyle) at: &_frameStyle];
   [aCoder encodeValueOfObjCType: @encode(NSImageScaling) at: &_imageScaling];
   [aCoder encodeSize: _original_image_size];
+
+  [aCoder encodeValueOfObjCType: @encode(int) at: &_tag];
+  [aCoder encodeConditionalObject: _target];
+  [aCoder encodeValueOfObjCType: @encode(SEL) at: &_action];
+  [aCoder encodeConditionalObject: _control_view];
 }
 
 - (id) initWithCoder: (NSCoder *)aDecoder
@@ -380,6 +426,15 @@
   [aDecoder decodeValueOfObjCType: @encode(NSImageFrameStyle) at: 
&_frameStyle];
   [aDecoder decodeValueOfObjCType: @encode(NSImageScaling) at: &_imageScaling];
   _original_image_size = [aDecoder decodeSize];
+
+  if ([aDecoder versionForClassName: @"NSImageCell"] >= 2)
+    {
+      [aDecoder decodeValueOfObjCType: @encode(int) at: &_tag];
+      _target = [aDecoder decodeObject];
+      [aDecoder decodeValueOfObjCType: @encode(SEL) at: &_action];
+      _control_view = [aDecoder decodeObject];
+    }
+
   return self;
 }
 
diff -Naru gui/Source/NSImageView.m gui.rev/Source/NSImageView.m
--- gui/Source/NSImageView.m    2003-12-29 13:57:43.000000000 +0900
+++ gui.rev/Source/NSImageView.m        2003-12-29 14:02:21.000000000 +0900
@@ -184,18 +184,30 @@
 - (BOOL) performDragOperation: (id <NSDraggingInfo>)sender
 {
   NSImage *image;
+  BOOL retval;
 
-  image = [[NSImage alloc] initWithPasteboard: [sender draggingPasteboard]];
-  if (image == nil)
+  if ([sender draggingSource] == self || ![self isEditable])
     {
-      return NO;
+      retval = NO;
     }
-  else 
+  else
     {
-      [self setImage: image];
-      RELEASE(image);
-      return YES;
+      image = [[NSImage alloc] initWithPasteboard: [sender 
draggingPasteboard]];
+      if (image == nil)
+       {
+         retval = NO;
+       }
+      else 
+       {
+         [self setImage: image];
+         [self sendAction: [[self cell] action]
+                       to: [[self cell] target]];
+         retval = YES;
+         RELEASE(image);
+       }
     }
+
+  return retval;
 }
 
 - (void) concludeDragOperation: (id <NSDraggingInfo>)sender