Closed Bug 411332 Opened 14 years ago Closed 14 years ago

Fix OS/2 icon handler and add icon logic using RWS

Categories

(Core :: ImageLib, defect)

x86
OS/2
defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: mozilla, Assigned: mozilla)

References

Details

Attachments

(6 files, 4 obsolete files)

Bug 333253 changed the OS/2 version of nsIconChannel::MakeInputStream() to be a no-op for cairo builds. Let's repair that here and at the same time add the improved icon handling using RWS that was originally drafted in bug 305061.
One thing I have noticed... the icons weren't working on the local drive which I used to check it.  However, I find that with ftp sites it does give icons.  I wonder what would it take to add WPSWizard support so that the icon replacement comes through (icon replacement done via class replacement for WPS)?
The FTP icons are the normal dull ones, defined by the theme (GIFs). At least that's what I see. The ones for the local drive would be created/retrieved by RWS and only those would pass through the broken nsIconChannel.

I guess support for WPSWizard icons would need some deep understanding of how they are created and how a (non-WPS, RWS supported) program could ask for them. I don't really have a clue. They can be PNGs, right? So that part at least should work with the normal PNG decoder. But still, I have no idea how to pass this info over there...
Attached patch WIP (obsolete) — Splinter Review
This is the part of the RWS patch from bug 305061 that deals with the the icon handler with some more lines changed so that it displays _something_ (plus removal of the extra spaces after opening brackets). But as I am not good at all in making sense of the OS/2 BMP format I didn't even manage to get an all-blue icon display as such. I would be happy if somebody else could take this on. It's mainly about changing the four (similar) functions 
ConvertColorBitMap(), ShrinkColorBitMap(), ConvertMaskBitMap(), and ShrinkMaskBitMap() from BGR_A1 to 32bit ARGB.
Attached patch further WIP (obsolete) — Splinter Review
OK, it turns out despite what is said in nsIconChannel.h we still need to construct a BGRA image, so the code for the color channels is still OK. Just that the alpha channel needs to be 8bit wide instead of 1bit (or something like that).
So this patch at least gets the colors right but just marks every pixel as opaque, so many icons will appear to have ugly black regions...
Attachment #296832 - Attachment is obsolete: true
Rich, do you have a hint how I have to change ConvertMaskBitMap() and ShrinkMaskBitMap() so that I get full bytes of alpha information every fourth output byte? I thought I could just set every fourth byte to 255 or 0 at the same place in the code where it currently sets the 1 bit of output transparency. But somehow that doesn't work. I always get transparency that is shifted with regard to the color, as if the input data was too short or too long for the color data...

You don't need to compile and test just give me a few hints.
This contains replacement code that should get icon conversion working properly.  If it does, I'll leave it up to you to create a proper diff.

