Files without extensions don't get an icon

RESOLVED FIXED

Status

()

Core
ImageLib
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Peter Weilbacher, Unassigned)

Tracking

Trunk
x86
OS/2
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

10 years ago
 
(Reporter)

Comment 1

10 years ago
Follow-up to bug 411332 comment 10 (last paragraph): when displaying local folders files with an extension correctly display as
   O filename.ext
(where O is the icon). Files without extension don't get an icon, so replacement text is displayed instead which looks like
   File: filenamewithoutextension

Not sure if this is actually to be fixed in modules/libpr0n/decoders/icon/os2/nsIconChannel.cpp (where I so far have only encountered extensions without filenames) or if this has to be changed somewhere else.
Blocks: 411332
Summary: Files without extensions don → Files without extensions don't get an icon

Comment 2

10 years ago
I looked at the code & the logic is flawed but the fix is simple.
Here's the relevant code from nsIconChannel::MakeInputStream()

267   nsresult rv = ExtractIconInfoFromUrl(getter_AddRefs(localFile),
268                                        &desiredImageSize, contentType,
269                                        filePath);
270   NS_ENSURE_SUCCESS(rv, rv);
271 
272   // if the file exists, get its path
273   PRBool fileExists = PR_FALSE;
274   if (localFile) {
275     localFile->GetNativePath(filePath);
276     localFile->Exists(&fileExists);
277   }

This code is misleading because ExtractIconInfoFromUrl() returns an extension in "filepath", not a path.  The following lines - which should replace it with a valid path - always fail because the URL that ExtractIcon was given doesn't have enough info to create one.  Here's a sample URL:
  moz-icon://xyz.zip?size=16&contentType=application/zip

The result is that only the extension gets passed to GetIcon().  For a file like "xyz.zip", it returns the icon that would be assigned to *.zip.  For a file like "readme" which has no extension, GetIcon() receives an empty string & bombs out.

The solution is to use a generic extension.  Since the OS assumes such files are plain-text, "txt" would be the most appropriate one.  The fix should go in GetIcon().
  Change:
458   if (file.IsEmpty())
459     return 0;
  to:
458   if (file.IsEmpty())
459     file.Append("txt");  // my recollection of nsCString methods is poor
(Reporter)

Comment 3

10 years ago
(In reply to comment #2)
> I looked at the code & the logic is flawed but the fix is simple.
> Here's the relevant code from nsIconChannel::MakeInputStream()
> 
> 267   nsresult rv = ExtractIconInfoFromUrl(getter_AddRefs(localFile),
> 268                                        &desiredImageSize, contentType,
> 269                                        filePath);
> 270   NS_ENSURE_SUCCESS(rv, rv);
> 271 
> 272   // if the file exists, get its path
> 273   PRBool fileExists = PR_FALSE;
> 274   if (localFile) {
> 275     localFile->GetNativePath(filePath);
> 276     localFile->Exists(&fileExists);
> 277   }
> 
> This code is misleading because ExtractIconInfoFromUrl() returns an extension
> in "filepath", not a path.  The following lines - which should replace it with
> a valid path - always fail because the URL that ExtractIcon was given doesn't
> have enough info to create one.  Here's a sample URL:
>   moz-icon://xyz.zip?size=16&contentType=application/zip

Yes, this is what I have been seeing. For local files in a dir listing I see either
   moz-icon://unknown?size=16
for files without extension or e.g.
   moz-icon://.gif?size=16
for those with. The localFile case can never happen, I wonder if we should remove it. We could also get rid of the code and parameters for the exists case in other functions.

> The solution is to use a generic extension.  Since the OS assumes such files
> are plain-text, "txt" would be the most appropriate one.  The fix should go in
> GetIcon().
>   Change:
> 458   if (file.IsEmpty())
> 459     return 0;
>   to:
> 458   if (file.IsEmpty())
> 459     file.Append("txt");  // my recollection of nsCString methods is poor

Shouldn't we use an extension that the user is less likely to have done anything about? I imagine that many people associate .txt files with something else than the default. So maybe use "a_generic_file_extension".

Comment 4

10 years ago
(In reply to comment #3)

The sample moz-icon:// URL I gave was generated by very old code;  it looks like you get even less info nowadays.  However, I wouldn't be in a hurry to remove any code.  With it there, we're ready if these URLs ever start delivering genuinely useful info.

WRT the default extension:  if you don't like "txt", I'd suggest "tmp" or "moztmp" or "pmwrlw" :-)  Anything with 7 or fewer characters is OK - if it's longer, nsRWS won't cache its icon.
(Reporter)

Comment 5

10 years ago
Created attachment 299023 [details] [diff] [review]
Rich's suggestion

Yes, "pmwrlw" I like. :-)

Rich's suggestion, so I give r+.
Attachment #299023 - Flags: review+
(Reporter)

Comment 6

10 years ago
And I noticed that Windows and Mac also still have code for the localFile/fileExists case so we should indeed leave this in for OS/2.

Comment 7

10 years ago
(In reply to comment #6)
> And I noticed that Windows and Mac also still have code for the
> localFile/fileExists case so we should indeed leave this in for OS/2.

I got a nightly for my Mac & found that it has the exact same issue.  Be on the lookout for a cross-platform solution - particularly one that breaks something.
(Reporter)

Comment 8

10 years ago
Given that cross-platform stuff is much harder to get into the tree, I have checked this patch in now, so that it is fixed in time for Firefox 3beta3.

You should probably create a separate bug for cross-platform stuff anyway, so I'll mark this fixed.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.