Cocoa: hiding implementation details and allowing subclassing of InnerView

[Neil Hodgson]

Currently, code that wants to use ScintillaView is exposed to the implementation of the class ScintillaCocoa which causes it to see and be dependent on almost all of Scintilla's implementation. The additional 30 or so headers included slows down building and running analysis tools and can cause client code to break when changes are made to private Scintilla headers.

To fix this, I'd like to remove the #import of ScintillaCocoa.h from ScintillaView.h and add to ScintillaView.h just the few definitions needed by clients. Essentially:

#define WM_COMMAND 1001
#define WM_NOTIFY 1002
namespace Scintilla {
typedef void(*SciNotifyFunc) (intptr_t windowid, unsigned int iMessage, uintptr_t wParam, uintptr_t lParam);
class ScintillaCocoa; // ScintillaView has a property of this type so empty definition allows header to compile
}

Platform.h should also be removed as its for internal communication between Scintilla and platform layers. Scintilla.h and SciLexer.h could also be removed but some may see these as a necessary part of the interface to ScintillaView so want them to remain bundled.

Some implementors may want to write their applications in pure Objective C instead of Objective C++ and there are C++ features exposed through Scintilla's headers that currently make this impossible. On other platforms, Scintilla is accessible from C.

If we could depend on the modern (64-bit) runtime, the @private implementation details in the ScintillaView.h classes could be hidden inside ScintillaView.mm. Probably still too early to drop 32-bit builds, though.



ScintillaView contains an NSScrollView to handle scrolling and InnerView and MarginView objects to implement the text and margins. Sometimes client code would like to change ScintillaView behaviour. This can be done in some cases by subclassing ScintillaView. For example, SciTE uses a subclass of ScintillaView to handle some mouse click events. However, much of the functionality is provided by the InnerView class and to a lesser extent MarginView and neither of these can be subclassed.

A bug report today, https://sourceforge.net/p/scintilla/bugs/1522/ , reports that scrolling with a wheel mouse is too slow and provides a patch to InnerView. While it would be best if this can be fixed well within Scintilla, if that is not possible, some application authors may want to include their own fix and not have it overwritten whenever Scintilla is updated.

This could be achieved through a few different mechanisms. One is to have a method on ScintillaView to create the InnerView object. Applications can subclass ScintillaView and return a subclass of InnerView with their changes. For example,

@interface WheelScrollInnerView : InnerView
@end

@interface EditView : ScintillaView;
@end

@implementation WheelScrollInnerView

// Override wheel scrolling as too slow with real wheel mouse.
- (void) scrollWheel: (NSEvent *) theEvent {
if (([theEvent modifierFlags] & NSCommandKeyMask) != 0) {
[super scrollWheel:theEvent];
} else {
int deltaX = theEvent.deltaX < 0. ? -floorf(theEvent.deltaX) : -ceilf(theEvent.deltaX);
int deltaY = theEvent.deltaY < 0. ? -floorf(theEvent.deltaY) : -ceilf(theEvent.deltaY);
[self.owner message: SCI_LINESCROLL wParam: deltaX lParam: deltaY];
}
}

@end

@implementation EditView;

- (InnerView *)allocateInner {
return [[WheelScrollInnerView alloc] init];
}

@end

Neil

[Mike Lischke]

Hey Neil,

Post by Neil Hodgson
Currently, code that wants to use ScintillaView is exposed to the implementation of the class ScintillaCocoa which causes it to see and be dependent on almost all of Scintilla's implementation. The additional 30 or so headers included slows down building and running analysis tools and can cause client code to break when changes are made to private Scintilla headers.
#define WM_COMMAND 1001
#define WM_NOTIFY 1002
namespace Scintilla {
typedef void(*SciNotifyFunc) (intptr_t windowid, unsigned int iMessage, uintptr_t wParam, uintptr_t lParam);
class ScintillaCocoa; // ScintillaView has a property of this type so empty definition allows header to compile
}

Sounds good. In fact MarginView and InnerView can both go in ScintillaView.mm.

Post by Neil Hodgson
Platform.h should also be removed as its for internal communication between Scintilla and platform layers. Scintilla.h and SciLexer.h could also be removed but some may see these as a necessary part of the interface to ScintillaView so want them to remain bundled.

