133615 - Need to remove "ISO-2022-KR" from SaveAs dlgbox

Status: RESOLVED WORKSFORME

(Core :: Internationalization, defect)

Description

[Roy Yokoyama]

Slit from 132070

We don't have all the Unicoder Encoders in Composer and we'd like
to have a generic way to hide the encoder name from SaveAs dlgbox

One of suggestions are:
>we may make a new field for charset like 'iso-2022-kr.notForOutgoing'
>and set it to 'True' in charsetData.properties.
>Then, your first patch can be made generic and you don't have >'special-case' it.

Comment 1

[Roy Yokoyama]

adding [e-mail]

Comment 2

[Jungshik Shin]

The first patch (attachment 75081 [details] [diff] [review]) to bug 132070 could have been easily modified
to check 'notForOutgoing' to determine whether a charset has to be hidden or
not. Unfortunately, the charset tree is not built one by one any more due to the
change made to EditorSaveAsCharset.js last August by Neil. (for bug 144957) [1].
I'm not familiar with RDF and have little idea how 'RDF magic' build the charset
tree 'automagically'. If I knew, I would check out if it's possible to exclude
charset with '.notForOutgoing' in charsetData.properties. I'm adding Neil to Cc
to seek his help in this regard. 

Neil, can you help me with this? I'm wondering whether it's possible and how I
can, if possible, selectively remove charsets from the charset tree built by
'RDF magic'? depending on their properties (.notForOutgoing) specified in
charsetData.properties file 

BTW, I hacked nsCharsetMenu.cpp and the accompanying xul file [2] until I
realized that hiding charsets not for outgoing for 'composer' there doesn't hide
it from 'Save As Charset' list. However, it seems like hiding them from the
charset menu of 'mail edit' should be useful. 

[1]
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=EditorSaveAsCharset.js&branch=&root=/cvsroot&subdir=mozilla/editor/ui/dialogs/content&command=DIFF_FRAMESET&rev1=1.23&rev2=1.24

[2]http://lxr.mozilla.org/seamonkey/source/xpfe/components/intl/nsCharsetMenu.cpp

Comment 3

[[e-mail]]

> Neil, can you help me with this? I'm wondering whether it's possible and how 
> I can, if possible, selectively remove charsets from the charset tree built 
> by 'RDF magic'? depending on their properties (.notForOutgoing) specified in 
> charsetData.properties file
>
> BTW, I hacked nsCharsetMenu.cpp until I realized that hiding charsets not for 
> outgoing for 'composer' there doesn't hide it from 'Save As Charset' list.

That's because that list uses the .notForBrowser properties - you need to create
a new list (NC:EncodersRoot?) for the .notForComposer properties.

Oh, and the RDFliner - it just interally queries the container elements and
reads the values, just like the code in pref-charset.js but quicker!

Comment 4

[Jungshik Shin]

I'm sorry I was not  clear(reading what I wrote, I realized that it's not clear
at all).   I tested '.notForOutging' in nsCharsetMenu.cpp and accompanying xul
files *after* adding  that attribute (in charsetData.properties file in
intl/uconv/src) to charsets (ISO-2022-KR and Visual Hebrew) I want to hide from
SaveAsCharset menu until I realized that 'composerMenu' in nsCharsetMenu.cpp is
NOT for exporting files BUT for importing files. So, I was hacking the 'wrong'
menu. 
 

> Oh, and the RDFliner - it just interally queries the container elements and
> reads the values, just like the code in pref-charset.js but quicker!

> you need to create a new list (NC:EncodersRoot?) for the 
> .notForComposer properties.

  Aha, this is it. Thanks a lot ! I'll make a patch. 

Comment 5

[Jungshik Shin]

Attachment: a simple patch per Neil's suggestion

This is a very simple patch. In addition to ISO-2022-KR and Hebrew (Visual),
I just added all the charsets with .notForBrowser to charsetData.properties
file with .notForOutgoing. I  could have made nsCharsetMenu.cpp more complex
but with fewer entries with .notForOutgoing in charsetData.properties  by
getting any entry with .notForBrowser excluded from SaveAsCharset menu as well.

