118915 - [RFE] Please add filename to image title

Status: VERIFIED FIXED

(Core :: Layout, enhancement)

Description

[Stephen Moehle]

When viewing an image (.gif, .jpg, .etc), the browser window title is "Image nxn
pixles"  Could the filename be included in the title?  That would make having a
lot of windows or tabs open much easier to manage.

Comment 1

[Daniel Brooks [:db48x]]

it'll also make bookmarking images slightly more sane.

Comment 2

[Morten Nilsen]

Just made a patch for this... attaching in a sec

Comment 3

[Morten Nilsen]

Attachment: proposed patch

Comment 4

[Morten Nilsen]

really taking this time

Comment 5

[Morten Nilsen]

Attachment: revised patch

fixes a typo, and removes some dead code

Comment 6

[Daniel Brooks [:db48x]]

Comment on attachment 64450 [details] [diff] [review]
revised patch

r=db48x

Comment 7

[Alec Flett]

Comment on attachment 64450 [details] [diff] [review]
revised patch

close.. use NS_ConvertUTF8toUCS2() - urlspecs are expected to be in utf8

also, use nsXPIDLCString rather than a raw string and nsCOMPtr<nsIURL> rather
than a raw nsIURL - it will clean up the code significantly

Comment 8

[Morten Nilsen]

Attachment: patch v3

Comment 9

[Morten Nilsen]

Attachment: patch v3.1

new, timeless nit-pick free, patch

Comment 10

[Daniel Brooks [:db48x]]

I think there's a slight logic error in there. you have:

if (&pName)
 show "%S (Image %Sx%S)"
else
 show "Image"

which basically kills the dimensions if it can't find the filename. You need to 
have four cases now, one where you have the name and the dimensions, two where 
you have either the name or the dimensions but not both, and another where you 
have neither. However, you probably don't need to be that verbose, you could 
have an entity to substitute the filename into, one to insert the dimensions 
into, and just concatinate them for the output. You'd still need a third entity 
for the case where you had neither bit of data. Or, you could just go the 
verbose way in the intrest of readability and maintainibility, especially if 
there might later need to be a change in the format, it's easier to change a 
couple of l10n strings than the js code.

sorry, can't r= it just yet, but once you get that fixed it'll be no problem

Comment 11

[Daniel Brooks [:db48x]]

also apologies for not catching that the first time around, would have saved 
time.

Comment 12

[Morten Nilsen]

making a new patch... I really need to cut back on the internet use...

coming up later today probably...

Comment 13

[Morten Nilsen]

Attachment: patch v3.2

db48x: the logical error you mentioned was a different one than actually
excisted.
to result of the logical error would be that the title is set to Image (Image
XxY) when no filename was found... and Image (Image) if neither filename nor
size was found.

this new patch is a slightly different approach, and should be much better...

Comment 14

[Daniel Bratell]

Comment on attachment 64710 [details] [diff] [review]
patch v3.2

I don't see ImageTitleWithoutDimensions used at all in the patch. Shouldn't the
filename be inserted there if there are no width and height. (Does that case
even exist)

Comment 15

[Morten Nilsen]

it's in the .cpp file, but not in my patch...

but now that you mention that, I suddenly see a few cases where this patch is
incomplete... v3.3 coming up

Comment 16

[Morten Nilsen]

Attachment: patch v3.3

Comment 17

[Daniel Bratell]

Comment on attachment 64725 [details] [diff] [review]
patch v3.3 

Looks better to me. The worst that could happen is that the title becomes:
" (Image)". I'm not familiar enough with the code to give formal reviews
though.

Comment 18

[Morten Nilsen]

Attachment: patch v4.0

this patch implements db48x' suggestions about using a nested if at the end...
I've tested it, giving these results:
filename, size: filename (Image NxN pixels)
filename: filename (Image)
size: (Image NxN pixels)
neither: (Image)

this looks all good and well :)

Comment 19

[Daniel Brooks [:db48x]]

Comment on attachment 64752 [details] [diff] [review]
patch v4.0

looks good, r=db48x

Comment 20

[Morten Nilsen]

cc'ing alecf to get sr=

Comment 21

[Alec Flett]

Comment on attachment 64752 [details] [diff] [review]
patch v4.0

close, but no cigar:
- use nsXPIDLCString instead of char* for pName
- this makes no sense:
+    nsCOMPtr<nsIURL> pURL = do_QueryInterface(mDocumentURL);
+    if (NS_SUCCEEDED(mDocumentURL->QueryInterface(NS_GET_IID(nsIURL),(void
**)getter_AddRefs(pURL)))) {



You're randomly assigning a value to pURL, and then immediately reassigning the
value in the next line

how about:

nsCOMPtr<nsIURL> url = do_QueryInterface(mDocumentUrl);
if (url) {
...

}

- don't create intermediate nsAutoStrings when you could be using the
LITERAL_STRING directly:

i.e. instead of 

nsAutoString foo;
foo.Assign(NS_LITERAL_STRING("foostring"));
somehowUseFoo(foo.get(),...)

instead say:
somehowUseFoo(NS_LITERAL_STRING("foostring").get(),...)

- instead of if (fileStr) say if (!fileStr.IsEmpty())

- why do the localized strings have to be in parentheses? i.e. why "(Image
%Sx%S pixels)" and not "Image %Sx%S pixels"

But the basic approach looks ok

Comment 22

[Alec Flett]

by the way, db48x, this is all stuff that should have been caught in a general
review - please don't mark r= if you aren't familiar with the standard mozilla
string/nsCOMPtr operations.

Comment 23

[Morten Nilsen]

> - use nsXPIDLCString instead of char* for pName

I can't do that.. because:
no matching function for call to `nsDerivedSafe<nsIURL>::GetFileName
(nsXPIDLCString *)'
candidates are: nsresult nsIURL::GetFileName(char **)

but I've taken your other comments into account... new patch coming later today.

Comment 24

[Morten Nilsen]

Attachment: patch v4.1

still using char * pName...

seems logical to me to put use (Image) as it's not the name of the file...

"filename (Image NxN) " looks better than "filename Image NxN" IMHO

Comment 25

[Morten Nilsen]

Attachment: testcase

this testcase needs a php-aware server, and will give means to test the four
cases of titles...
the expected results are in the html file...
and worked fine with my current patch.

Comment 26

[Morten Nilsen]

Attachment: patch v4.2

If all goes my way, final patch

Comment 27

[Daniel Brooks [:db48x]]

Comment on attachment 65017 [details] [diff] [review]
patch v4.2

well, now that we both know what to use instead of char*, and where to look for
that info, r=db48x

Comment 28

[Alec Flett]

Comment on attachment 65017 [details] [diff] [review]
patch v4.2

*sigh*

please go read other code, look for examples which use nsXPIDLCString... don't
blindly just substitute the type and say "it doesn't work" 
however, I'll give you a lesson right here:

nsXPIDLCString foo;
GetSomeString(getter_Copies(foo));

db48x, please stop marking r= if you're not familiar with this stuff...

Comment 29

[Morten Nilsen]

didn't you read my latest patch? I allready have done so...
no more char *

Comment 30

[Alec Flett]

Comment on attachment 65017 [details] [diff] [review]
patch v4.2

ack. my bad. this looks great.

Comment 31

[Morten Nilsen]

checked in

Comment 32

[Morten Nilsen]

v