Platform.h can go, but not Scintilla.h and SciLexer.h. That's because we allow direct calls which require values defined in these header files.

Post by Neil Hodgson
Some implementors may want to write their applications in pure Objective C instead of Objective C++ and there are C++ features exposed through Scintilla's headers that currently make this impossible. On other platforms, Scintilla is accessible from C.

Is this really worth a consideration? I think XCode can freely mix pure Obj-C and Obj-C++ without problems. That's a per-file decision, I think, and one that on that's almost artificial given that you use the same compiler for both.
We definitily cannot drop 32 bit support yet, but I'm surprised this is a function of the architecture. Never realized this before and you barely find help when looking for errors like "Instance variables may not be placed in class extension".

Post by Neil Hodgson
ScintillaView contains an NSScrollView to handle scrolling and InnerView and MarginView objects to implement the text and margins. Sometimes client code would like to change ScintillaView behaviour. This can be done in some cases by subclassing ScintillaView. For example, SciTE uses a subclass of ScintillaView to handle some mouse click events. However, much of the functionality is provided by the InnerView class and to a lesser extent MarginView and neither of these can be subclassed.
A bug report today, https://sourceforge.net/p/scintilla/bugs/1522/ , reports that scrolling with a wheel mouse is too slow and provides a patch to InnerView. While it would be best if this can be fixed well within Scintilla, if that is not possible, some application authors may want to include their own fix and not have it overwritten whenever Scintilla is updated.
This could be achieved through a few different mechanisms. One is to have a method on ScintillaView to create the InnerView object. Applications can subclass ScintillaView and return a subclass of InnerView with their changes. For example,

A factory method for inner classes? Hmm, if that's really needed. But that prevents us from moving the inner classes completely to the mm file and hiding it so from the public.

Mike
--
www.soft-gems.net

Mike

[Neil Hodgson]

Post by Mike Lischke
Platform.h can go, but not Scintilla.h and SciLexer.h. That's because we allow direct calls which require values defined in these header files.

OK.

Post by Mike Lischke
Is this really worth a consideration? I think XCode can freely mix pure Obj-C and Obj-C++ without problems. That's a per-file decision, I think, and one that on that's almost artificial given that you use the same compiler for both.

Many people strongly dislike C++ although that's been more of an issue on Linux than elsewhere. If no one wants change here then that's good.

Post by Mike Lischke
We definitily cannot drop 32 bit support yet, but I'm surprised this is a function of the architecture. Never realized this before and you barely find help when looking for errors like "Instance variables may not be placed in class extension".

Apple could have implemented the modern runtime for 32-bits on OS X - its available on 32-bit iOS. I'm quite happy with Apple improving Objective C but some features aren't available on older operating systems or for 32-bit builds. As well as the big impact of whether the modern runtime is available, there are less important differences. For example, weak references are available back to 10.6 but they don't automatically zero before 10.7.

https://developer.apple.com/library/ios/releasenotes/ObjectiveC/ObjCAvailabilityIndex/

http://www.effectiveobjectivec.com is useful.

Post by Mike Lischke
A factory method for inner classes? Hmm, if that's really needed. But that prevents us from moving the inner classes completely to the mm file and hiding it so from the public.

Its very common for people to ask for more control over behaviour, such as filtering particular keys or performing some action when the view is resized. Adding all these to Scintilla on all platforms would be complex and require much work. On Windows, its easy to subclass the Scintilla control and implement a wide selection of features. On Cocoa, much of the code is in InnerView and its difficult to specialise InnerView's behaviour.

If there is no demand for this, then hiding InnerView and MarginView will be a good simplification.

Neil

[Neil Hodgson]

A change that removes imports of Platform.h and ScintillaCocoa.h from ScintillaView.h has been committed. This is the minimum change that hides most of Scintilla's implementation from client code. More simplification of the ScintillaView.h header may be possible in the future.

http://sourceforge.net/p/scintilla/code/ci/a77c1573074fd71303cf9e68b798f39b36156a3e/

Application code may be using features from ScintillaCocoa. Since the implementation of ScintillaCocoa can change, that code should be modified to use features from ScintillaView where possible. If functions are needed that are not available, please mention them so that they can be added to ScintillaView.

