57549 - [PATCH] Image type not shown in titlebar

Status: VERIFIED FIXED

(Core :: Internationalization: Localization, defect, P3)

Description

[Karl Ove Hufthammer]

When viewing images directly (not embedded in web pages), the title bar isn't
localizable.

1 Go to <URL:http://home.no.net/huftis/mozilla/bilde/skal-gul.png>
2 Titlebar now says 'PNG Image 602x402 pixels - Mozilla (build ...)
3 Both the text 'PNG Image' (or 'Jpeg Image' or whatever), the text 'pixels' and
the 'x' (multiplication sign' should be localizable. They currently are *not*.

Comment 1

[rchen]

Reassigned to ftang.

Comment 2

[Karl Ove Hufthammer]

Adding nsbeta1 keyword to localizability bug.

Comment 3

[Frank Tang]

The hard coded string is in the following location- end of
mozilla/layout/html/document/src/nsImageDocument.cpp

397 attinasi 1.37 // append the image information...

398 scc 1.43 titleStr.AppendWithConversion( " Image" );

399 nisheeth 1.26 if (mImageRequest) {
400                   PRUint32 width, height;
401                   mImageRequest->GetNaturalDimensions(&width, &height);

402 attinasi 1.40 // if we got a valid size (sometimes we do not) then display it

403 attinasi 1.37 if (width != 0 && height != 0){

404 scc 1.43 titleStr.AppendWithConversion( " " );
405                     titleStr.AppendInt((PRInt32)width);
406                     titleStr.AppendWithConversion("x");
407                     titleStr.AppendInt((PRInt32)height);
408                     titleStr.AppendWithConversion(" pixels");

409 attinasi 1.37 }

410 nisheeth 1.26 }

411 attinasi 1.37 // set it on the document

412 nisheeth 1.26 SetTitle(titleStr);


reassign to attinasi

attinasi- please fix the localizability issue ASAP. Thanks. Let rchen or tao
know if you need some info about how to do it.

Comment 4

[rchen]

The way to fix it is to remove the string to a properties file 
and read it from the file so that the localizers can translate the string into 
the different languages. 

Comment 5

[Marc Attinasi]

Thanks, rchen, for the tip. Is there a standard API I should use to read the
properties file entry?

Comment 6

[Marc Attinasi]

Any pointer to an existing example of a property file being used would be
helpful as well...

Comment 7

[rchen]

Frank, can you give Marc an example? Thanks.

The bug is not resolved fixed.

Reopen it.

Comment 8

[Marc Attinasi]

Reaccepting...

Comment 9

[Marc Attinasi]

OK, I think I have this figured out now (thanks to sfraser for the
nsIStringBundle clue). I'll attach my patch and a couple of new files for review...

Comment 10

[Marc Attinasi]

Attachment: Patch to make 'Image' localizable in nsImageDocument

Comment 11

[Marc Attinasi]

Attachment: ImageDocument.properties file with the one string in it

Comment 12

[Marc Attinasi]

Attachment: JAR file for jarrin' up the properties file

Comment 13

[Marc Attinasi]

I only tested my changes on Windows and Linux - is there something else I have
to do for the Mac?

Comment 14

[Marc Attinasi]

setting milestone M0.8

Comment 15

[Marc Attinasi]

The fixes are attached - still waiting for a review.

Comment 16

[tao]

Hi, Marc:

Some minor points:

1. Makefile.in. Unix uses "/" instead of "\" as the directory delimitor. 
   Please verify patches on all platform again since you are adding new file.
   For Mac, sfraser can help you on adding new files to build process.
2. nsImageDocument.cpp. You don't need to "getspec()" from a uri. 
   CreateBundle() takes "const char*" as the uri string.

thx

Comment 17

[tao]

Oops, I forgot to mention another point: I didn't see "x" and "pixel" been 
externalized into the property file. Is this intentional?

Comment 18

[Marc Attinasi]

Thanks for the review, Tao. You caught my big mistake of not externalizing
'pixels'. As for the X, I'm not sure what to do with that, since it is used to
indicate 'by' as in 10 X 10 pixels (ten by ten pixels). Can that really be
translated? I guess it is best to externalize it and let the localizers try, eh?

I'll get new patches soon. Thanks again!

Comment 19

[Marc Attinasi]

Attachment: New Patch: as per tao's comments

Comment 20

[Marc Attinasi]

Attachment: new ImageDocument.properties file: has 'pixels' and 'x' now as well

Comment 21

[Marc Attinasi]

I have attached a new patch and new ImageDocument.properties file. 

The other two pieces of text (pixels and x) are now externalized, and the 
CreateBundle is passed the properties file URL as a const char* directly (much 
nicer!). Also, I fixed the paths for the UNIX makefile, although the DOS-path 
delimiter seemed to work (really, I did test it on Linux). I have not retested 
teh makefile.in on Linux but I will on Monday (Linux machine at home is dead).

Tao, please check again when you have a chance - thanks!

Comment 22

[tao]

Hi, Marc:


Thanks for the quick action. A few nit-picking points:
1. you might want to use upper case for macro definition: the property file url.
2. I am wondering if you can 'extend' the life span of the handle to 
   nsIStrinBundleService. With the current code, it is "initialized" three times
   for each string retrieval; same thing happens to the stringBundle. The latter
   cached by strinbundleservice already, though.
3. I didn't see the patch for Mac's MANIFEST file which exports the property
   file to dist. This is fine when jar packaging is turned on (the current
   default in the current release/tinderbox build process.)
   It will break someone's debug build if s/heturn jar packaging off. 

(3) is optional since I am not sure if anyone does turn-off the jar.mn stuff.
I brought it up because I saw your changes in win/unix makefiles.


thx

Comment 23

[Marc Attinasi]

Attachment: MANIFEST_PROPERTIES for Mac

Comment 24

[Marc Attinasi]

Attachment: New Patch: more efficient version

Comment 25

[Marc Attinasi]

I uppercased the URL macro and improved the lookup function to avoid repeatedly 
initializing the service and the bundle. I also added what I believe to be the 
Mac manifest file we neeed (still need to test on a Mac). Please review this 
one, tao, and hopefully it is correct now! Thanks.

Comment 26

[tao]

please verify the patch on mac/linux. r=tao. Thanks.

Comment 27

[tao]

BTW, I didn't see any change in Mac's build script to install/copy files in
MANIFEST_PROPERTIES. Is this intentional?

Comment 28

[Marc Attinasi]

Regarding the Mac - nothing is intentional ;) I don't even know where the Mac
build scripts are... I have not yet contacted sfraser for his help, so I was
just following the lead of layout/html/forms in creating the MANIFEST_PROPERTIES
file. I have no idea of what it means to update the Mac, but I will be learning
that very very soon. And, to be clear, I will test on all three main platforms
before I check in. There is a Mac developer here who will test it for me.
Thanks, Tao.

