Patch for 64-bit compatibility
Ryan Govostes
<http://groups.google.com/group/growldiscuss/web/rgov-20081101-
compatibility.diff>
which includes two relatively minor changes relating to 64-bit
compatibility. First, I updated LoginItemsAE.c to reflect changes to
data types. Second, I fixed code in GrowlNanoWindowController.m to
find the height of the menubar. The API it was using, NSMenuView, is
deprecated and not available to 64-bit applications.
Comments welcome.
Nick Zitzmann
On Nov 2, 2008, at 1:51 PM, Ryan Govostes wrote:
> which includes two relatively minor changes relating to 64-bit
> compatibility. First, I updated LoginItemsAE.c to reflect changes to
> data types. Second, I fixed code in GrowlNanoWindowController.m to
> find the height of the menubar. The API it was using, NSMenuView, is
> deprecated and not available to 64-bit applications.
>
> Comments welcome.
There is a lot more than that that needs to be done to make Growl 64-
bit clean. Last summer I submitted a patch that made Growl fully 64-
bit clean, including the application and plugins, but it got swept
under the rug. I assume that was due to lots of other things going on
at the time.
So I updated my repository to the latest in Mercurial as of right now,
and updated my patch. It does the following thing:
1. Discontinues support for Xcode 2.x, which is necessary for 64-bit
building.
2. Ports **everything** to all four CPU architectures. Ints have been
replaced with NSIntegers, floats have been replaced with CGFloats,
etc. I added some macros so that everything will still build with
older SDKs.
3. Since the MailMe plugin depends on the Messaging framework, and
that framework is not available for X86-64 or PPC64, the MailMe plugin
now uses the Scripting Bridge to send a message with Mail on those
architectures.
4. Resolves a problem where the project wouldn't build if Xcode was
set to place built products in a custom location. (The Growl plugin
was looking for the Automator action in a static path.)
Nick Zitzmann
<http://seiryu.home.comcast.net/>
Chris Forsythe
Do either of these patches, either from Ryan or Nick, cause problems
with 10.4 support?
Chris
Nick Zitzmann
On Nov 2, 2008, at 8:46 PM, Chris Forsythe wrote:
> Do either of these patches, either from Ryan or Nick, cause problems
> with 10.4 support?
No. Mine continues to use the older SDKs for 32-bit, and uses macros
to define NSInteger and CGFloat for the older SDKs. I didn't include
any Leopard-only code or anything...
Nick Zitzmann
<http://seiryu.home.comcast.net/>
Rudy Richter
verison.
Ned Holbrook
simplifications. Please do also let me know if this is not the
intended channel for doing so. Cheers,
Ned
Christopher Forsythe
Ned Holbrook
a whole, so I understand if any of these suggestions are mooted due to
"house style".
I have to say that casting sizeof() values strikes me as being overly
fussy: it doesn't seem to be helpful in that the value will be
truncated anyway and the casts add visual noise. For example, the
'(UInt32)sizeof(struct ProcessInfoRec)' in /Common/Source/
GrowlPathUtilities.m I would just leave as 'sizeof(struct
ProcessInfoRec)'.
For integer constants of explicit size such as those in Common/Source/
sha2.c, I would suggest using C99 specifiers such as UINT32_C, defined
in <stdint.c>.
For integer constants lacking explicit size such as the
'numPathComponents > 2U' in Core/Source/GrowlPreferencePane.m, just
write 2, since the U suffix is wrong half the time and generally
superfluous.
For CGFloat constants, there are a couple of alternate approaches:
doubles everywhere and explicit casts. Taking the former approach for
'1.0f', leave the 'f' suffix off and let the compiler generate the
appropriate type as necessary. Rarely does one encounter a situation
where a single-precision constant is required, and since the C
language prefers double-precision syntactically, go with the flow. :)
Using the same 1.0 with the latter approach yields '(CGFloat)1.0'.
While a bit more verbose, it has the advantage that the constant will
always be of the correct precision for both 32- and 64-bit builds.
For CGFloat in general, I would recommend not using CGFLOAT_IS_DOUBLE
in lieu of C99's <tgmath.h>. For example, instead of the dual calls to
ceil() and ceilf() in Core/Source/ACImageAndTextCell.m, you can just
#include <tgmath.h> and call ceil(); the header will automatically
insert the proper function call at compilation time depending on the
type of its argument. The C++ programmer in me cringes a bit, but it's
in the C99 language proper. I would also define a compatibility
version of CGFLOAT_MAX in Common/Source/GrowlDefines.h to be used
instead of FLT_MAX in these cases.
And finally, shouldn't there be an assignment to lpb.launchAppRef in
Common/Source/GrowlPathUtilities.m when __LP64__ is defined? This one
seems to be an actual bug as opposed to pedantry. :)
I'm eager to see Growl become available to 64-bit clients and whether
these suggestions are helpful or not, thanks for your consideration.
Cheers,
Ned
Ned Holbrook
> <stdint.c>
Whoops! That should be <stdint.h>, of course.
Ned
Peter Hosey
> Great! I will admit that I'm not too familiar with the source base as
> a whole, so I understand if any of these suggestions are mooted due to
> "house style".
>
> I have to say that casting sizeof() values strikes me as being
> overly fussy: it doesn't seem to be helpful in that the value will
> be truncated anyway and the casts add visual noise. For example, the
> '(UInt32)sizeof(struct ProcessInfoRec)' in /Common/Source/
> GrowlPathUtilities.m I would just leave as 'sizeof(struct
> ProcessInfoRec)'.
That's what it is, in what I have:
struct ProcessInfoRec info = { .processInfoLength = sizeof(struct
ProcessInfoRec) };
What version of the source code are you looking at?
> For integer constants of explicit size such as those in Common/
> Source/sha2.c, I would suggest using C99 specifiers such as
> UINT32_C, defined in <stdint.c>.
>
> For integer constants lacking explicit size such as the
> 'numPathComponents > 2U' in Core/Source/GrowlPreferencePane.m, just
> write 2, since the U suffix is wrong half the time and generally
> superfluous.
I'm not sure that UINT32_C(2) is any clearer than (UInt32)2, and other
than hard-coding the size of the type at four bytes, I'm not seeing
the advantage over 2U.
Speaking of which, I've written 2U specifically in cases where the
relevant variable or return type is an unsigned int. Nowadays, I would
often writen 2UL for comparison to NSUInteger, which is an unsigned
long*.
> For CGFloat constants, there are a couple of alternate approaches:
> doubles everywhere and explicit casts. Taking the former approach for
> '1.0f', leave the 'f' suffix off and let the compiler generate the
> appropriate type as necessary. …
The reason we use float constants is because those used to be single-
precision floats. For future code, especially after we change things
over to Leopard-only, we'll use doubles and double literals.
> While a bit more verbose, it has the advantage that the constant
> will always be of the correct precision for both 32- and 64-bit
> builds.
Assigning a double to a float will generate an implicit cast anyway,
so I don't think the explicit cast is necessary.
> For CGFloat in general, I would recommend not using
> CGFLOAT_IS_DOUBLE in lieu of C99's <tgmath.h>.
I hadn't heard of tgmath.h before. That looks handy.
On the one hand, it's premature optimization; on the other hand, it's
basically free—all we have to change is every line that includes
math.h. Also, I think the functions that the macros expand to are
(currently) compiler functions.
> I would also define a compatibility version of CGFLOAT_MAX in Common/
> Source/GrowlDefines.h to be used instead of FLT_MAX in these cases.
Absolutely.
Nick, what do you think about these changes?
> And finally, shouldn't there be an assignment to lpb.launchAppRef in
> Common/Source/GrowlPathUtilities.m when __LP64__ is defined? This
> one seems to be an actual bug as opposed to pedantry. :)
I'm looking at that file (the same one you mentioned above) and I
don't see anything named lpb. The same question pertains here: What
version are you looking at?
* On 32-bit OS X systems, NSUInteger is unsigned int, in order to
leave the method signature unchanged from previous OSs, which defined
those methods as returning unsigned int by that name. On 64-bit OS X
systems, it is unsigned long. More importantly, on 32-bit OS X
systems, unsigned int is the same size as unsigned long, so treating
them as unsigned long is harmless in either case.
Ned Holbrook
> On Dec 30, 2008, at 15:03:06, Ned Holbrook wrote:
> What version of the source code are you looking at?
> I'm not sure that UINT32_C(2) is any clearer than (UInt32)2, and other
> than hard-coding the size of the type at four bytes, I'm not seeing
> the advantage over 2U.
Typically it doesn't matter what the exact type is for integral
constants, especially when used as a literal (loop control, etc.), but
in cases where, say, an enum or constant data is being initialized and
the size is crucial I would use one or the other. The advantage of
using the C99 specifier is that you don't need to know how the sized
type is represented under the hood (long vs. int, say).
> The reason we use float constants is because those used to be single-
> precision floats. For future code, especially after we change things
> over to Leopard-only, we'll use doubles and double literals.
guess this was more of a proactive complaint. :)
> Assigning a double to a float will generate an implicit cast anyway,
> so I don't think the explicit cast is necessary.
literals inline, something like '9.0f / 5.0f * foo + 32.0f', in which
case the 'f' suffix is problematic in that the compiler would be
forced to convert a CGFloat foo to single precision.
> On the one hand, it's premature optimization; on the other hand, it's
> basically free—all we have to change is every line that includes
> math.h. Also, I think the functions that the macros expand to are
> (currently) compiler functions.
> > And finally, shouldn't there be an assignment to lpb.launchAppRef in
> > Common/Source/GrowlPathUtilities.m when __LP64__ is defined? This
> > one seems to be an actual bug as opposed to pedantry. :)
>
> I'm looking at that file (the same one you mentioned above) and I
> don't see anything named lpb. The same question pertains here: What
> version are you looking at?
Common/Source/LoginItemsAE.c in the aforementioned Growl64v3.patch. I
should also clarify that all of my comments are to the patch in
particular, it wasn't my intent to complain about existing ways of
doing things, merely trying to minimize the number of changes needed
for 64-bit compatibility in particular.
Thanks!
Ned
Peter Hosey
> On Dec 30, 6:33 pm, Peter Hosey <p...@growl.info> wrote:
>> On Dec 30, 2008, at 15:03:06, Ned Holbrook wrote:
>> What version of the source code are you looking at?
>
> I was looking at Growl64v3.patch from earlier in this thread.
Ah, I see.
I agree: That should remain cast-less.
>> I'm not sure that UINT32_C(2) is any clearer than (UInt32)2, and
>> other
>> than hard-coding the size of the type at four bytes, I'm not seeing
>> the advantage over 2U.
>
> … The advantage of using the C99 specifier is that you don't need to
> know how the sized type is represented under the hood (long vs. int,
> say).
Truth. The cast definitely does not have this advantage (for example,
(UInt64)9999999999 is a cast *up* from a type too small to hold the
value expressed by the literal).
>> Assigning a double to a float will generate an implicit cast
>> anyway, so I don't think the explicit cast is necessary.
>
> That's true, but I was thinking of cases where there are multiple
> literals inline, something like '9.0f / 5.0f * foo + 32.0f', in
> which case the 'f' suffix is problematic in that the compiler would
> be forced to convert a CGFloat foo to single precision.
It will do no such thing. The compiler will always cast up, never
down, unless you explicitly request otherwise (in this case, with
(float)foo). That means that it will convert the three float literals
to doubles.
Since it can do this at compile-time, that conversion is essentially
free. Its only cost is in human reading, since we're reading float
literals in what will be a double expression.
Either way, we should change drawing-related code to use double
literals, simply because double (as in CGFloat under 64-bit) is now
the right type for them.
>>> And finally, shouldn't there be an assignment to lpb.launchAppRef in
>>> Common/Source/GrowlPathUtilities.m when __LP64__ is defined? This
>>> one seems to be an actual bug as opposed to pedantry. :)
>>
>> I'm looking at that file (the same one you mentioned above) and I
>> don't see anything named lpb. The same question pertains here: What
>> version are you looking at?
>
> Another paste-o slipped into my message, sorry! I was referring to
> Common/Source/LoginItemsAE.c in the aforementioned Growl64v3.patch.
Ah.
Yup, you're right. Leaving that uninitialized could be really bad (a
crash).
That said, we should burninate that code once we drop Leopard, anyway.
Instead, we'll want to use the LSSharedFileList API.
Nick Zitzmann
On Dec 30, 2008, at 4:03 PM, Ned Holbrook wrote:
> I have to say that casting sizeof() values strikes me as being overly
> fussy: it doesn't seem to be helpful in that the value will be
> truncated anyway and the casts add visual noise. For example, the
> '(UInt32)sizeof(struct ProcessInfoRec)' in /Common/Source/
> GrowlPathUtilities.m I would just leave as 'sizeof(struct
> ProcessInfoRec)'.
It's not overly fussy. I did that for a reason; I turned on the
warning that shows implicit 64-bit to 32-bit conversions, which is SOP
when doing a proper 64-bit conversion. That raises warnings whenever a
program passes the results of sizeof() into a function that takes a
UInt32 argument, because sizeof() returns a size_t, which is an
unsigned long. Adding a cast silences the warnings.
> For CGFloat constants, there are a couple of alternate approaches:
> doubles everywhere and explicit casts. Taking the former approach for
> '1.0f', leave the 'f' suffix off and let the compiler generate the
> appropriate type as necessary. Rarely does one encounter a situation
> where a single-precision constant is required, and since the C
> language prefers double-precision syntactically, go with the flow. :)
> Using the same 1.0 with the latter approach yields '(CGFloat)1.0'.
> While a bit more verbose, it has the advantage that the constant will
> always be of the correct precision for both 32- and 64-bit builds.
Using doubles everywhere causes the implicit conversion warnings in 32-
bit builds, and using single-precision constants causes precision to
be lost in 64-bit builds. I think my approach gave the best of both
worlds without having to cast constants.
Nick Zitzmann
<http://seiryu.home.comcast.net/>
Nick Zitzmann
On Dec 30, 2008, at 7:33 PM, Peter Hosey wrote:
>> And finally, shouldn't there be an assignment to lpb.launchAppRef in
>> Common/Source/GrowlPathUtilities.m when __LP64__ is defined? This
>> one seems to be an actual bug as opposed to pedantry. :)
>
> I'm looking at that file (the same one you mentioned above) and I
> don't see anything named lpb. The same question pertains here: What
> version are you looking at?
If this is about the code in LoginItemsAE.c, then yes, that does need
to have a launchAppRef in the 64-bit version, or there could be
trouble. Of course, that structure really should be wiped with bzero()
first... Shall I make this change? Was my patch ever checked in?
Nick Zitzmann
<http://seiryu.home.comcast.net/>
Ned Holbrook
loaded. In my experience the shorten warning in gcc 4.0 can be wonky,
particularly for this reason. My personal opinion is that I shouldn't
have to spoon-feed the compiler by littering casts throughout
otherwise well-written code and I have typically resorted to other
techniques for finding these issues. Indeed, the warnings spit out by -
Wall or even -Wmost should suffice for finding the critical issues. As
ever, it's possible that my approach is by no means the best, that's
just been my experience in porting system-level code to 64-bit over
the past couple years.
As for the (CGFloat) casting of FP literals, I still think it would
offer the best of both worlds, but to each his own. :)
Speaking as J. Random Dude blundering into the discussion, I did want
to say thank you very much for your help in preparing the patch; as I
mentioned earlier, I'm very pleased to see Growl making this
transition.
Ned
Peter Hosey
> Of course, that structure really should be wiped with bzero()
> first... Shall I make this change?
No need; looking now, it already does that with memset.
> Was my patch ever checked in?
Nope. Last I heard, Rudy is going to review it. (My current Growl
project is writing the Growl Developer Handbook, which is why I'm not
reviewing it.)
Ned Holbrook
would also ensure that the rest of the struct is zeroed. An example of
this technique is forkPB in Common/Source/CFGrowlAdditions.c, but even
there the initialization of .forkIterator is superfluous. Cheers,
Ned
Peter Hosey
> Another option would be to use the C99 member initializers, which
> would also ensure that the rest of the struct is zeroed.
That's what I thought, but when I looked in C99, it seemed to imply
the opposite. Citation?
I don't want to rely on a perk of GCC.
Ned Holbrook
Reference Manual_, 5th ed. Note especially the example involving
'struct S s3'. This behavior would seem to follow logically from the
description of structure initializers with fewer initializers than
components in section 4.6.6, whereby any remaining components are
initialized to their default values.
Ned
Peter Hosey
> I was basing this statement on my reading of section 4.6.9 of _C: A
> Reference Manual_, 5th ed.
I've never heard of that and don't have a copy.
(Executive summary of the following: You're right and I found backing
for it in C99. Skip to the end for the important details.)
This page has a link to a draft of a revision to C99:
http://www.open-std.org/JTC1/SC22/WG14/www/projects#9899
That document contains the complete standard (more or less,
considering it's a draft of a revision).
We start out in section 6.7.8 (“Initialization”), paragraphs 9 and
10:
> 9 Except where explicitly stated otherwise, for the purposes of this
> subclause unnamed members of objects of structure and union type do
> not participate in initialization. Unnamed members of structure
> objects have indeterminate value even after initialization.
>
> 10 If an object that has automatic storage duration is not
> initialized explicitly, its value is indeterminate. If an object
> that has static storage duration is not initialized explicitly, then:
>
> — if it has pointer type, it is initialized to a null pointer;
> — if it has arithmetic type, it is initialized to (positive or
> unsigned) zero;
> — if it is an aggregate, every member is initialized (recursively)
> according to these rules;
> — if it is a union, the first named member is initialized
> (recursively) according to these rules.
WRT the storage duration of the object, section 6.2.4:
> 6.2.4 Storage durations of objects
> 1 An object has a storage duration that determines its lifetime.
> There are three storage durations: static, automatic, and allocated.
> Allocated storage is described in 7.20.3.
>
> 2 The lifetime of an object is the portion of program execution
> during which storage is guaranteed to be reserved for it. An object
> exists, has a constant address, and retains its last-stored value
> throughout its lifetime. If an object is referred to outside of its
> lifetime, the behavior is undefined. The value of a pointer becomes
> indeterminate when the object it points to reaches the end of its
> lifetime.
>
> 3 An object whose identifier is declared with external or internal
> linkage, or with the storage-class specifier static has static
> storage duration. Its lifetime is the entire execution of the
> program and its stored value is initialized only once, prior to
> program startup.
>
> 4 An object whose identifier is declared with no linkage and without
> the storage-class specifier static has automatic storage duration.
7.20.3 is the section describing malloc, realloc, and free.
So our launch parameter block has automatic storage duration.
Returning to 6.7.8, here's the paragraph that backs up my previous
knowledge, and your statement, that omitted members must initialize to
NULL/zero:
> 21 If there are fewer initializers in a brace-enclosed list than
> there are elements or members of an aggregate, or fewer characters
> in a string literal used to initialize an array of known size than
> there are elements in the array, the remainder of the aggregate
> shall be initialized implicitly the same as objects that have static
> storage duration.
That said, since we have a memset call in place and LoginItemsAE is on
the chopping block, I'm inclined to not worry about it there.
Rudy Richter
Growl proper ;)
Christopher Forsythe
Chris
Peter Hosey
> <Growl64v3.patch>
My review, with the more significant changes first and style quibbles
last:
> @@ -524,7 +524,7 @@
> struct FSForkIOParam forkPB = {
> .ref = fileRef,
> .forkIterator = {
> - .initialize = 0L
> + .initialize = 0
> },
> .outForkName = &forkName,
> };
Why?
> --- a/Common/Source/GrowlDefines.h Fri Sep 19 00:23:24 2008 -0700
> +++ b/Common/Source/GrowlDefines.h Sun Nov 02 19:39:32 2008 -0700
None of your changes to this file belong in it. This file is part of
the framework, which means that applications include it, too. We will
not redefine NSInteger, NSUInteger, and CGFloat in applications'
source code. Please move these changes to another header (perhaps
GrowlDefinesInternal.h).
Also, why int and unsigned int, not long and unsigned long?
> +#ifndef __LP64__
> lpb.launchAppSpec = &appSpec;
> +#endif
That would seem to not make it not work on LP64, since this is the
part that tells it what application to launch.
Better to set launchAppRef when __LP64__ is defined. You can use the
#ifndef up above, to block out the FSGetCatalogInfo call.
> - int numberOfRects;
> + NSInteger numberOfRects;
> -- (NSRectArray)copyRectsInSet:(NSSet *)rectSet count:(int
> *)outCount padding:(float)padding excludingDisplayController:
> (GrowlDisplayWindowController *)displayController
> +- (NSRectArray)copyRectsInSet:(NSSet *)rectSet count:(NSInteger
> *)outCount padding:(CGFloat)padding excludingDisplayController:
> (GrowlDisplayWindowController *)displayController
> - int count = [rectSet count];
> + NSInteger count = [rectSet count];
I cannot think of a good reason for these to be signed. They should be
NSUInteger. (That also goes for the for loop that counts from 0 to
numberOfRects.)
> -- (int) UDPPort;
> -- (void) setUDPPort:(int)value;
> +- (CFIndex) UDPPort;
> +- (void) setUDPPort:(CFIndex)value;
Actually, if that really is a port number, it should be a short int.
(Same goes for the .m version of these prototypes.)
> -- (int)windowNum;
> +- (NSInteger)windowNum;
> -- (int)windowNum
> +- (NSInteger)windowNum
> {
> return _windowNum;
> }
NSWindow already has a method for this—windowNumber. In the absence
of any comment indicating a reason for a separate method, I say we
burninate this alternate wheel.
Also, we should change all references to _windowNum into messages to
[self windowNumber].
Actually, I think I'll try this myself. If it works, I'll commit it
separately, and v4 of your patch will hardly need to do anything
relating to the window number.
> +#ifdef __LP64__
> + yPosition -= [[NSApp mainMenu] menuBarHeight];
> +#else
> yPosition-=[NSMenuView menuBarHeight];
> +#endif
Did you check that -[NSMenu menuBarHeight] works now? A couple years
ago, it didn't:
http://boredzo.org/blog/archives/2006-03-17/report-an-apple-bug-friday-11
The style quibbles follow.
> +++ b/Common/Source/NSMutableAttributedStringAdditions.m Sun
> Nov 02 19:39:32 2008 -0700
> @@ -7,6 +7,7 @@
> //
>
> #import "NSMutableAttributedStringAdditions.h"
> +#import "GrowlDefines.h" // for NSInteger
I think you meant NSUInteger, since that's the type you use in this
file.
> --- a/Common/Source/NSWindow+Transforms.m Fri Sep 19 00:23:24 2008
> -0700
> +++ b/Common/Source/NSWindow+Transforms.m Sun Nov 02 19:39:32 2008
> -0700
> - [self rotate:radians about:NSMakePoint(_frame.size.width * 0.5,
> _frame.size.height * 0.5)];
> + [self rotate:radians about:NSMakePoint(_frame.size.width * 0.5f,
> _frame.size.height * 0.5f)];
> - [self scaleX:x Y:y about:NSMakePoint(_frame.size.width * 0.5,
> _frame.size.height * 0.5) concat:YES];
> + [self scaleX:x Y:y about:NSMakePoint(_frame.size.width * 0.5f,
> _frame.size.height * 0.5f) concat:YES];
> - [self scaleX:x Y:y about:NSMakePoint(_frame.size.width * 0.5,
> _frame.size.height * 0.5) concat:NO];
> + [self scaleX:x Y:y about:NSMakePoint(_frame.size.width * 0.5f,
> _frame.size.height * 0.5f) concat:NO];
> - NSPoint p = [self
> windowToScreenCoordinates:NSMakePoint(0.0, _frame.size.height)];
> + NSPoint p = [self
> windowToScreenCoordinates:NSMakePoint(0.0f, _frame.size.height)];
> - original = CGAffineTransformScale(original, 1.0 / x, 1.0 / y);
> + original = CGAffineTransformScale(original, 1.0f / x, 1.0f / y);
> - NSPoint point = [self windowToScreenCoordinates:NSMakePoint(0.0,
> _frame.size.height)];
> + NSPoint point = [self windowToScreenCoordinates:NSMakePoint(0.0f,
> _frame.size.height)];
> --- a/Core/Source/ACImageAndTextCell.m Fri Sep 19 00:23:24 2008 -0700
> +++ b/Core/Source/ACImageAndTextCell.m Sun Nov 02 19:39:32 2008 -0700
> --- a/Core/Source/GrowlImageAdditions.m Fri Sep 19 00:23:24 2008 -0700
> +++ b/Core/Source/GrowlImageAdditions.m Sun Nov 02 19:39:32 2008 -0700
(except one)
> - NSPoint point = NSMakePoint(_frame.size.width / 2.0,
> _frame.size.height / 2.0);
> + NSPoint point = NSMakePoint(_frame.size.width / 2.0f,
> _frame.size.height / 2.0f);
> - original.a = 1.0 ; original.b = 0.0 ;
> - original.c = 0.0 ; original.d = 1.0 ;
> + original.a = 1.0f ; original.b = 0.0f ;
> + original.c = 0.0f ; original.d = 1.0f ;
> - [result setAlphaValue:1.0];
> + [result setAlphaValue:1.0f];
> - [rippleFilter setValue:[NSNumber numberWithFloat:40.0]
> forKey:@"inputCornerRadius"];
> - [rippleFilter setValue:[CIVector
> vectorWithX:rippleRect.origin.x-40.0 Y:(rippleRect.origin.y - 40.0)]
> forKey:@"inputPoint0"];
> - [rippleFilter setValue:[CIVector vectorWithX:
> (rippleRect.origin.x + rippleRect.size.width + 40.0) Y:
> (rippleRect.origin.y + rippleRect.size.height + 40.0)]
> forKey:@"inputPoint1
> "];
> + [rippleFilter setValue:[NSNumber numberWithFloat:40.0f]
> forKey:@"inputCornerRadius"];
> + [rippleFilter setValue:[CIVector
> vectorWithX:rippleRect.origin.x-40.0f Y:(rippleRect.origin.y -
> 40.0f)] forKey:@"inputPoint0"];
> + [rippleFilter setValue:[CIVector vectorWithX:
> (rippleRect.origin.x + rippleRect.size.width + 40.0f) Y:
> (rippleRect.origin.y + rippleRect.size.height + 40.0f)]
> forKey:@"inputPoint1"];
> - [rippleFilter setValue:[NSNumber numberWithFloat:0.0]
> forKey:@"inputPhase"];
> + [rippleFilter setValue:[NSNumber numberWithFloat:0.0f]
> forKey:@"inputPhase"];
> --- a/Plugins/Displays/Animation & Transitions/Window Transitions/
> GrowlFadingWindowTransition.m Fri Sep 19 00:23:24 2008 -0700
> +++ b/Plugins/Displays/Animation & Transitions/Window Transitions/
> GrowlFadingWindowTransition.m Sun Nov 02 19:39:32 2008 -0700
> --- a/Plugins/Displays/Animation & Transitions/Window Transitions/
> GrowlFlippingWindowTransition.m Fri Sep 19 00:23:24 2008 -0700
> +++ b/Plugins/Displays/Animation & Transitions/Window Transitions/
> GrowlFlippingWindowTransition.m Sun Nov 02 19:39:32 2008 -0700
> @implementation GrowlScaleWindowTransition
> - (id) initWithDuration:(NSTimeInterval)duration animationCurve:
> (NSAnimationCurve)curve {
> if((self = [self initWithDuration:duration
> animationCurve:curve])) {
> - [self setFrameRate:(1.0 / duration)];
> + [self setFrameRate:(1.0f / (float)duration)];
> - register float a_coeff = 1.0f - a;
> + register CGFloat a_coeff = 1.0f - a;
> - [panel setAlphaValue:0.0];
> + [panel setAlphaValue:0.0f];
> - float sizeReduction = GrowlSmokePadding +
> iconSize + (GrowlSmokeIconTextPadding * 0.5f);
> + CGFloat sizeReduction = GrowlSmokePadding +
> iconSize + (GrowlSmokeIconTextPadding * 0.5f);
These seem wrong, since CGFloat is double. They should stay or become
double literals.
> - float bestSecondaryOrigin = 0;
> + CGFloat bestSecondaryOrigin = 0;
> - [rippleFilter setValue:[NSNumber numberWithFloat:160*(now -
> startTime)] forKey:@"inputPhase"];
> + [rippleFilter setValue:[NSNumber numberWithDouble:160*(now
> - startTime)] forKey:@"inputPhase"];
> - float maxWidth = 0;
> + CGFloat maxWidth = 0;
Should be 0.0 (etc.), while you're at it.
> -- (void) setInteger:(int)value forKey:(NSString *)key {
> +- (void) setInteger:(CFIndex)value forKey:(NSString *)key {
> +#ifdef __LP64__
> + NSNumber *object = [[NSNumber alloc] initWithInteger:value];
> +#else
> NSNumber *object = [[NSNumber alloc] initWithInt:value];
> +#endif
> - (NSNumber*) idleThreshold {
> +#ifdef __LP64__
> + return [NSNumber numberWithInteger:[self
> integerForKey:GrowlStickyIdleThresholdKey
> ]];
> +#else
> return [NSNumber numberWithInt:[self
> integerForKey:GrowlStickyIdleThresholdKey]];
> +#endif
Any reason we can't just pass the CFIndex value to initWithInteger
unconditionally?
> + MailApplication *mail = [SBApplication
> applicationWithBundleIdentifier:@"com.apple.Mail"];
I'd be OK with “Mail” instead of “mail” as the name of this
variable.
“[Mail classForScriptingClass:@"outgoing message"]”… yeah, that
looks good.
> bool doFadeIn;
You missed one to change to BOOL.
Peter Hosey
>> -- (int)windowNum;
>> +- (NSInteger)windowNum;
>
>> -- (int)windowNum
>> +- (NSInteger)windowNum
>> {
>> return _windowNum;
>> }
>
> NSWindow already has a method for this—windowNumber. In the absence
> of any comment indicating a reason for a separate method, I say we
> burninate this alternate wheel.
>
> Also, we should change all references to _windowNum into messages to
> [self windowNumber].
>
> Actually, I think I'll try this myself. If it works, I'll commit it
> separately, and v4 of your patch will hardly need to do anything
> relating to the window number.
Um, yeah. So, apparently, we only use AWRippler in
GrowlRippingWindowTransition, we don't use
GrowlRipplingWindowTransition, and if I hack Smoke to use it, I find
that it doesn't work.
Any objections to the flaming death of a couple of classes?
Nick Zitzmann
On Feb 6, 2009, at 10:21 PM, Peter Hosey wrote:
>> @@ -524,7 +524,7 @@
>> struct FSForkIOParam forkPB = {
>> .ref = fileRef,
>> .forkIterator = {
>> - .initialize = 0L
>> + .initialize = 0
>> },
>> .outForkName = &forkName,
>> };
>
> Why?
Because CatPositionRec.initialize is an SInt32. Passing in 0L will
cause a compiler warning, since 0L is technically a 64-bit value when
building for 64-bit.
> None of your changes to this file belong in it. This file is part of
> the framework, which means that applications include it, too. We will
> not redefine NSInteger, NSUInteger, and CGFloat in applications'
> source code. Please move these changes to another header (perhaps
> GrowlDefinesInternal.h).
OK, that's a fair request.
> Also, why int and unsigned int, not long and unsigned long?
Because the latter will cause problems with C++ code. But if you don't
feel like that would be a problem, then I'm fine with setting them to
long.
> That would seem to not make it not work on LP64, since this is the
> part that tells it what application to launch.
>
> Better to set launchAppRef when __LP64__ is defined. You can use the
> #ifndef up above, to block out the FSGetCatalogInfo call.
Yeah, that will be a problem.
> I cannot think of a good reason for these to be signed. They should be
> NSUInteger. (That also goes for the for loop that counts from 0 to
> numberOfRects.)
You're right; those should be changed. I was just translating the
original code, and assumed there was a reason for using signed ints
there (because sometimes there is a good reason, e.g. iterating in
reverse)...
> Actually, if that really is a port number, it should be a short int.
> (Same goes for the .m version of these prototypes.)
Then shouldn't that really be an unsigned short int?
> Did you check that -[NSMenu menuBarHeight] works now? A couple years
> ago, it didn't:
Good catch. I just checked, and it is still broken. Unfortunately
since there's no NSMenuView class in the 64-bit framework, I had to
make this a static number to get it to work.
I'll implement the rest of what you suggested and submit it again. I
also realized that I forgot to mark GrowlHelperApp's symbols as
exported, which broke most of the plugins, and MailMe wasn't being
linked to the ScriptingBridge framework.
Nick Zitzmann
<http://seiryu.home.comcast.net/>
Nick Zitzmann
On Feb 6, 2009, at 10:21 PM, Peter Hosey wrote:
> Any reason we can't just pass the CFIndex value to initWithInteger
> unconditionally?
Actually, yes, unless you don't care about blowing away Panther and
Tiger backward compatibility. +numberWithInteger: was added in
Leopard. Since the 64-bit builds will require Leopard, it makes sense
to limit the use of +numberWithInteger: to 64-bit builds only right now.
Nick Zitzmann
<http://seiryu.home.comcast.net/>
Peter Hosey
> On Feb 6, 2009, at 10:21 PM, Peter Hosey wrote:
>>> @@ -524,7 +524,7 @@
>>> struct FSForkIOParam forkPB = {
>>> .ref = fileRef,
>>> .forkIterator = {
>>> - .initialize = 0L
>>> + .initialize = 0
>>> },
>>> .outForkName = &forkName,
>>> };
>>
>> Why?
>
>
> Because CatPositionRec.initialize is an SInt32.
Ah. Silly me for believing the documentation and not checking the
header…
>> Also, why int and unsigned int, not long and unsigned long?
>
> Because the latter will cause problems with C++ code. But if you
> don't feel like that would be a problem, then I'm fine with setting
> them to long.
Yeah, since these macros should only be in Growl code and not
application code, affecting C++ code is not a problem.
>> Actually, if that really is a port number, it should be a short int.
>> (Same goes for the .m version of these prototypes.)
>
> Then shouldn't that really be an unsigned short int?
Looking at netinet/in.h just now, it looks like it should be UInt16:
#ifndef _IN_PORT_T
#define _IN_PORT_T
typedef __uint16_t in_port_t;
#endif
Peter Hosey
I just checked it myself, and it isn't on 10.5.6. According to the
test app on the blog post, [[NSApp mainMenu] menuBarHeight] returns
22, which is the correct value.
When I apply v4 of your patch, I'll try it on both 10.5.6 and 10.4.10.
I'll try it both as you supplied it and with -[NSMenu menuBarHeight],
and see what happens in the latter case.
Nick Zitzmann
On Feb 7, 2009, at 11:56 PM, Peter Hosey wrote:
> I just checked it myself, and it isn't on 10.5.6. According to the
> test app on the blog post, [[NSApp mainMenu] menuBarHeight] returns
> 22, which is the correct value.
Which CPU architecture did you use? When I tried it on x86_64, it was
returning 0.
Nick Zitzmann
<http://seiryu.home.comcast.net/>
Peter Hosey
> On Feb 7, 2009, at 11:56 PM, Peter Hosey wrote:
>> I just checked it myself, and it isn't on 10.5.6. According to the
>> test app on the blog post, [[NSApp mainMenu] menuBarHeight] returns
>> 22, which is the correct value.
>
> Which CPU architecture did you use? When I tried it on x86_64, it
> was returning 0.
I used the build that I provided in the archive, so i386.
If I build it in x86_64 only and run it, it crashes while checking in
with Launch Services.
Nick Zitzmann
On Feb 8, 2009, at 2:05 AM, Peter Hosey wrote:
> I used the build that I provided in the archive, so i386.
>
> If I build it in x86_64 only and run it, it crashes while checking in
> with Launch Services.
OK, I just tried your sample on x86_64, and it worked for me, though I
had to comment out the call to NSMenuView. And it did return 22.
I think I know why [[NSApp mainMenu] menuBarHeight] is returning zero
in GrowlHelperApp. GrowlHelperApp doesn't load any MainMenu nib for
some reason, which means -mainMenu is probably returning nil, but I
couldn't detect this, since normally I'd type "po [NSApp mainMenu]" in
GDB to detect this, but Leopard's GDB is really awful at debugging 64-
bit applications, and keeps telling me there is no such selector even
though there is...
So we have a choice here with the 64-bit version of the Nano plugin:
1. Use a constant for the menu bar height, and update this if it ever
changes in the future. This is what my patch currently does, though I
know now that the comment I made is incorrect.
2. Give Growl a main menu nib, and switch to using NSApplicationMain()
to start up the app in HelperMain.m.
(2) would make me feel better, although it is a pretty big change.
Nick Zitzmann
<http://seiryu.home.comcast.net/>
Peter Hosey
> So we have a choice here with the 64-bit version of the Nano plugin:
3. We could simply create an empty menu and set it as the main menu in
code. Saves loading a nib for UI elements the user will never see.
Nick Zitzmann
On Feb 8, 2009, at 10:14 PM, Peter Hosey wrote:
> 3. We could simply create an empty menu and set it as the main menu in
> code. Saves loading a nib for UI elements the user will never see.
D'oh! You're right - I just tried this, and it worked as expected.
Here's a new patch with the original call to [[NSApp mainMenu]
menuBarHeight] placed back into service, and a faux main menu set up
early in the execution.
Nick Zitzmann
<http://seiryu.home.comcast.net/>
Peter Hosey
<64bitv5.patch>
--- a/Common/Source/NSMutableAttributedStringAdditions.m Fri Sep 19 00:23:24 2008 -0700+++ b/Common/Source/NSMutableAttributedStringAdditions.m Mon Feb 09 00:12:50 2009 -0700
+#import "GrowlDefines.h" // for NSInteger
+#ifdef __LP64__+ return (unsigned)[self intValue];+#elsereturn [self unsignedLongValue];+#endif
- err = AECreateDesc(typeLongInteger, &itemIndexPlusOne, sizeof(itemIndexPlusOne), &indexDesc);
+ err = AECreateDesc(typeSInt32, &itemIndexPlusOne, sizeof(itemIndexPlusOne), &indexDesc);
diff -r a476173fbe3f Common/Source/sha2.c--- a/Common/Source/sha2.c Fri Sep 19 00:23:24 2008 -0700+++ b/Common/Source/sha2.c Mon Feb 09 00:12:50 2009 -0700
- + ceilf((cellFrame.size.height - retRect.size.height) * 0.5f);+#if CGFLOAT_IS_DOUBLE == 1+ + ceil((cellFrame.size.height - retRect.size.height) * 0.5);+#else+ + ceilf((cellFrame.size.height - retRect.size.height) * 0.5);+#endif
+ NSInteger defaultStyleRow = NSNotFound;
+ [NSApp setMainMenu:[[[NSMenu alloc] init] autorelease]];
-// const float colors[2] = {0.15f, 0.35f};
- const float colors[2] = {27.0f / 255.0f * 1.5f, 58.0f / 255.0f};+// const float colors[2] = {0.15, 0.35};+ const CGFloat colors[2] = {27.0 / 255.0 * 1.5, 58.0 / 255.0};
- (NSArray *) inactiveTransitions {- int count = [windowTransitions count];+ NSInteger count = [windowTransitions count];NSMutableArray *result = [NSMutableArray arrayWithCapacity:count];NSArray *transitionArray = [windowTransitions allValues];- int i;+ NSInteger i;for (i=0; i<count; ++i) {
Nick Zitzmann
On Feb 17, 2009, at 3:43 PM, Peter Hosey wrote:
>> +#import "GrowlDefines.h" // for NSInteger
>
> That still should be NSUInteger. ☺
And even that would still be wrong, since it was removed from there by
your request. :) That line is gone now.
> Why not cast unsignedLongValue to unsigned int? Then you're going
> from greater positive range to lesser positive range, instead of the
> other way around.
But then we'd be downscan converting a 64-bit number into a 32-bit
number, and I don't know what will actually happen if that has to
happen to the number (is that behavior even defined?). Though I agree
it's not likely to happen, it's better to be safe...
> Don't forget to change itemIndexPlusOne to an SInt32.
D'oh! Thanks.
> More code for the chopping block. I believe we can switch to
> CommonCrypto, which comes with Tiger and later. (Not in this patch,
> though.)
Or switch to OpenSSL's libcrypto, if all you need to do is generate
SHA hashes. Version 0.9.7 has shipped with every version of the OS
since Panther. Or is there a reason why OpenSSL is not being used...?
> Did this actually raise a warning? Seems like the multiplication by
> 0.5 should make the expression a double anyway. (Perhaps you didn't
> mean to delete the f from that version?)
Well, if we're going to do double precision floating point math in 64-
bit builds, then let's do double precision floating point math in 64-
bit builds, and single-precision floating point math in 32-bit builds
for backward compatibility reasons. If it would make you feel more
comfortable, in the past, I've used macros to define the proper math
function for the type of CGFloat, like this:
#if CGFLOAT_IS_DOUBLE == 1
#define MyCeil(x) ceil(x)
#else
#define MyCeil(x) ceilf(x)
#endif
That eliminates the extra lines of code.
> Should be NSUInteger. (Everything relating to array indexes in this
> method should have been unsigned to begin with—now is a good
> opportunity to fix that.)
Absolutely. Done there, and in another place in GrowlSpeechPrefs.
Those were the only two I could find, not counting code that has been
commented out.
> You should allocate NSMenu objects in the NSMenu zone: [NSMenu
> allocWithZone:[NSMenu menuZone]]
Done.
> I think they should both be CGFloat.
Well, one of the two is commented out, but OK...
> Should be NSUInteger.
Absolutely, and so should the count variable. :)
Here is the latest patch.
Nick Zitzmann
<http://seiryu.home.comcast.net/>
Peter Hosey
> On Feb 17, 2009, at 3:43 PM, Peter Hosey wrote:
>> Why not cast unsignedLongValue to unsigned int? Then you're going
>> from greater positive range to lesser positive range, instead of
>> the other way around.
>
> But then we'd be downscan converting a 64-bit number into a 32-bit
> number, and I don't know what will actually happen if that has to
> happen to the number (is that behavior even defined?).
It'll lop off the high bits.
More importantly, this is unsignedIntValue. It returns a 32-bit value,
which means the cast has to happen at some point anyway. Better to
cast down and lop off high bits (or to clamp) than to cast up and
falsely claim that we can return the full range of unsigned int.
(IIRC, the main reason we have it is to work around a Cocoa exception.
I don't remember what it was, though. Might be fun to take out that
method and see if we can fix the problem properly—but not right now.)
>> More code for the chopping block. I believe we can switch to
>> CommonCrypto, which comes with Tiger and later. (Not in this patch,
>> though.)
>
> Or switch to OpenSSL's libcrypto, if all you need to do is generate
> SHA hashes.
OpenSSL only supports SHA-1, not SHA-2. We support both.
> … in the past, I've used macros to define the proper math function
> for the type of CGFloat, like this:
>
> #if CGFLOAT_IS_DOUBLE == 1
> #define MyCeil(x) ceil(x)
> #else
> #define MyCeil(x) ceilf(x)
> #endif
>
> That eliminates the extra lines of code.
Earlier in this thread, Ned Holbrook suggested using tgmath.h instead
of math.h. tgmath.h defines macros like this for us, and under the
names of the double-precision functions (so the ceil macro expands to
either the ceil function or the ceilf function).
That's part of C99, so we should be able to use it, assuming that its
strange location (somewhere in /usr/lib) doesn't cause GCC to not know
where it is.
>> I think they should both be CGFloat.
>
> Well, one of the two is commented out, but OK...
You changed the literals on the other end of the assignment, so I
figured it would be good to change the type name at the front, too. ☺
> <64bitv5.patch>
Shouldn't that be v6? ;)
Nick Zitzmann
On Feb 18, 2009, at 2:34 PM, Peter Hosey wrote:
> It'll lop off the high bits.
OK, I'll take your word for it.
> Earlier in this thread, Ned Holbrook suggested using tgmath.h instead
> of math.h. tgmath.h defines macros like this for us, and under the
> names of the double-precision functions (so the ceil macro expands to
> either the ceil function or the ceilf function).
I already tried tgmath.h, and it won't work very well, because it
raises compiler warnings about implicit conversions. So I looked at
the preprocessed code, and it looks like tgmath.h expands floor() to
include a call to floorf(), but with the double arguments still in
place, which would cause the compiler to complain. So if no one minds
the compiler warnings, we can do this, although I think my solution is
actually cleaner, and doesn't raise any warnings.
> Shouldn't that be v6? ;)
Oh no, I'm starting to lose count... :S
Nick Zitzmann
<http://seiryu.home.comcast.net/>
Peter Hosey
> On Feb 18, 2009, at 2:34 PM, Peter Hosey wrote:
>> It'll lop off the high bits.
>
> OK, I'll take your word for it.
Let's ask C99. Quoth § 6.3.1.3:
> 6.3.1.3 Signed and unsigned integers
> 1 When a value with integer type is converted to another integer
> type other than _Bool, if
> the value can be represented by the new type, it is unchanged.
> 2 Otherwise, if the new type is unsigned, the value is converted by
> repeatedly adding or
> subtracting one more than the maximum value that can be represented
> in the new type
> until the value is in the range of the new type.
So let's try an example. Here's 2 ** 60 + 2 ** 28:
0x1000000010000000
This is a number within range for a 64-bit type, but outside the range
of a 32-bit type, but with a lower half that is within range for a 32-
bit type.
The spec says we repeatedly subtract UINT_MAX+1. UINT_MAX is 2**32 -
1, so we repeatedly subtract 2 ** 32.
(Yeah, that takes a couple minutes. Yay 64-bit math! Another way would
be to just do the modulus operator, but I'm strictly following the
standard here.)
Behold:
; x = 2 ** 60 + 2 ** 28
; x
1152921504875282432
; x % 2 ** 32
268435456
; subtracted_from = x
; while (subtracted_from > (2**32 - 1)) { subtracted_from -= 2**32; }
; subtracted_from
268435456
; 2 ** 28
268435456
; x & 0xffffFFFF
268435456
So, yup: It'll (effectively) lop off everything above the low 32 bits.
>> Earlier in this thread, Ned Holbrook suggested using tgmath.h
>> instead of math.h. tgmath.h defines macros like this for us, and
>> under the names of the double-precision functions (so the ceil
>> macro expands to either the ceil function or the ceilf function).
>
> I already tried tgmath.h, and it won't work very well, because it
> raises compiler warnings about implicit conversions. … So if no one
> minds the compiler warnings, we can do this, although I think my
> solution is actually cleaner, and doesn't raise any warnings.
Yeah, let's go with your macros. Fewer warnings = better code.
Nick Zitzmann
one more. I had forgotten about the Growl-WithInstaller framework, so
I made one little change to that to get it to build - since the Carbon
HIView API is not available for 64-bit, I had to #ifdef all of that
stuff out.
All this means is Cocoa developers that want to use the Growl-
WithInstaller framework _must_ use the ObjC application bridge or the
installer will not appear. I don't think that's an unreasonable thing
to ask of Cocoa developers...
BTW, the "nb" version of GrowlInstallation.strings file is in the
wrong format; the file in the repository is in UTF-8 format, but it
has to be in UTF-16 format, or Xcode won't build the framework. I
can't make that change; someone with checkin privileges will have to
do it.
Nick Zitzmann
<http://seiryu.home.comcast.net/>
Peter Hosey
> OK, and in the latest version of the patch, I made those changes and
> one more. I had forgotten about the Growl-WithInstaller framework,
> so I made one little change to that to get it to build - since the
> Carbon HIView API is not available for 64-bit, I had to #ifdef all
> of that stuff out.
>
> All this means is Cocoa developers that want to use the Growl-
> WithInstaller framework _must_ use the ObjC application bridge or
> the installer will not appear. I don't think that's an unreasonable
> thing to ask of Cocoa developers...
Cocoa developers are already using the Cocoa API. It's the Carbon
developers we need to tell that, but they're not using 64-bit anyway,
so they won't notice.
> BTW, the "nb" version of GrowlInstallation.strings file is in the
> wrong format; the file in the repository is in UTF-8 format, but it
> has to be in UTF-16 format, or Xcode won't build the framework.
Xcode 3.1 and later will convert encodings of strings files for
applications. Does it not do that for frameworks?
> I can't make that change; someone with checkin privileges will have
> to do it.
Mercurial is a DVCS. Everyone has commit privileges. ☺
When you clone our repository, you're not just getting a window into
our repository like you do with a CVCS—you're getting *your own
repository*, and you can commit to it any time you want.
For a patch like this that's undergoing a lot of revisions, it's good
to make use of the mq extension. I've never used it, but this is
exactly the sort of thing it's for. It'll let you iterate on a single
commit, rather than make a whole lot of commits with each round of
improvements.
http://www.selenic.com/mercurial/wiki/index.cgi/MqTutorial
Once you're done (whether you use mq or just the rollback command),
you would export the patch and send that to us. Then it has your own
name in the byline and a commit message of your choosing.
> <64bitv999.patch>
Hah!
I'll start reading it later today.
Nick Zitzmann
On Feb 19, 2009, at 6:47 AM, Peter Hosey wrote:
> Cocoa developers are already using the Cocoa API. It's the Carbon
> developers we need to tell that, but they're not using 64-bit anyway,
> so they won't notice.
I know, but it might be worth warning people anyway, since you never
know when someone decided to do things differently for the sake of
doing things differently...
>> BTW, the "nb" version of GrowlInstallation.strings file is in the
>> wrong format; the file in the repository is in UTF-8 format, but it
>> has to be in UTF-16 format, or Xcode won't build the framework.
>
> Xcode 3.1 and later will convert encodings of strings files for
> applications. Does it not do that for frameworks?
Apparently not. If you try checking out the source, and building Growl-
WithInstaller, it won't work, because whatever is copying over the
strings files expects the strings to already be in UTF-16 format.
The German language strings are also in UTF-8 format, but they're
saved in Apple's plist format, so maybe it expects UTF-8 files to be
in plist format.
> Mercurial is a DVCS. Everyone has commit privileges. ☺
OK, let me rephrase that then: Someone with the ability to push their
checkin changes to the master repository. :)
Nick Zitzmann
<http://seiryu.home.comcast.net/>
Evan Schoenberg
changeset which touches a ton because I did a whole bunch of cleanup
throughout Growl in the course of implementation. This is about to
suck, I'm afraid.
-Evan
> --~--~---------~--~----~------------~-------~--~----~
> You received this message because you are subscribed to the Google
> Groups "Growl Development" group.
> To post to this group, send email to growl-
> devel...@googlegroups.com
> To unsubscribe from this group, send email to growl-developm...@googlegroups.com
> For more options, visit this group at http://groups.google.com/group/growl-development?hl=en
> -~----------~----~----~----~------~----~------~--~---
>
Peter Hosey
> This is a huge changeset. My growl-tcp branch is also a huge
> changeset which touches a ton because I did a whole bunch of cleanup
> throughout Growl in the course of implementation. This is about to
> suck, I'm afraid.
Maybe not. I tried it just now, and patch -p1 only failed four hunks.
It looks to me like Mercurial + FileMerge will be able to apply it
with minimal pain.
Peter Hosey
> set to place built products in a custom location. (The Growl plugin
> was looking for the Automator action in a static path.)
I fixed this problem just now:
http://growl.info/hg/growl-development/rev/2d299900249b
This means that you can now build in the Development configuration
without having to build at least GrowlAction in Deployment first.
Peter Hosey
Reminder for anyone who's forgotten: This is actually version 7 of the
patch. ☺
> + * @defined GrowlCGCeil()
> + * @defined GrowlCGFabs()
> + * @defined GrowlCGFloor()
Does that actually work? AFAIK, they need to be separate comments, one
per macro.
Also, I'm not married to the old C function names. We can be more
verbose:
GrowlCGFloatCeiling
GrowlCGFloatFloor
GrowlCGFloatAbsoluteValue
> +#if CGFLOAT_IS_DOUBLE == 1
Style issue: You can cut out the “== 1”.
> + NSUInteger i = 0U, len = [type length];
Should be 0UL, no? It'll either be size-equivalent (LP32) or the same
type (LP64).
Same with existingLoginItemIndex a bit further down (should be -1L),
the return statement in GrowlPreferencesController_unsignedShortForKey
(should be 0U, not 0), and various numeric literals compared and
assigned to freespace and usedspace in sha2.c.
I'm not hung up on fixing these, esp. in the case of sha2.c (which
we'll replace with CommonCrypto at some point) and LoginItemsAE (see
next section). You may want to double-check them for warnings; as long
as they're not warnings, I'm OK with bare int literals.
> +#ifdef __LP64__
> +#pragma unused(appSpec)
> +#else
My first thought was:
Why not just make the declaration itself conditional?
But then I looked further. We don't use LoginItemsAE at all from the
framework (I didn't think we did, but I checked anyway), so as far as
we're concerned, this code requires 10.4. Therefore, we can cut out
“the compatible” way entirely.
And if anyone is wondering what we'll do if Apple updates this code:
First, I doubt they will, since they've introduced LSSharedFileList.
More importantly, we'll switch over to LSSharedFileList when we drop
Tiger, so applying updates to LoginItemsAE is extremely low on my list
of priorities.
> --- a/Core/Source/GrowlPreferencePane.h Fri Sep 19 00:23:24 2008 -0700
> +++ b/Core/Source/GrowlPreferencePane.h Wed Feb 18 21:57:54 2009 -0700
> @@ -8,6 +8,7 @@
> +#import "GrowlDefines.h" // for NSInteger
You should probably mention NSUInteger here, too, since there are some
uses of that in the .m file.
Speaking of these imports, you should change them all to
GrowlDefinesInternal.h, since you moved the declarations of these
types there.
> + else if (theIndex < 0)
> + return 0;
This should be return 0U;, although I wouldn't reject the patch over
it. ☺
> +typedef size_t CSSM_SIZE; // for pre-Leopard SDKs
Won't that break on the Leopard and later SDKs? You can't typedef
something more than once.
> --- a/Framework/Source/GrowlApplicationBridge-Carbon.c Fri Sep 19
> 00:23:24 2008 -0700
> +++ b/Framework/Source/GrowlApplicationBridge-Carbon.c Wed Feb 18
> 21:57:54 2009 -0700
It would probably be a good idea to log a message when we would have
run the installation or upgrade prompt but didn't because we were
running in 64-bit mode. Something like:
Warning: Growl [installation|upgrade] prompt suppressed because this
is a Carbon-based application running in 64-bit mode - please contact
the application developer and ask them to switch to the Cocoa-based
Growl API
It should be localized, of course, using the CF localization macros.
> if (!promptedToInstallGrowl) {
> +#ifdef __LP64__
> + promptedToInstallGrowl = false;
> +#else
It's already false there, so this can just be an #ifndef.
> - psn.highLongOfPSN = [[dict
> objectForKey:@"NSApplicationProcessSerialNumberHigh"] longValue];
> + psn.highLongOfPSN = [[dict
> objectForKey:@"NSApplicationProcessSerialNumberHigh"]
> unsignedIntValue];
> - psn.lowLongOfPSN = [[dict
> objectForKey:@"NSApplicationProcessSerialNumberLow"] longValue];
> + psn.lowLongOfPSN = [[dict
> objectForKey:@"NSApplicationProcessSerialNumberLow"]
> unsignedIntValue];
Tricksy Apple, retyping these longs as UInts32!
It would probably be a good idea to add a comment here noting that the
actual type of these members is not long.
> + "MACOSX_DEPLOYMENT_TARGET[arch=ppc]"
> = 10.3;
I see that these are in the project's configurations. That means that
it should be 10.4, with ppc=10.3 specified explicitly in the framework
targets, as Growl itself and all the extras require Tiger.
> - [rippleFilter setValue:[NSNumber numberWithFloat:40.0]
> forKey:@"inputCornerRadius"];
> + [rippleFilter setValue:[NSNumber numberWithFloat:40.0f]
> forKey:@"inputCornerRadius"];
> - [rippleFilter setValue:[NSNumber numberWithFloat:0.0]
> forKey:@"inputPhase"];
> + [rippleFilter setValue:[NSNumber numberWithFloat:0.0f]
> forKey:@"inputPhase"];
Considering CIVector takes CGFloats, I'd say that these should be
CGFloats as well, which means doubles. That would also make these
consistent with your use of double later on (which makes particular
sense there because that computation involves an NSTimeInterval).
> + // On 64-bit, the Message framework is not
> available. Instead, use the Scripting Bridge.
I'm surprised I hadn't thought of this before, but: What if the user
doesn't use Mail?
Many people use Entourage or Thunderbird, and a few use other clients
such as GyazMail. I suspect that most of these people have either
never set up Mail or not kept it current (changed ISP, changed
employer, changed password, etc.).
I'm thinking that it'd be better to simply add SMTP controls (server
and port) to MailMe and have a Python script to send the message. If
you don't know Python, I can write this part separately.
Even if we do talk to Mail with the Scripting Bridge:
> + [[Mail outgoingMessages] addObject:message];
> + [message.toRecipients addObject:recipient];
I think we should add the recipient before adding the message to
outgoingMessages. (If you tried this and it didn't work, this should
have a comment explaining the order.)
-
Another thing I was thinking is that I should move the code for Smoke,
Bubbles, etc. into a common class, and move the definitions of their
separate looks into plist files. This would not only simplify our
code, but greatly shorten your patch and eliminate its most tedious-to-
read part—the part that takes the longest for me to review. What do
you think?
Those last two make 64-bit definitely at least a 1.1.6 feature, if not
1.2.
Nick Zitzmann
On Apr 6, 2009, at 7:25 PM, Peter Hosey wrote:
> Should be 0UL, no? It'll either be size-equivalent (LP32) or the same
> type (LP64).
Technically, yes, but that is very pedantic :), and is not raising any
warnings. We're also not building the code with NS_BUILD_32_LIKE_64
defined yet, so NSIntegers are still ints on 32-bit.
> Won't that break on the Leopard and later SDKs? You can't typedef
> something more than once.
GCC's preprocessor would like to disagree with you. :) And it's highly
unlikely CSSM_SIZE will ever be anything other than a size_t, so I
don't think you need to worry about that.
> Warning: Growl [installation|upgrade] prompt suppressed because this
> is a Carbon-based application running in 64-bit mode - please contact
> the application developer and ask them to switch to the Cocoa-based
> Growl API
I think I asked about that before. I implemented something like this,
but I had to rephrase that, because most of the Carbon HI stuff
doesn't work in 64-bit apps, not the other way around. ;)
> I see that these are in the project's configurations. That means that
> it should be 10.4, with ppc=10.3 specified explicitly in the framework
> targets, as Growl itself and all the extras require Tiger.
Speaking of which, I noticed that GC is turned on for the framework
targets. If you still want to support the pre-Leopard cats, then this
has to change, since unfortunately GC-enabled code won't run prior to
Leopard. I'm going to change it so that, by default, only the 64-bit
frameworks support GC, since they require Leopard anyway. Of course,
third parties can always download the source and build things their
way if they wish.
> Considering CIVector takes CGFloats, I'd say that these should be
> CGFloats as well, which means doubles. That would also make these
> consistent with your use of double later on (which makes particular
> sense there because that computation involves an NSTimeInterval).
The documentation and all of the sample code I've seen strongly
suggest that CIFilters use floats internally for property values, even
on 64-bit. So I don't think this will matter.
> I'm surprised I hadn't thought of this before, but: What if the user
> doesn't use Mail?
Then the MailMe plugin as written will not work, because even when
it's running in 32-bit mode, the Messaging framework requires Mail to
at least be configured in order to function. On 64-bit, it needs to be
configured, and it needs to be run once in order to send the message,
which we must hope is not too much to ask. (If it _is_ too much to
ask, then it won't break my heart too much if you decide to make
GrowlHelperApp run as 32-bit by default. My main concerns are both
frameworks.)
In the future, the MailMe plugin should probably be rewritten so that
the message is sent using a lower level framework, such as EDMessage
or Pantomime, or something. This will probably require a rewrite,
since you'd need to ask the user for information about his/her SMTP
server in order to send the message.
> Another thing I was thinking is that I should move the code for Smoke,
> Bubbles, etc. into a common class, and move the definitions of their
> separate looks into plist files. This would not only simplify our
> code, but greatly shorten your patch and eliminate its most tedious-
> to-
> read part—the part that takes the longest for me to review. What do
> you think?
There's an apocryphal story that says that, when Mahatma Gandhi was
asked about Western civilization, he said that he thought it was a
good idea.
I think that's also a good idea for a future version, but right now, I
just wanted to get this conversion out the door so that third-parties
that were holding back on 64-bit due to the lack of frameworks can get
started now, and we can make changes later. The main reason why I was
interested in this was because I was trying a 64-bit conversion of an
app that was using Growl, but neither Growl nor any of the frameworks
were 64-bit clean back then, and I'm sure I'm not the only person who
has ever given up on a 64-bit app due to dependency issues.
Again, if you want to be conservative with the next point release,
then we can easily make it so that GrowlHelperApp runs as a 32-bit app
unless the user turns on the 64-bit version in the Get Info window. In
fact, that would probably be a very good idea, since older point
releases of Leopard had buggy (and sometimes crashy) 64-bit
frameworks. But without saying _too_ much, that shouldn't be set
forever.
Besides, this patch already cleans up a lot of stuff as it is. :) I
incorporated your remaining comments in this one.
Nick Zitzmann
<http://seiryu.home.comcast.net/>