There is a possibility that some application code may rely on system headers that are no longer visible so may not compile. For example, if std::vector is used without #include <vector>, then #include <vector> should be added to the application.

Neil

[Chinh Nguyen]

Post by Neil Hodgson
If there is no demand for this, then hiding InnerView and MarginView
will be a good simplification.

Count me in as someone who would like the ability to subclass InnerView.
Right now, I have to merge over my changes to the InnerView every time I
upgrade to the latest version of Scintilla.

Changes like setting/unsetting the focus with SCI_SETFOCUS whenever my
window becomes/resigns key, overriding acceptsFirstResponder/becomeFirstResponder
and keyDown:, and making the scroll wheel handling a little more sensitive
for third party mice.

I've also had to make changes to ScintillaCocoa to get a little more
information when files are dragged and dropped onto my view. I disabled
scroll wheel with command key modifier to zoom because if your view is
still scrolling due to kinetic scrolling and you press command-W to close
the window the view is in, the view is zoomed in/out (depending on the
direction of the scroll) right before it closes.

In addition, I wish I could provide a notification function that returns a
value to determine whether the default notification function should be
called or whether the notification has been handled. This would allow me
to change the behavior of a few notifications without having to worry about
keeping my notification function in sync with any possible future changes
to ScintillaView's default notification function. Or at least don't
declare it as a static.

[Neil Hodgson]

In addition, I wish I could provide a notification function that returns a value to determine whether the default notification function should be called or whether the notification has been handled. This would allow me to change the behavior of a few notifications without having to worry about keeping my notification function in sync with any possible future changes to ScintillaView's default notification function. Or at least don't declare it as a static.

For SciTE, I just replace the notification function with my own by calling registerNotifyCallback:value:. Most of the behaviour of notification is to dispatch notifications to watchers, including the info bar which isn't present in SciTE. It also toggles folds but assumes that margin 2 is the fold margin which isn't really safe. The current best way to implement automatic folding is http://www.scintilla.org/ScintillaDoc.html#SCI_SETAUTOMATICFOLD

registerNotifyCallback:value: is something I added to ScintillaView because I already had a lot of code that dealt with Scintilla at a low level and didn't want any of the behaviour or overhead of the notification function. Its not great as it overlaps with the ScintillaView delegate and exposes the type ScintiillaCocoa. If I'd understood Cocoa better at that time it would have been implemented as a delegate protocol.

There should be a low-cost way to receive all notifications from Scintilla in user code along with a way to opt out or in to the additional processing performed by the notification function. However, client code is built to the current interface and we should avoid breaking clients.

The current ScintillaView delegate receives all WM_NOTIFY notifications but there is no way to opt out of the default processing of SCN_ZOOM or SCN_UPDATEUI. The delegate does not receive WM_COMMAND which is mostly wanted for changes to focus. Most of the code in the notification function would be better moved into a ScintillaView method where it can be overridden.

Neil

[Neil Hodgson]

Post by Neil Hodgson
Most of the code in the notification function would be better moved into a ScintillaView method where it can be overridden.

Here is a proposed implementation which allows clients to subclass ScintillaView, override the notification method, but reuse the ScintillaView implementation when wanted with [super notification:...].

- (void) notification: (unsigned int) iMessage wParam: (uintptr_t) wParam lParam: (uintptr_t) lParam
{
// Copy in the processing code from the current static notification function
}

static void forwardNotification(intptr_t windowid, unsigned int iMessage, uintptr_t wParam, uintptr_t lParam)
{
ScintillaView *view = (ScintillaView*)windowid;
[view notification:iMessage wParam:wParam lParam:lParam];
}

- (id) initWithFrame: (NSRect) frame
{
// ...
mBackend->RegisterNotifyCallback((uintptr_t)self, forwardNotification);
}

Neil

[Mike Lischke]

Post by Neil Hodgson

Post by Neil Hodgson
Most of the code in the notification function would be better moved into a ScintillaView method where it can be overridden.

Here is a proposed implementation which allows clients to subclass ScintillaView, override the notification method, but reuse the ScintillaView implementation when wanted with [super notification:...].
- (void) notification: (unsigned int) iMessage wParam: (uintptr_t) wParam lParam: (uintptr_t) lParam
{
// Copy in the processing code from the current static notification function
}