Comment 29

[Marc Attinasi]

Attachment: Patch to add the new jar.mn file to the Mac build script

Comment 30

[Blake Ross]

Attachment: [patch] localize entire string to allow word reordering

Comment 31

[Blake Ross]

Attached a new patch to localize the entire string, so words can be reordered 
(saw Erik's comment in n.p.m.reviewers).  Marc, can you review this?  Thanks.

Comment 32

[Marc Attinasi]

Blake, it looks great to me! Thanks for taking this on, I appreciate it. r=attinasi

Comment 33

[Erik van der Poel]

sr=erik

Comment 34

[Blake Ross]

Marc: no problem; I was doing a similar fix for another bug, and figured I 
could help here.  Thanks for the earlier patches.

Fix checked in.

Comment 35

[Teruko Kobayashi]

Changed QA contact to [e-mail]. Yuying, after we get Japanese 
localized build, please verify this.  

Comment 36

[Yuying Long]

I checked 03-29 trunk build, there is no string like "PNG Image...." inside the 
title bar, although we still don't have any licalized build at this point, but 
for English version there is no string exists any more.  Is it the right way?  
I'll re-open it, please change the status if I am wrong.

Comment 37

[Blake Ross]

Damn.  Someone broke this, it definitely used to work.  I want to blame pav, 
but I don't have any proof.  Yuying, can you try a few older nightly builds so 
we can get an idea of when this regressed?

Comment 38

[Yuying Long]

Seems the string still exists on 03-16 trunk build, so must be something after 
that.

Comment 39

[Michele Carlson]

status please?

Comment 40

[Marc Attinasi]

I'll investigate. Clearing the [fix in hand] notation as that was for the old
fix. Adding regression

Comment 41

[Marc Attinasi]

Cool - the bulk of the work to put the image name in the title is done in
nsImageDocument::UpdateTitle (nsImageDocument.cpp). The bulk of that routine is
wrapped in a #ifndef USE_IMG2 block, so it is not compiled in right now.

The code in that block does not compile now, however I moved the #ifndef so that
it just wraps the image size gathering code and it compiles and works OK.

Pav, how do I do this now?

  > mImageRequest->GetNaturalDimensions(&width, &height);

Comment 42

[Marc Attinasi]

Never mind Pavlov, I figured it out. Patch coming...

Comment 43

[Marc Attinasi]

Attachment: Patch for fix getting image size using IMG2 (libpr0n)

Comment 44

[Marc Attinasi]

Description of change: with the new image interfaces we need to get the image
from the request, then query the image for the height and width. In the old
interfaces we jsut asked the image request for the dimensions. The change
supports both interfaces via the USE_IMG2 #ifdef already established.

Comment 45

[Stuart Parmenter]

r=pavlov

Comment 46

[Chris Waterson]

So, just the last patch? If so, sr=waterson

pavlov, have you left any other #ifdef treats for us to find?

Comment 47

[Stuart Parmenter]

chris: only in rdf ;)

Comment 48

[Christopher Blizzard (:blizzard)]

a=blizzard for 0.9

Comment 49

[Boris Zbarsky [:bzbarsky]]

*** Bug 76213 has been marked as a duplicate of this bug. ***

Comment 50

[Marc Attinasi]

Fixed

Comment 51

[dylang]

Does this fix the that that the throbber keeps going and going and going with an
image loaded?

That's part of the issue I reported in bug 76213

Comment 52

[Boris Zbarsky [:bzbarsky]]

OK.  Here's what I'm seeing:

1)  Open a blank new browser
2)  Go to an image URL (http://www.mozilla.org/images/mozilla-banner.gif)
3)  Throbber stops. The titlebar updates.
4)  Go to http://www.mozilla.org
5)  Go to http://www.mozilla.org/images/mozilla-banner.gif
6)  Throbber stops. The titlebar does _not_ update.
7)  Reload
8)  Throbber stops. The titlebar updates.

So this is not completely fixed. Looks like nsImageDocument::EndLayout() is not
always called....  Should this just be a separate bug?

Comment 53

[Brendan Eich [:brendan]]

Yes, please file a separate bug if you haven't yet, and cite it here.  Thanks,

/be

Comment 54

[Boris Zbarsky [:bzbarsky]]

bug 77378 filed

Comment 55

[Yuying Long]

Mark it as verified due bug 77378 has been filed.