Comment 6

[[e-mail]]

Comment on attachment 123699 [details] [diff] [review]
a simple patch per Neil's suggestion

Simpler than I thought!

>@@ -696,6 +701,7 @@
>     res = rdfUtil->MakeSeq(mInner, kNC_ComposerCharsetMenuRoot, NULL);
>     if (NS_FAILED(res)) goto done;
>     res = rdfUtil->MakeSeq(mInner, kNC_DecodersRoot, NULL);
>+    res = rdfUtil->MakeSeq(mInner, kNC_EncodersRoot, NULL);
>     if (NS_FAILED(res)) goto done;
It looks as if there's an if (NS_FAILED(res)) missing here...

>@@ -964,6 +971,7 @@
>     if (NS_FAILED(res))  return res;
> 
>     res = InitMoreMenu(othersDecoderList, kNC_DecodersRoot, ".notForBrowser");                 
>+    res = InitMoreMenu(othersDecoderList, kNC_EncodersRoot, ".notForOutgoing");                 
>     if (NS_FAILED(res)) return res;
And here...

Comment 7

[Jungshik Shin]

Attachment: a new patch with error checking

Added error checkings as Neil pointed out. Also made 'customize' menu of
mailedit 
charset use NC:Encoders_Root.

Comment 8

[Jungshik Shin]

Comment on attachment 123772 [details] [diff] [review]
a new patch with error checking

Asking r/sr. 
Roy, can you also take a look?

Comment 9

[[e-mail]]

