768757 - [HiDPI] icons in the bookmark manager are oversized and clipped when they contain multiple sized images

Status: RESOLVED INVALID

(Camino Graveyard :: Bookmarks, defect)

Description

[philippe (part-time)]

When a HiDPI is used the @2x images are displayed at twice the size of their allocated space. This results in clipped, oversized images. The same icons display correctly in other locations, such as bookmarks bar, location bar (when appropriate). This could also affect the 'control' images used at the bottom of the bookmark manager, I haven't tested yet.

Here are two icons that can be used for testing (WIP):
http://dev.l-c-n.com/camino/_d/_loc_bm-test.zip

Some icons display correctly, though: e.g bm_horizontal_separator and bookmark_group_badge

Bug 766015 comment 27 has a screenshot: attachement 636977.

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

As far as I can tell, we're doing nothing different to history_icon than the others there: http://mxr.mozilla.org/camino/source/camino/src/bookmarks/BookmarkManager.mm#432

But, try changing

[[self historyFolder]       setIcon:[NSImage imageNamed:@"history_icon"]];

to

[[self historyFolder]       setIcon:[[NSImage imageNamed:@"history_icon"] setSize:NSMakeSize(16, 16)]];

and see if that makes the right thing happen (I think I have the brackets right there so that will compile…).

Comment 2

[philippe (part-time)]