Sounds good.

Post by Neil Hodgson
static void forwardNotification(intptr_t windowid, unsigned int iMessage, uintptr_t wParam, uintptr_t lParam)
{
ScintillaView *view = (ScintillaView*)windowid;
[view notification:iMessage wParam:wParam lParam:lParam];
}
- (id) initWithFrame: (NSRect) frame
{
// ...
mBackend->RegisterNotifyCallback((uintptr_t)self, forwardNotification);
}

I can imagine some more changes to make this code a better citizen in the Cocoa/Obj-C world, like defining a protocol for this and maybe other callbacks from ScintillaCocoa. ScintillaView would implement that and set itself as delegate in ScintillaCocoa. We wouldn't need a static forwardNotification function then.

Mike
--
www.soft-gems.net

Mike

[Neil Hodgson]

Post by Mike Lischke
I can imagine some more changes to make this code a better citizen in the Cocoa/Obj-C world, like defining a protocol for this and maybe other callbacks from ScintillaCocoa. ScintillaView would implement that and set itself as delegate in ScintillaCocoa. We wouldn't need a static forwardNotification function then.

To minimize entities and disruption, the existing ScintillaNotificationProtocol can be reused and extended with an optional 'command' method:

@protocol ScintillaNotificationProtocol
- (void)notification: (Scintilla::SCNotification*)notification;
@optional
- (void)command:(int)command ctrlID:(int)ctrlID;
@end

The old RegisterNotifyCallback mechanism provides more context information: it allows passing in a context pointer and one parameter of the callback is a pointer to the ScintillaCocoa object. Other delegates in Cocoa don't seem to allow passing in a context pointer. The ctrlID is used for command since that is similar to other platforms, but it may help to add a pointer to the ScintillaView object. This helps clients that manage multiple ScintillaView objects.

ScintillaView implements the ScintillaNotificationProtocol protocol to receive updates from ScintillaCocoa and also forwards notifications to its delegate.

@interface ScintillaView : NSView <InfoBarCommunicator, ScintillaNotificationProtocol>
{
//...
- (void) notification: (Scintilla::SCNotification*)notification;
- (void) command:(int)command ctrlID:(int)ctrlID;
}

It sets the ScintillaCocoa delegate to itself in its initializer

mBackend->SetDelegate(self);

The protocol implementation in ScintillaView is responsible for both forwarding the messages and performing local processing. A subclass can override these methods to stop the local processing.

