discuss-gnustep
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: ANN: PikoPixel pixel-art editor


From: Josh Freeman
Subject: Re: ANN: PikoPixel pixel-art editor
Date: Mon, 21 Sep 2015 03:17:23 -0400

Hi Nikolaus,

Agreed, method-swizzling should generally be avoided in applications. It's used in PikoPixel as a way to compartmentalize functionality that doesn't belong in the application - especially functionality that'll be removed at some point.

What belongs in the PikoPixel application are code & resources that support drawing & editing pixel art; What doesn't belong are:

1. GNUstep glue (PPGNUstepGlue_*.m, PPGNUstepCocoaGlue.m)

These files fill in the few differences/gaps in functionality between the Mac & GNUstep frameworks that affect PP. Each PPGNUstepGlue_*.m source file contains all the workaround code targeted at a single issue or several connected issues, usually patching multiple methods in one or more classes. (PPGNUstepCocoaGlue.m & PPGNUstepGlue_Miscellaneous.m contain fixes for various unrelated issues that have only one or two patches).

While this functionality is currently required for PP to run on GNUstep, it belongs in the GS frameworks, and I plan to submit patches/ bug-reports for the remaining issues (Fred has already checked in various fixes - thanks! - so some PPGNUstepGlue_*.m patches are already obsolete). As an issue gets updated in GS, the corresponding patches can easily be removed from PP because they're all in one place.


2. Screencasting popup (PPScreencastingController.m, PPApplication_Screencasting.m)

When enabled, a popup window shows up whenever a key is pressed or mouse clicked. It's useful as a demonstration aid to let others know the current keyboard/mouse state during a screencast or live demo of PP. However, it's unrelated to the pixel-art purpose of the app, and will be used rarely by only a small percentage of users. It's also currently disabled on GNUstep due to GS's missing NSMenuDidEndTrackingNotification functionality.

Keyboard/mouse-state display belongs elsewhere, such as a screencasting-manager application, or possibly as a feature of the window manager. If a future version of PikoPixel adds an API, screencasting functionality will likely be removed from the app and turned into an optional plugin.


3. OS X glue (PPOSXGlue_KeyUpDuringControlTracking.m)

This Mac-specific functionality is a workaround for a system issue introduced in OS X 10.10 (see the comments in the file for details). The patches are only installed on 10.10 & later - if the bug is fixed at some point, I'll also add an upper-limit version check. This is the only method-swizzling code that's intended to remain in the codebase.


Cheers,

Josh



On Sep 20, 2015, at 11:35 AM, H. Nikolaus Schaller wrote:

Hi,

Am 20.09.2015 um 16:54 schrieb Fred Kiefer <fredkiefer@gmx.de>:

Hi Nikolaus,

Am 20.09.2015 um 10:41 schrieb H. Nikolaus Schaller:
Am 20.09.2015 um 00:06 schrieb Josh Freeman
<gnustep_lists@twilightedge.com>:
On Sep 19, 2015, at 7:20 AM, Fred Kiefer wrote:
I am at the moment trying to get it running on gcc with the gcc
objc runtime. I know this isn't the supported constellation, but
I would like to at least understand what features are missing in
this setup.

PikoPixel's implementation of method-swizzling doesn't work with
the gcc runtime because the runtime's function,
class_replaceMethod(), behaves differently than on the objc2 or
Apple runtimes. That’s the only issue I know of (though I didn't
test much further on gcc after finding it), so in theory, a
reimplementation of method-swizzling (in
NSObject_PPUtilities_MethodSwizzling.m) that removed/reworked the
class_replaceMethod() calls could allow PP to use the gcc runtime.

I have tried to understand a little why PikoPixel uses method
swizzling at all. I know that it is a *very* powerful design
pattern, but makes code quite difficult to understand what happens.
And significant portability issues as we see them. But I never felt
that I need the method/class swizzling pattern (and using IMP) in
any Cocoa or GNUstep client code I did write - there are always
simple and efficient solutions without (e.g. delegation, proxy,
simple if-statements, performSelector).

At least in toggleScreencastingEnabled: I get the impression that
PikoPixel wants to either call some default sendEvent: or an
extended one (screencastOverrideIMP_SendEvent:) depending on some
toggle button.

But in screencastOverrideIMP_SendEvent it again checks if
gPPScreencastingIsEnabled which must always be true because it has
not been swizzled it in otherwise.

At least here you could just override sendEvent:, do the test and
call [super sendEvent:] at the end.

You even comment this as:

// sendEvent: only needs to be overridden during screencasting, so
rather than defining // the method in the class @implementation,
which makes a permanent override, instead // add/switch the method's
implementation on-the-fly using class_replaceMethod()

What do you want to achieve by a dynamic override? Save 20 ns in
event handling? You loose it by doing the swizzling each time the
user applies the toggle button. Why not just do the permanent
override?

I think you are right about this specific use, but then again this isn't
actually swizzling. You seem to fail to get the general idea behind
swizzling. You use swizzling when you don't want to have a subclass with specific behaviour but need the objects of the original class to behave
slightly different.

I get this, but I question why it is necessary to avoid the subclass design
pattern - except in bug fixing or really low level stuff.


I have not looked into other places where it is used and why, but
[object performSelector:sel] can do almost the same as “[object
swizzle:method with:sel]; [object method]”. performSelector: and
performSelector:withObject: are much more portable.

I don't understand this remark. In what cases could performSelector:
replace swizzling?

In the example I have studied, the toggle method makes the sendEvent
method do either this or that by swizzling. This can also be achieved
by doing perfomSelector:sel1 or performSelector:sel2. And therefore
it is possible to define a variable which stores the real selector. Then
method swizzling will be replaced by a dynamically chosen selector
to choose two different behaviours.

I.e.

- methodNotSwizzled
{
        [self performSelector:swizzleSelector];
}

- fakeSwizzle:(SEL) sel
{
        swizzleSelector=sel;
}

- behaviour1
{
        …
}

- behaviour2
{
        …
}

So you can dynamically change the behaviour of methodNotSwizzled
from the outside. Basically you do not replace the IMP pointer of methodNotSwizzled
but the selector used by performSelector to achieve this result.

But usually it is even be simpler to implement without any swizzling

- methodNotSwizzled
{
        if(otherBehaviour)
                [self behaviour2];
        else
                [self behaviour1];
}

This is why I question the use of swizzling at all in standard application code. IMHO there is no need to use it since the alternatives are easier to understand
and more portable.


By using a slightly higher level API I think the code will become
much shorter, easier to understand and more portable without needing
any other efforts outside of PikoPixel.

So my suggestion would be to modify PikoPixel to avoid swizzling at
all, instead of adding another lowest level compatibility layer to
GNUstep.

Swizzling should be avoided where it isn't needed, for example by fixing he bugs in GNUstep instead of working around them but in other cases it
is quite a useful tool.

It can be useful for special cases, but IMHO should be avoided in any application code.

And if the gcc runtime has an issue with it, we
should try to get that fixed.

Of course, compatibility of the gcc runtime be extended. And if a bug occurs in areas
where swizzling is really needed, it must be fixed of course.

BR,
Nikolaus


_______________________________________________
Discuss-gnustep mailing list
Discuss-gnustep@gnu.org
https://lists.gnu.org/mailman/listinfo/discuss-gnustep





reply via email to

[Prev in Thread] Current Thread [Next in Thread]