(In reply to Smokey Ardisson (not following bugs - do not email) from comment #1)

Unfortunately, that failed to compile :-(

/Users/phiw/Documents/lizard/build/camino/src/bookmarks/BookmarkManager.mm: In function 'void -[BookmarkManager setupSmartCollections](BookmarkManager*, objc_selector*)':
/Users/phiw/Documents/lizard/build/camino/src/bookmarks/BookmarkManager.mm:432: error: void value not ignored as it ought to be


In related news: 

1. the images in the controlbar at the bottom of the BM manager do _not_ have the same problem. It only affects the icons (except the folder icon) in the sidebar, and the Camino provided icons (smallbookmarks.tiff) in the main pane.

2. Any reason why we are using our own bonjour icon, instead of the system provided one ?
(NSImageNameBonjour according to the Apple docs)

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Test patch: specify a size for history_icon

(In reply to philippe from comment #2)
> Unfortunately, that failed to compile :-(

Try this patch.

Comment 4

[philippe (part-time)]

(In reply to Smokey Ardisson (not following bugs - do not email) from comment #3)
> Created attachment 637341 [details] [diff] [review]
> Test patch: specify a size for history_icon
> 
> (In reply to philippe from comment #2)
> > Unfortunately, that failed to compile :-(
> 
> Try this patch.

With a minor correction [1], that builds, but unfortunately it doesn't fix the issue, the icon is still oversized and clipped.

[1] one '[' too much 
NSImage* historyImage = [NSImage imageNamed:@"history_icon"];

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

As philippe noted, that doesn't work (even once the I provided the actual version I used, which did compile, unlike the version I attached) :P

Unfortunately, I don't know very much about the Bookmark Manager views, other than that the Collections view is our circa-10.2 ExtendedTableView, which in turn uses the circa-2001 version of Apple's ImageAndTextCell sample code.

Even if I add an [mImage setSize:NSMakeSize(16, 16)]; to setImage: in ImageAndTextCell (as the later versions of that sample code do), it still doesn't fix the problem.  I don't particularly understand the math sections of the rest of the code (which also has changed drastically in newer versions, including switching to Obj-C 2), so I'm not sure where the problem is.

Clearly the whole current mess of code works correctly with the OS-provided folder icon, so I'm really confused.

Comment 6

[philippe (part-time)]

Note also that the badge icon for the bookmarksgroup folder also displays correctly.

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

So, pretty sure this is ImageAndTextCell's fault.  I built the DragNDropOutlineView example (IATC ca. 2006, file modification date 2008) and modified a couple of the images to have 32px members (also, at one point, 128px, since I was exporting straight from Xcode's .icns; that result was really funky!)

There's a second copy of ImageAndTextCell in EnhancedDataBurn (IATC ca. 2007, file modification date 2007); I copied that into DragNDropOutlineView, cleaned, and rebuilt, and the result is the same.

I then used history_icon.tiff from the zip in comment 0, and I see the same result as in Camino (I wonder why the wrong result differs depending on icon?).

philippe, can you build the copy of DragNDropOutlineView that comes with Xcode on 10.6 and/or 10.7 and replace one of the Image*.tiff with one that has both 16px and 32px members, and see if the newer ImageAndTextCell sample code produces correct results?

It's still possible there's something related to OutlineView/TableView subclasses that were subclassed pre-HiDPI, but I'm thinking IATC is the most likely culprit right now…

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #7)
> I then used the WIP history_icon.tiff from the zip in comment 0, and I see the same
> result as in Camino (I wonder why the wrong result differs depending on
> icon?).

Interestingly, if I extract the 32px member from history.tiff and 

 tiffutil -cat history_32.tiff Camino.app/history_icon.tiff -out new.tiff

(32 first, then 16, whereas philippe's image puts 16 first and then 32), I get a third different result: the full 32px image, scaled up (this is similar to the result when I inadvertently left a 128px member in the "header file" image I extracted from Xcode).

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

When I did the test with DragNDropOutlineView for bug 769122, once again the OS-provided image (Bonjour in this case) is being drawn correctly in HiDPI simulation (needs a setSize: of 16 to work right in standard DPI when the image is returned), then HiDPI works correctly automatically, using the full 32px image drawn in the right place at the right size).

Which leads me back to wondering why the patch in comment 3 didn't work.

The pattern that works seems to be:
1) Get an image
2) [image setSize:NSMakeSize(16,16)];
3) return image; to some other method, or setIcon:image;

Except it only works when the image in Step 1 comes from the OS, because when I substitute the original "fetch-image-from-bundle" code back in for the "fetch-image-from-OS" code, the same old problems return.

Comment 10

[philippe (part-time)]

(In reply to Smokey Ardisson (not following bugs - do not email) from comment #7)

> philippe, can you build the copy of DragNDropOutlineView that comes with
> Xcode on 10.6 and/or 10.7 and replace one of the Image*.tiff with one that
> has both 16px and 32px members, and see if the newer ImageAndTextCell sample
> code produces correct results?

I would try, but I can't find any copy of DragNDropOutlineView to build anywhere in the Xcode folders (both 3.x.x / SL and 4.3.3 / Lion)…

(... from comment #8)
 
> 
>  tiffutil -cat history_32.tiff Camino.app/history_icon.tiff -out new.tiff
> 
> (32 first, then 16, whereas philippe's image puts 16 first and then 32), I
> get a third different result: the full 32px image, scaled up (this is
> similar to the result when I inadvertently left a 128px member in the
> "header file" image I extracted from Xcode).

16x16px image first then @2x image is the order recommended by Apple. It gives otherwise funky results (in Camino): in both 'normal' mode and HiDPI mode, the 32x32px is used, and it escapes the boundaries of its containing box (I used a custom made icon to make sure which image is used in both modes).

Comment 11

[philippe (part-time)]

I think I understand now.

Using the version of tiffutil that ships with Xcode 3.x on Snow Leopard returns 2 images, both report having the same resolution (72x72) in Preview.

tiffutil -cat image.tiff [e-mail] -out image-final.tiff
or 
tiffutil -cathidpicheck image.tiff [e-mail] -out image-final.tiff

The images appear at the wrong size in HiDPI mode and on Retina screens.

Doing the same with tiffutil on Lion and Xcode 4.3.x, then previewing the images in Preview reports one image at 72x72 and one at 144x144 (even though both input files have the same 72x72 resolution).

On Lion we have tiffutil v254.1. I wasn't able to find the version number for Snow Leopard

Here is what tiffutil warns about on Snow Leopard:

$ tiffutil -cathidpicheck addressbook_icon.tif [e-mail] -out addressbook_icon.tiff
Warning: Sizes of concatenated images do not follow Aqua guidelines for resolution independent multi-image TIFFs.
         Please provide two images, same point size, one at 72 dpi, and other at 288 dpi.
 Image 1 in file addressbook_icon.tif: 16x16 points (16x16 pixels, 72x72 dpi)
 Image 1 in file [e-mail]: 32x32 points (32x32 pixels, 72x72 dpi)
2 images written to addressbook_icon.tiff.

I haven't tried yet to follow the advice to create the @2x image at 288dpi

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

We can close this INVA now, right? ("Can't use 10.6-or-below tiffutil with two 72dpi source images")

tiffutil -cathidpicheck on 10.5 (also version number-less) gives the same error text; I suspect prior to 10.7 used a different format for its resolution-independent images.

Comment 13

[philippe (part-time)]

(In reply to Smokey Ardisson (not following bugs - do not email) from comment #12)
> We can close this INVA now, right? ("Can't use 10.6-or-below tiffutil with
> two 72dpi source images")

Yeah; only tiffutil as shipping with Xcode 4.3+ - 10.7+ returns the expected images. Current version is tiffutil v268.1.