Regarding 1.1.3
bgannin
release. I'm trying to make time this week (or over the weekend) to
investigate and hopefully resolve them, but if someone else is able to
do so in the meantime, excellent.
They are:
1. WebKit display styles' icons are broken in some fundamental way.
This is easily reproduced using built-in styles and hitting preview
(using a fresh build of ToT.) You get a blue ? graphic for a missing
resource, as reported by beta testers.
2. Changing the default display causes a 100% reproducible hang (at
least on my machine) in the KVO mechanisms of
GrowlPreferencesController_boolForKey.
Once these are fixed 1.1.3 is ready to ship. Thanks to all the testers
and developers who have put so much effort into solidifying this
substantive update!
- brian 'bgannin' ganninger
Clint Hime
1.1.3b3 using webkit styles, sometimes the icon from the app that is
growling is shrunk much smaller than the size it should fit. Such as
google notifier. In 1.1.2, which i'm currently using, the google
notifier icon fills the growl window, but in the b3 it is smaller. It
doesn't seem to shrink every image in a webkit growl window, but some
do.
Peter Hosey
> Something else about the first issue you are gonna fix, is that in
> the 1.1.3b3 using webkit styles, sometimes the icon from the app
> that is growling is shrunk much smaller than the size it should fit.
This isn't specific to WebKit-based styles. Currently, Mike Lee's
Twitter icon shows up tiny for me, and I use Smoke (a Cocoa display).
The reason is probably Growl over-respecting the DPI property of the
image. @bmf's icon is 230 dpi; that would do it.
Clint Hime
Christopher Forsythe
Peter Hosey
> 2. Changing the default display causes a 100% reproducible hang (at
> least on my machine) in the KVO mechanisms of
> GrowlPreferencesController_boolForKey.
Same here. I just went looking for it, and in so doing, found this:
2008-05-08 07:52:40.538 System Preferences[15474] *** -
[NSCFDictionary length]: selector not recognized [self = 0x14604b70]
Running System Preferences in the debugger revealed that the line
that raised the exception was line 796 of GrowlPreferencePane.m:
pluginToUse = [ticketsArrayController valueForKey:
[[displayPluginsArrayController content] objectAtIndex:
[(NSPopUpButton *)sender indexOfSelectedItem]]];
So I'm guessing that somebody changed that array to contain
dictionaries rather than strings, or is putting dictionaries into it
by accident at some place.
I don't know whether this is related to the hang, but it certainly
doesn't help.
Peter Hosey
> If it isn't we should probably pull it from 1.1.3.
Looking at the output of `strings Safari`, it has plenty of
notification names in it. There's a good chance that we may be able
to give it a similar rebirth to the one we gave GrowlMail.
While we're at it, I'd also like us to see if we can reimplement it
as a real Safari plug-in, rather than an input manager hack.
Christopher Forsythe
Chris
Peter Hosey
> pluginToUse = [ticketsArrayController valueForKey:
> [[displayPluginsArrayController content] objectAtIndex:
> [(NSPopUpButton *)sender indexOfSelectedItem]]];
>
> So I'm guessing that somebody changed that array to contain
> dictionaries rather than strings, or is putting dictionaries into
> it by accident at some place.
Moreover, why are we getting some property of an array controller,
and why are we passing anything from that pop-up menu as the key for
that property?
It seems like the correct behavior would be to simply assign the
dictionary itself to that variable (which is an NSDictionary *).
> I don't know whether this is related to the hang, but it certainly
> doesn't help.
My first change was to make the result of the objectAtIndex: message
the receiver of an objectForKey:GrowlPluginInfoKeyName message. This
fixed the immediate exception (the one I mentioned) *and the hang*,
but it causes a different exception.
I do believe that I'm on the right track, however.
Peter Hosey
> Moreover, why are we getting some property of an array controller,
> and why are we passing anything from that pop-up menu as the key
> for that property?
>
> It seems like the correct behavior would be to simply assign the
> dictionary itself to that variable (which is an NSDictionary *).
Committed as [4816] on the trunk:
- pluginToUse = [ticketsArrayController valueForKey:
[[displayPluginsArrayController content] objectAtIndex:
[(NSPopUpButton *)sender indexOfSelectedItem]]];
+ pluginToUse = [[displayPluginsArrayController content]
objectAtIndex:[(NSPopUpButton *)sender indexOfSelectedItem]];
This fixes the hang and all exceptions, *and* the formerly-non-
working automatic preview when the user changes the display.
I don't think I've ever fixed three bugs in one one-liner before. :D
Peter Hosey
> It used to be something else entirely as well, a simbl plugin (or
> whatever those are called) as well, so if simbl works on 10.5 maybe
> we could resurrect that for the short term, and then do this in the
> long term?
Not a big improvement, if any. SIMBL is simply the Smart Input
Manager Bundle Loader.
So we'd be (re-)off-shoring the input manager hackery to another
program, but it'd still be an input manager hack.
Christopher Forsythe
Chris
Evan Schoenberg
bgannin
Great work Peter, thanks!
- brian 'bgannin' ganninger
Peter Hosey
I've had this with Twitterrific (@bmf's icon, for example, shows off
the problem nicely), and went investigating earlier.
By writing every notification that Growl receives to a file, I
determined that this is a problem with the image coming from the
application: Twitterrific, for example, scales the image but doesn't
strip or adjust its DPI number. Thus, Growl receives an image that is
48 pixels by 48 pixels, and 288 pixels per inch. Result: @bmf pinky-
nail tattoo.
Growl *could* work around it by ignoring the DPI setting, but that's
bad for resolution independence—apps that work *correctly* would lose
out if we did that.
It's the apps' behavior that's wrong, not Growl's, so it's the app
developers (including IconFactory and those of us on the Adium
Project) that need to fix it.
Relevant Adium ticket:
Peter Hosey
> fit.
Furthermore, while investigating the other WebKit images bug (the
'?'), I noticed just now that this problem actually *doesn't* occur
with a WebKit style. If you're seeing this bug, either you're
actually not using a WebKit style or you're encountering it for some
other reason than the one I found.
Clint Hime
whenever I use Smoke, none of my images in notifications are small,
but when I use the neat-looking iPhonesque, it shrinks a few that show
up normal size in Smoke. So I know that in some instances the size is
showing up correctly.
Clint Hime
image, but in 1.1.3b3 when I click preview on all of the styles, the
only ones that show the growl image instead of the ? are Bezel,
Brushed, Bubbles, iCal, Music Video, Nano, and Smoke. I'm not sure if
any of these are Webkit styles, but I think they aren't.
Peter Hosey
> I can't say for sure if its that being a Webkit style brings the ?
I can. It is.
> … but in 1.1.3b3 when I click preview on all of the styles, the
> only ones that show the growl image instead of the ? are Bezel,
> Brushed, Bubbles, iCal, Music Video, Nano, and Smoke. I'm not sure
> if any of these are Webkit styles, but I think they aren't.
They aren't. They are all Cocoa-based displays.
Peter Hosey
> This is easily reproduced using built-in styles and hitting preview
> (using a fresh build of ToT.) You get a blue ? graphic for a
> missing resource, as reported by beta testers.
Fixed in [4820] and [4821].
Full explanation:
This is reproducible using an IconFamily (.icns) file as the
notification icon. For example, try this on [4819] with this image:
/System/Library/CoreServices/CoreTypes.bundle/Contents/Resources/
DeleteAliasIcon.icns
Here's what you'd get in the run log:
2008-05-09 06:30:20.903 GrowlHelperApp[2527:10b] Returning bitmap
image rep NSBitmapImageRep 0x1f4470 Size={32, 32}
ColorSpace=NSCalibratedWhiteColorSpace BPS=1 BPP=2 Pixels=32x32
Alpha=YES Planar=NO Format=2 CGImage=0x1f5730
libpng error: Invalid bit depth for grayscale+alpha image
2008-05-09 06:30:20.904 GrowlHelperApp[2527:10b]
CGImageDestinationFinalize failed for output type 'public.png'
This causes the line of HTML code that *should* contain PNG image
data to instead say:
<div id="icon"><img src="data:image/png;base64,(null)" alt="icon" /
></div>
As you can guess, “(null)” isn't a valid base-64 representation of a
PNG image.
Our WebKit display generates the PNG data by looking for the largest
bitmap image rep in the input image, and taking the PNG
representation of that.
The problem was, we weren't updating the maxWidth variable to which
we compared each rep's size. Thus, we compared each size to 0;
unsurprisingly, we find that every single one is bigger.
The result is that we simply used the last rep in the image.
Among the formats that an IconFamily image can contain are 1-bit
32×32 and 1-bit 16×16. And these are usually the last representations
in the file or resource.
Result: Our loop, intended to pick the largest rep, instead picks the
1-bit rep, freaking out libpng and leading to a '?' in the
notification window.
The immediate fix ([4820]) was simply to update maxWidth—the line
that should have been there from the beginning. This helps in 90% of
cases, but it's only a start, and it's still order-dependent. The
largest rep by width (or, more accurately, the first rep with the
greatest width) could be a 1-bit rep, in which case we'd be back
where we started.
I fixed it completely in [4821]. Now, we ignore 1-bit reps outright,
and when the only representation available is 1-bit, we give up on
PNG entirely and use TIFF. 1-bit images now correctly show up, even
in WebKit styles.
Here's a 1-bit-only version of the above-linked image, in case any
developers want to play with it:
Evan Schoenberg
On May 9, 2008, at 11:32 AM, Peter Hosey wrote:
> As you can guess, “(null)” isn't a valid base-64 representation of a
> PNG image.
or of anything else ;)
> The problem was, we weren't updating the maxWidth variable to which
> we compared each rep's size. Thus, we compared each size to 0;
> unsurprisingly, we find that every single one is bigger.
>
> The result is that we simply used the last rep in the image.
Oops! The same problem exists in Adium's implementation of -
[NSImage(AIImageAdditions) largestBitmapImageRep]. I applied your fix
in Adium's [23376]. Good catch :)
> I fixed it completely in [4821]. Now, we ignore 1-bit reps outright,
> and when the only representation available is 1-bit, we give up on
> PNG entirely and use TIFF. 1-bit images now correctly show up, even
> in WebKit styles.
¡Fantastico!
-Evan
Christopher Forsythe
1) If the image is correct, just display it.
2) If it is not correct, do the following:
2a) Make it so that this issue won't be apparent to the user within the notification.
2b) Log something to console about this issue, telling the user to contact the developer about this issue.
Is this something that can be done?
Chris
Peter Hosey
> Making the devs fix it is problematic since it means that for a
> decent amount of time we're going to be getting a bunch of bug
> reports about this.
I'm OK with that. We'd simply give them a stock answer:
This is a bug in Twitterrific/Adium/XYZ, not Growl.
To be specific, it's changing the size in pixels of the image, but
not removing or adjusting the property that says how many pixels per
inch it is; this results in the image having very small physical
dimensions.
We recommend and ask that you report this bug to the application's
developer.
> Can we detect this situation programatically?
Not with 100% accuracy. There's no way to tell whether the image has
an incorrect DPI or is simply really, really small. We would have to
pretend that there are no really, really small images, and simply
ignore the DPI of any image whose pixel size is less than some hard-
coded number.
I'm of the opinion that Growl's current behavior (always respecting
the DPI of the image) is correct, and that we shouldn't break it.
This is an app bug, so we should leave it to the app developers to fix.