FYI...  because the BGRA32 format is simpler than BGR_A8, I was able to eliminate ShrinkColorBitMap() & ShrinkMaskBitMap().  The remaining functions now receive the 'fShrink' flag and handle converting an icon from large to small format.  The attachment identifies the lines in MakeInputStream() that need to be changed accordingly.
(In reply to comment #1)
> One thing I have noticed... the icons weren't working on the local drive which
> I used to check it.  However, I find that with ftp sites it does give icons.
> I wonder what would it take to add WPSWizard support so that the icon
> replacement comes through (icon replacement done via class replacement for
> WPS)?

Are you saying that WPSWizard is unable to deliver conventional icons?  If so, it would break a lot of code (e.g. WarpCenter, XCenter, etc.).  I'm unable to investigate because installing it put my WPS into a continuous crash/restart cycle at startup.
WPSWizard allows icon replacement, I am using PNG files for my icons.  The current code here was putting icons in ftp pages but those icons were not the replaced ones but rather the ones I would have if I turned off WPSWizard.  Xcenter and Larsen Commander show the WPSWizard icons so I am guessing it is doing what it should do.  These latest changes here may fix this automagically.
All we are doing here is about the OS/2 icon format. PNG icons would have to pass through the PNG decoder. But I am actually not sure where to start in the Mozilla sources (nor do I really care at the moment, as I don't have WPSWizard installed ;-)).
(In reply to comment #8)
> WPSWizard allows icon replacement, I am using PNG files for my icons.  The
> current code here was putting icons in ftp pages but those icons were not the
> replaced ones but rather the ones I would have if I turned off WPSWizard. 
strange, I see the wpswizard replaced icons in download-manager, on ftp-sites, local drives, tools-options-applications. When I uninstall the wpswizard icon theme the OS/2 icons are back. With the current patch I see even less problems with wpswizard png icons.
I found that the mail-application of SeaMonkey trunk shows also attachments with two icons and crashes like described for thunderbird
It gets fixed when I change
+
+// reduces overhead by preventing calls to nsRws when it isn't present
+#ifdef MOZ_THUNDERBIRD
+static PRBool sUseRws = PR_FALSE; // XXX off for now for Thunderbird, otherwise
+                                  // causes duplicate attachment icons and crashes

to
+
+// reduces overhead by preventing calls to nsRws when it isn't present
+#if defined(MOZ_THUNDERBIRD) || defined(MOZ_SUITE)
+static PRBool sUseRws = PR_FALSE; // XXX off for now for Thunderbird and MailNews, otherwise
+                                  // causes duplicate attachment icons and crashes

with this fix mailnews shows only one icon per attachment and doesn't crash, download manager and ftp/local drive icons still work.
However the small attachment icons in mailnews seem to miss the lowest one or two lines, when you click on such an icon the missing lines are displayed (w/o the currently black "surrounding" pixels).
On ftp sites I see still some file:linktofile when the file has no extension, e.g. README. Is there a possibility to show a default icon when no extension or association is available through the wps/rws?
(In reply to comment #6)
> Created an attachment (id=298266) [details]
> ConvertMaskBitMap() & ConvertColorBitMap() fix
> 
> This contains replacement code that should get icon conversion working
> properly.  If it does, I'll leave it up to you to create a proper diff.
> 
> FYI...  because the BGRA32 format is simpler than BGR_A8, I was able to
> eliminate ShrinkColorBitMap() & ShrinkMaskBitMap().  The remaining functions
> now receive the 'fShrink' flag and handle converting an icon from large to
> small format.  The attachment identifies the lines in MakeInputStream() that
> need to be changed accordingly.

Thanks, Rich. It's a nice idea to combine the functions. I applied the suggested changes. Unfortunately during testing I found that ConvertMaskBitMap() overruns the output buffer by about twice the available size, for both sizes. For a 40x40 icon it writes outside the buffer when writing ndx=0,bitNdx=0 in row=15 (with row starting at 40).
(In reply to comment #11)
> during testing I found that ConvertMaskBitMap() overruns the output buffer
> by about twice the available size, for both sizes. For a 40x40 icon it writes
> outside the buffer when writing ndx=0,bitNdx=0 in row=15 (with row starting
> at 40).

I'll try to figure out how it could be so wrong.  If you comment-out ConvertMaskBitMap(), does the color bitmap display OK (presumably with black pixels where the transparent pixels should be)?  I want to be sure that there aren't any issues with ConvertColorBitmap() (e.g. the bitmap it creates is the wrong size).
Yes, if I comment it ConvertMaskBitMap() out then it does display OK (at least if I set a non-white background, otherwise it's invisible because all transparent). And I do get 40x40 and 20x20 pixels on my system for large and small icons.
(In reply to comment #12)
> I'll try to figure out how it could be so wrong.

Once I looked at the code, it took about 10 seconds to figure out.  Because the rows are DWORD-aligned, a 40x40 icon has 24 bits of padding per row & a 20x20 has 12 bits (64-40=24 & 32-20=12, respectively).  Those extra bits caused an overflow exactly where "expected":  after doing 25 rows (40x40 = 25x64).

I'll have a revised version for you by tomorrow.
ConvertMaskBitMap() has been revised to deal with the padding bits found in most icon formats.  The rest of the file is the same as the original version.
Attachment #298266 - Attachment is obsolete: true
Attached patch working fixSplinter Review
Thanks Rich, this works perfectly now. Nice display and no crashes. :-)

Walter, I have changed it to only enable it for Firefox (MOZ_PHOENIX) where I think I haven't heard about crashes yet. And yes, I have seen the problem that files without extension don't get an icon. The problem is that GetIcon only gets the file extension that is created somewhere else, so the current method doesn't work. I'll have to study this a bit more, so let's postpone both issues to follow-up bugs.

Because Mike has already given this positive review in bug 305061 and I'm happy with the code, too, I'll mark this r+ and check it in in a minute.
Assignee: nobody → mozilla
Attachment #296851 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #298519 - Flags: review+
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Depends on: 413529
Depends on: 413530
I have built Seamonkey with icon support on and indeed the WPSWizard icons do show up when I open the local drives.  Interestingly, when I open a ftp site I get the icons as if WPSWizard were not running.
You should switch to HTML based network dir format (network.dir.format=2), otherwise you get the icons from the SeaMonkey theme.
What I see here is that in HTML format I get the zip icon but some generic folder icon (yellow).  With XUL (network.dir.format=3) I see the icons that WPSWizard replaces for folders and OpenOffice documents but a generic icon for zip files (locally that is, on the ftp server if I have xul selected I get the icons I would have if I turn off WPSWizard [these are David Graser's BlueAngled icons that I used the eCS icon replacement program that replace the icons in the DLLs] - which is not the same as the generic folder icon I get if I switch to HTML).  I'll do some screenshots, not that it is critical.
Attached image xul on: local
Locally with xul I do see zip icons.
Attached image html on: local
Attached image xul on: ftp
Normal WPS icons for folders but no zip icon.
Attached image html on: ftp (obsolete) —
Zip icon but generic folder icon.
HTML does give better consistency, if not the folder icons I would expect.
Attached image html on: ftp
This is the correct jpg for ftp with html.
Attachment #298575 - Attachment is obsolete: true
(In reply to comment #20)
Add the fix I proposed in https://bugzilla.mozilla.org/show_bug.cgi?id=413530 and see what happens (if anything).
You need to log in before you can comment on or make changes to this bug.