bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

save image (imagename) ... for composer image context menu

VERIFIED FIXED in mozilla0.9

Status

()

Core
Editor
--
enhancement
VERIFIED FIXED
18 years ago
17 years ago

People

(Reporter: Doron Rosenberg (IBM), Assigned: Charles Manske)

Tracking

Trunk
mozilla0.9
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Reporter)

Description

18 years ago
spun off 21515 - I'm working on a patch to get the imagename into the context
menus for browser/mailnews, and I think we should have it as well in composer's
context menu.  According to mpt, composer should have a "save image (imagename)
...". This means a new feature. I have a working patch that does this.

The questions that remain is, first, if the editor owners want this feature in
at all and if yes, which position it should take in the image-context menu

cc hurricane and mpt
(Reporter)

Comment 1

18 years ago
Created attachment 21488 [details] [diff] [review]
proposed patch

Comment 2

18 years ago
r=rcassin@supernova.org on the patch, smfr can you sr=?

Comment 3

18 years ago
assigning to cmanske
Assignee: beppe → cmanske

Comment 4

18 years ago
Please use <stringbundle/> instead of strres.js.
(Assignee)

Comment 5

18 years ago
I think this item should be place just below:
<menuitem id="objectProperties_cm" observes="cmd_objectProperties"/>
because if the context click is on an image, this item will display:
Image Properties...
so it makes sense for Save Image to be near the property item.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9
(Reporter)

Comment 6

18 years ago
Created attachment 22897 [details] [diff] [review]
patch with cmanske's order change request
(Reporter)

Comment 7

18 years ago
moved the item to the requested postion, anything else?
(Assignee)

Comment 8

18 years ago
One more requirement. In EditorFillContextMenu, there is the line:
  var haveProps = IsMenuItemShowing("objectProperties_cm");
we need to change this to:
  var haveProps =
    IsMenuItemShowing("objectProperties_cm") ||
    IsMenuItemShowing("menu_saveImage_cm");
This is used to decide if the separator after the properties menu item and
this new item should be displayed.
Also, while I realize using "menu_" is a good habit for menu IDs, we don't use
that for other context menu items, so maybe "saveImage_cm" is a more consistent
with what's there.
r=cmanske with the suggested changes. Should I check this in for you?
Simon: Can you SR this please?

Comment 9

18 years ago
The patch does a string bundle load:

  var bundle = srGetStrBundle("chrome://editor/locale/editor.properties"); 

this does a string bundle load. I thought using the <stringbundle> XUL tag was 
preferred (and sharable)?
(Assignee)

Comment 10

18 years ago
Thanks for pointing that out. I think we can simply use
"window.editorShell.getString()", which is what we use in all our JS.
I'll try that and test all of this further...
(Assignee)

Comment 11

18 years ago
Created attachment 22912 [details] [diff] [review]
Revised patch for XP portion
(Assignee)

Comment 12

18 years ago
Created attachment 22913 [details] [diff] [review]
Revised patch for editor (apply in mozilla/editor/ui/composer)
(Assignee)

Comment 13

18 years ago
Just one additional change: I used "%NAME%" instead of "%NAME" in
editor.properties since we already have a localization note telling others to
not translate text enclosed between "%".
Simon: All code is tested. Ready for SR now.

Comment 14

18 years ago
Then don't you need to change

+     var menutext = window.editorShell.GetString("SaveImageAs").replace(/%NAME/
,imageName);

to

+     var menutext = window.editorShell.GetString("SaveImageAs").replace(/%NAME%/
,imageName);

?

Otherwise sr=sfraser.
(Reporter)

Comment 15

18 years ago
yeah, timeless told me about the %NAME%, must have forgotten to change it. and 
yes, i don't have cvs commit rights yet, so if you can check this in.
(Assignee)

Comment 16

18 years ago
Yes, the change that Simon described was done.
Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 17

18 years ago
just a comment for cmanske et al:
You shouldn't need to do %NAME% and the s/// operator. Instead, you should use
bundle.formatStringFromName() - this is the l10n-perscribed way of doing
localization. it uses nsTextFormatter::smprintf() to do positional string
substitution as appropriate.

for instance, you would instead have the string read:
SaveImageAs=Save Image (%S) ...

and then use

caption = bundle.formatStringFromName("SaveImageAs", [imageName], 1);

this way the substitution is done inside the string bundle before returning the
string to the caller... the other bonus is that you don't have to argue about
specific conventions (%NAME vs %NAME%) because nsTextFormatter is standardized..

Comment 18

18 years ago
we'll keep that in mind. fwiw %name was purely accidental.

Comment 19

18 years ago
verified in 2/6 build.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.