OK, so now you can't compose mail with ISO-2022-KR, and you can't save as
charset with ISO-2022-KR, but in editor you can see ISO-2022-KR in the customize
dialog and also under view/coding/more/east asian; this looks wrong to me
(perhaps editor's charset menu should look like mail compose), is that within
the scope of this bug?

Also, where you check for mailedit,
+  if (applicationArea.indexOf("mail") != -1) {  // mailedit
+    LoadAvailableCharSets("NC:EncodersRoot");
+  } else {                                      // browser, mailview, composer
+    LoadAvailableCharSets("NC:DecodersRoot");
+  }
the second comment is really bad. As your test is for "mail" your comment looks
odd that mailview doesn't match. In fact the other menus all look like browser.
Perhaps to fix the customize dialog for editor as well you need to really use
browser, mailview and editor and then you can test for edit:
if (/edit/.test(applicationArea)) { // editor, mailedit
  LoadAvailableCharSets("NC:EncodersRoot");
} else {                            // browser, mailview
  LoadAvailableCharSets("NC:DecodersRoot");
}

Comment 10

[Jungshik Shin]

> in editor you can see ISO-2022-KR in the customize dialog
> and also under view/coding/more/east asian

  Yes, you must be able to select  iso-2022-kr there. That's because in composer
'View | Character Coding' is used for two purposes. One is for importing
_external_ html files (composed _outside_ Mozilla) [1] and the other is for
setting charset for the current document. For the former, we need to let users
select charsets tagged with '.notForOutgoing'. Some UI change is necessary to
cleanly separate these two functionalities.  Actually, I'm not sure whether that
menu is doing the second function as well. If not, it's all right (although
confusing) as it is now. (I can't test it right now). Anyway, if any UI change
is necessary, I guess that's beyond the scope of this bug. 

> In fact the other menus all look like browser

   As you found, pref-charset.xul invokes pref-charset.js that way (with
'mailedit' for mailedit and 'browser' for all others) and I didn't bother to
change it. 

+  if (applicationArea.indexOf("mail") != -1) {  //mailedit
 
Perhaps, I'll replace the above line with 

+  if (applicationArea == "mailedit") {   // mailedit

or equivalent with RE.

[1] As I wrote in comment #2 and comment #4, I also thought that View |
Character Coding is _solely_ for setttig the charset of files to publish, which
turned out not to be the case. 

Comment 11

[[e-mail]]

Comment on attachment 123772 [details] [diff] [review]
a new patch with error checking

In that case it looks fine to me --> Roy

Comment 12

[Roy Yokoyama]

Comment on attachment 123772 [details] [diff] [review]
a new patch with error checking

/r=yokoyama

Comment 13

[jag (Peter Annema)]

Comment on attachment 123772 [details] [diff] [review]
a new patch with error checking

+  if (applicationArea.indexOf("mail") != -1) {  // mailedit


What about this?

Comment 14

[Jungshik Shin]

Thank you, Neil and Roy for review.

+  if (applicationArea == "mailedit") {   // mailedit

jag, would you be satisfied with the above, instead? Actually, there's another
instance of  'if (applicationArea.indexOf("mail") != -1)' several lines above in
the original file (not in the patch). I can replace that as well.  

Comment 15

[Jungshik Shin]

I took a second look at what 'View | Character Coding' in Composer is supposed
to do. It's pretty confusing. 

1. When you open an empty(new) composer window, it's definitely for setting the
charset for publishing/saving. 

2. When you open a composer window by perssing 'Ctrl-E' (or File| Edit Page) and
import a page into the composer window, it appears that it's just used to
indicate the charset you set in the browser window immediately before importing.
The bullet point appears to the left of that charset. Changing it to another
value has no effect. 

If it's just for #1, what Neil suggested makes perfect sense. Even with #2, we
may go that way, but I'm less certain. ISO-2022-KR is rare enough, but some
other charsets tagged with notForOutgoing (but NOT with notForBrowser) is more
frequently used (e.g Visual Hebrew), I guess. 

I think the answer depends on how often users import (instead of starting
afresh) html files (made outside Mozilla) that are in one of charsets marked
with 'notForOutgoing' but NOT with 'notForBrowser'. 

Comment 16

[jag (Peter Annema)]

Comment on attachment 123772 [details] [diff] [review]
a new patch with error checking

+  if (applicationArea == "mailedit") {   // mailedit

That seems good to me.

sr=jag iff the new behaviour is considered acceptable.

Comment 17

[Jungshik Shin]

Thanks for sr, jst.
Now adding bobj to CC.

Roy,Kat and Bob,

What would say about the current behavior? In a composer window, View|Character
Coding is kind of 'overloaded' (comment #15). We can committ my patch (with the
revision per jag and neil) for the now and keep this bug open for further work.
Or, do you think it's better to implement what Neil has suggested (plus some
more changes in nsCharsetMenu.cpp to hide ISO-2022-KR and Visual Hebrew in
secondary-tier menus as well: East Asia and Middle Eastern) ignoring the fact
that ' View|Character Coding' is overloaded? 

Comment 18

[Jungshik Shin]

attachment 123772 [details] [diff] [review] was checked in to the trunk, but I'll keep this open to
address the remaining issue.

Comment 19

[Simon Montagu :smontagu]

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

Comment 20

[Jungshik Shin]

Comment on attachment 123772 [details] [diff] [review]
a new patch with error checking

This is to fix a long-standing bug (noted in I18N release notes) that
'character codings' for which we have no encoder (Unicode to character coding)
are listed in 'SaveAsCharset' menu. By not listing them in 'SaveAsCharset'
menu, we prevent Mozilla from confusing users (they can select 'character
coding' only to be told later that   they can't save to a file in the selected
character coding).
risk : low to those who never uses two character codings in question
(ISO-2022-KR and Visual Hebrew)

Comment 21

[Asa Dotzler [:asa]]

Comment on attachment 123772 [details] [diff] [review]
a new patch with error checking

moving approval request forward.

Comment 22

[Asa Dotzler [:asa]]

Comment on attachment 123772 [details] [diff] [review]
a new patch with error checking

We're out of time for 1.4.1. Let's get this into the trunk for 1.5.

Comment 23

[Henri Sivonen (:hsivonen) (on vacation)]

ISO-2022-KR is gone from mozilla-central.