Closed
Bug 411332
Opened 17 years ago
Closed 17 years ago
Fix OS/2 icon handler and add icon logic using RWS
Categories
(Core :: Graphics: ImageLib, defect)
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.
Comment 1•17 years ago
|
||
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)?
Assignee | ||
Comment 2•17 years ago
|
||
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...
Assignee | ||
Comment 3•17 years ago
|
||
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.
Assignee | ||
Comment 4•17 years ago
|
||
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
Assignee | ||
Comment 5•17 years ago
|
||
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.
Comment 6•17 years ago
|
||
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.
Comment 7•17 years ago
|
||
(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.
Comment 8•17 years ago
|
||
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.
Assignee | ||
Comment 9•17 years ago
|
||
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 ;-)).
Comment 10•17 years ago
|
||
(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?
Assignee | ||
Comment 11•17 years ago
|
||
(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).
Comment 12•17 years ago
|
||
(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).
Assignee | ||
Comment 13•17 years ago
|
||
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.
Comment 14•17 years ago
|
||
(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.
Comment 15•17 years ago
|
||
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
Assignee | ||
Comment 16•17 years ago
|
||
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+
Assignee | ||
Comment 17•17 years ago
|
||
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment 18•17 years ago
|
||
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.
Assignee | ||
Comment 19•17 years ago
|
||
You should switch to HTML based network dir format (network.dir.format=2), otherwise you get the icons from the SeaMonkey theme.
Comment 20•17 years ago
|
||
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.
Comment 21•17 years ago
|
||
Locally with xul I do see zip icons.
Comment 22•17 years ago
|
||
Comment 23•17 years ago
|
||
Normal WPS icons for folders but no zip icon.
Comment 24•17 years ago
|
||
Zip icon but generic folder icon.
HTML does give better consistency, if not the folder icons I would expect.
Comment 25•17 years ago
|
||
This is the correct jpg for ftp with html.
Attachment #298575 -
Attachment is obsolete: true
Comment 26•17 years ago
|
||
(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.
Description
•