- (void) notification: (Scintilla::SCNotification*)scn
{
// Parent notification. Details are passed as SCNotification structure.

if (mDelegate != nil)
{
[mDelegate notification: scn];
if (scn->nmhdr.code != SCN_ZOOM && scn->nmhdr.code != SCN_UPDATEUI)
return;
}

switch (scn->nmhdr.code)
{
case SCN_MARGINCLICK:
//...
}

- (void) command:(int)command ctrlID:(int)ctrlID
{
if ((mDelegate != nil) && ([(id)mDelegate respondsToSelector:@selector(command:ctrlID:)]))
{
[mDelegate command:command ctrlID:ctrlID];
}

// Notifications for the editor itself.
switch (command)
{
case SCEN_KILLFOCUS:
[self sendNotification: NSTextDidEndEditingNotification];
break;
case SCEN_SETFOCUS: // Nothing to do for now.
break;
}
}

ScintillaCocoa has a delegate member, a SetDelegate method, and calls the delegate (if set) after each notify callback call (if set) like

void ScintillaCocoa::NotifyParent(SCNotification scn)
{
scn.nmhdr.hwndFrom = (void*) this;
scn.nmhdr.idFrom = GetCtrlID();
if (notifyProc != NULL)
notifyProc(notifyObj, WM_NOTIFY, GetCtrlID(), (uintptr_t) &scn);
if (delegate)
[delegate notification:&scn];
}

Neil

[Mike Lischke]

Neil,

Post by Neil Hodgson
@protocol ScintillaNotificationProtocol
- (void)notification: (Scintilla::SCNotification*)notification;
@optional
- (void)command:(int)command ctrlID:(int)ctrlID;
@end
The old RegisterNotifyCallback mechanism provides more context information: it allows passing in a context pointer and one parameter of the callback is a pointer to the ScintillaCocoa object. Other delegates in Cocoa don't seem to allow passing in a context pointer. The ctrlID is used for command since that is similar to other platforms, but it may help to add a pointer to the ScintillaView object. This helps clients that manage multiple ScintillaView objects.
ScintillaView implements the ScintillaNotificationProtocol protocol to receive updates from ScintillaCocoa and also forwards notifications to its delegate.
@interface ScintillaView : NSView <InfoBarCommunicator, ScintillaNotificationProtocol>
{
//...
- (void) notification: (Scintilla::SCNotification*)notification;
- (void) command:(int)command ctrlID:(int)ctrlID;
}
It sets the ScintillaCocoa delegate to itself in its initializer
mBackend->SetDelegate(self);

Sounds good. I was thinking if a property would be better, but I guess it's fine that way since ScintillaCocoa is a descendant of a ScintillaBase. The delegate in ScintillaView was originally meant to allow customizing the behavior, but I guess it doesn't allow enough control, so I'm fine with your proposal.

Mike

[Neil Hodgson]

Post by Mike Lischke
Sounds good. I was thinking if a property would be better, but I guess it's fine that way since ScintillaCocoa is a descendant of a ScintillaBase.

The C++ and Objective-C object models are separate: you can't subclass C++ in Objective-C or subclass Objective-C in C++. It would vastly simplify SciTE if either of these were possible.

Objective-C properties can't be implemented on C++ classes. ScintillaCocoa could expose a delegate field but that wouldn't allow extra processing as is currently done to cache whether the delegate implements the optional command:idFrom: method in hasCommand.

Client code should now move to implementing a delegate or subclassing ScintillaView instead of using registerNotifyCallback:. The release after next will formally deprecate this method using __attribute__((deprecated)).

Committed with this change set:
http://sourceforge.net/p/scintilla/code/ci/6b9c0b650df17fcfd253c1bb20dddfb5192d603d/

Neil

[Neil Hodgson]

Post by Neil Hodgson
This could be achieved through a few different mechanisms. One is to have a method on ScintillaView to create the InnerView object. Applications can subclass ScintillaView and return a subclass of InnerView with their changes.

Customizing InnerView has been implemented but in a slightly different way as a ScintillaView class method returning a class object:

+ (Class) innerViewClass
{
return [InnerView class];
}

Client code can substitute their own InnerView subclass by overriding this in their ScintillaView subclass:

+ (Class) innerViewClass {
return [WheelScrollInnerView class];
}

Neil

[Mike Lischke]

Post by Neil Hodgson
+ (Class) innerViewClass
{
return [InnerView class];
}
+ (Class) innerViewClass {
return [WheelScrollInnerView class];
}

Hmm, that's one way to do it, however it requires to subclass ScintillaView. Usually, in Cocoa, this substitution is done differently. E.g. look at NSScrollView. You can set a document and a content view of your choice ("dependency injection" principle). No need to also subclass the container if you just want to replace the content. And maybe it would make sense to rename this inner class to contentView as well.

Mike

[Neil Hodgson]

Post by Mike Lischke
Hmm, that's one way to do it, however it requires to subclass ScintillaView. Usually, in Cocoa, this substitution is done differently.

There are multiple methods used by Apple. This one was copied from the way that UIView subclasses define their layer class
https://developer.apple.com/library/ios/documentation/uikit/reference/uiview_class/UIView/UIView.html#//apple_ref/occ/clm/UIView/layerClass

Post by Mike Lischke
E.g. look at NSScrollView. You can set a document and a content view of your choice ("dependency injection" principle).

Allowing setting the content view after construction would require more logic since the configuration done in the initialization code would have to be moved or duplicated. The previous content view would need to be removed and destroyed. It doesn't appear worth the work but if someone is prepared to implement this then its OK by me.

Post by Mike Lischke
And maybe it would make sense to rename this inner class to contentView as well.

A bare "ContentView" is a bit too generic. There's a convention that a project chooses a three letter prefix ("SCI" for Scintilla) so "SCIContentView" and "SCIMarginView". Not really backwards compatible to change to "SCIScintillaView".
https://developer.apple.com/library/ios/documentation/cocoa/conceptual/ProgrammingWithObjectiveC/Conventions/Conventions.html

Neil