69911 - "Save" not enabled when just change an existing file character coding (charset)

Status: RESOLVED WONTFIX

(SeaMonkey :: Composer, defect, P3)

Description

[Yuying Long]

Wrong feature of "Save" on N6/6.01 and recently Mtrunk build when just change an 
existing file character coding.

Steps to reproduce:
1. Launch N6/N6.5 and open Composer.
2. Open an exists html file, no matter it has meta-tag or not.
3. Change character coding by going View | Character Coding.
4. Save the file by click "Save" icon or go File | Save.

Result:
"Save" icon and menu item "Save" under File are disable, the changes can't be 
saved by "Save".

Note:
On N4.75, if you go same steps will pop-up a warning message dialog box after 
you "Save", and then overrride the charset and set the changed charset into 
meta-tag no matter whether it has meta-tag or not in original html file.

Comment 1

[Yuying Long]

Reassign to Roy and change the QA contact.

Comment 2

[Jaime Rodriguez, Jr.]

Roy - Please accept this one for nsbeta1 and set target milestone to M0.9.1

Comment 3

[marina]

re-assigning QA contact

Comment 4

[Roy Yokoyama]

jbetak: can you take a look at this?  This bug seems in your area.
Please assign back to me if it's not the case. Thanks

Comment 5

[Marek Z. Jeziorek]

if there's a cheap user workaround for this please set the target milestone to
0.9.2.

Comment 6

[[e-mail] (away - not reading bugmail)]

OK, this is essentially the same problem as bug 56195. Note that Teruko 
referred to "Save As" in bug 56195, since "Save" was most likely disabled when 
she tested. 

The "Save" menu item gets enabled as soon as the page content changes. When 
changing the viewing charset via "View|Character Coding->", the meta tag within 
the page doesn't change although it should. Hence the page content remains 
unchanged and the "Save" menu item continues to be grayed out. 

Since bug 56195 has been marked nsbeta1-, I'm futuring this also...

Comment 7

[Yuying Long]

Another case "Save" keep grey-out:
1. Open an existing file with meta-charset in Composer - better to have a 
double byte charset document, e.g. shift-jis.
2. Change the charset to something else(e.g. EUC-JP) by going: View | Character 
Coding | More.

You will see the character are missed up, that means the charset has been 
changed.  Then check the HTML source, the meta-charset still keep the old value.

So, even the document has been changed, but the "Save" menu and meta-charset 
won't get changed here.


 

Comment 8

[[e-mail] (away - not reading bugmail)]

Attachment: "work-in-progress" patch, this seems to cover the functional requirements pretty well. Requires some more refinement and thougth, I'm especially concerned about mail-related regressions.

Comment 9

[[e-mail] (away - not reading bugmail)]

Attachment: Patch #2 (cvs diff -u -w -b)

Comment 10

[[e-mail] (away - not reading bugmail)]

cmanske: 

although the proposed solution is not ideal, I think it's pretty self-contained 
and robust. Extending the nsIDocumentStateListener interface would be probably 
cleaner, but I don't think it's desirable in this context.

We are attempting to remedy the fact that a manual charset override in the 
composer will result in a document reload, which in turn negates all changes to 
the document's META charset. There is quite a number of mislabeled pages and 
it's quite sensible to correct the META charset, if it was deemed to be wrong 
by the user. I propose registering a dedicated document state listener, so we 
can reinsert the META charset changes into the document upon reload.

This scenario is a borderline case and I don't expect it to surface very often, 
if at all. The only reason for its visibility visible is bug 79735. Typically 
wrong charset labeling is corrected manually when viewing the document and not 
during the edit stage. Nevertheless we do offer the "Character Encoding" menu 
in the composer and should handle this correctly.

Comment 11

[Charles Manske]

Seems reasonable to me
r=cmanske

Comment 12

[[e-mail] (away - not reading bugmail)]

thanks Charlie! Updating whiteboard status and adding blizzard as i18n
super-reviewer.

Comment 13

[[e-mail] (away - not reading bugmail)]

oh, apparently I did not cc blizzard for possible sr the last time. Adding him 
to the cc list now ...

Comment 14

[Christopher Blizzard (:blizzard)]

sr=blizzard

Comment 15

[[e-mail] (away - not reading bugmail)]

thanks a lot Chris! Updating whiteboard summary.

Comment 16

[[e-mail] (away - not reading bugmail)]

patch checked in - thanks everyone!

Comment 17

[[e-mail] (away - not reading bugmail)]

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

Comment 18

[Yuying Long]

I checked it on 08-22 trunk build on all 3 platforms:
"Save" still not work - if you close the file after "Save" with a changed
charset, and re-open it again, or after "Save" the browse the page, the charset
still keep the old charset value.  Although the charset has been changed in
Composer HTML source once set charset to another one.

Re-opening it.

Comment 19

[[e-mail] (away - not reading bugmail)]

Attachment: this marks the document as dirty, when the charset has changed

Comment 20

[[e-mail] (away - not reading bugmail)]

cmanske would you help me out with yet another review? THis bugfix was 
unfortunately not complete and has been reopened...

Comment 21

[Charles Manske]

I realize that setting "gHTMLSourceChanged" will have the desired effect, but
that isn't technically the correct thing to do. I also realize that we don't 
have an interface to force the internal "document modified" flag to "modified".
I'll look into this more tomorrow.

Comment 22

[msanz]

minusing, too late for 0.9.4. Juraj, remove the minus if you disagree

Comment 23

[Charles Manske]

I'm going to take this bug since the fix I have is to add support for setting
"dirty flag" in composer code.

Comment 24

[Charles Manske]

I'll attach a patch with new Composer code required to fix this.

Comment 25

[Charles Manske]

tweaking summary to make it easier to searc for this.

Comment 26

[[e-mail] (away - not reading bugmail)]

cmanske, thanks for taking this! I look forward to seeing this resolved in a 
more elegant way.

Comment 27

[Charles Manske]

Attachment: Allow editor's "documentModified" flag to be writable so we can set it "true" when charset is changed.

Comment 28

[Charles Manske]

jbetak: Could you please review the editor patch. Thanks.
After changing the charset, the "Save" button is now enabled, but I am noticing 
one problem: After using "Undo", the charset is changed back to what it was 
before (looking at the meta tag in HTML Source reveals this), but then all of
the submenus under "Character Encoding" are disabled.
Is that a separate problem?

Comment 29

[Charles Manske]

Comment on attachment 44841 [details] [diff] [review]
Patch #2 (cvs diff -u -w  -b)

This patch is already 
checked in.

Comment 30

[[e-mail] (away - not reading bugmail)]

looks great: r=jbetak

BTW the problem you are seeing is both "a bug and a feature". Please see bug 
42837, where Shanjian wanted to avoid document reloaded to preserve
any meaningful document editing work. Attachment 10861 [details] [diff] shows how he did it.

Since changing the charset is now considered a document change itself, users 
would have only one shot at changing it. I don't think this reflects real-life 
usage and we should probably reopen bug 42837 or file a new one. Since "Undo" 
can tell what changes have been done to the document, could we use it to make 
sure the charset menu doesn't get disabled when only the charset has changed?

cc'ing shanjian, momoi-san

Comment 31

[Katsuhiko Momoi]

> Since changing the charset is now considered a document change itself, 
> users would have only one shot at changing it. I don't think this 
> reflects real-life usage

Our UE spec does not disallow character coding menu changes (any number
of them) on a new, existing, or just saved dsocuments as long as no input
has been entered. So the suggested change is OK as far as that is concerned.
You might however not want to allow infinite Undo's and might place
some arbitrary limit on Undo's while allowing unlimited number of 
menu changes on qualified documents.

Comment 32

[[e-mail] (away - not reading bugmail)]

cmanske:

I think I have an idea how to comply with the i18n UE spec and also limit the 
undo functionality Momoi-san mentioned in his comment. Could we simply disable 
undo via EnableUndo(false) at the beginning of NotifyDocumentStateChanged 
listener and re-enable it at the end? In the function updateCharsetPopupMenu we 
would check for documentModified and see if there are any undoable editing 
changes, the charset menu would get disabled only when both conditions are true.

If this sounds reasonable I could come up with an ammendment to your patch or 
file a new bug and take it from there.

Comment 33

[kinmoz]

I'm a little wary about allowing things other than the editor's transaction
manager listener to modify the DocModCount ... if you are modifying the doc by
adding a charset tag, you should be using a transaction which is undoable.

If we allowed the patch in, we could never get back to an unmodified state via undo.

Cc'ing sfraser for comment.

Comment 34

[Charles Manske]

I agree, the approach in the patch of 10/02/01 is probably not the right thing 
to do.

Comment 35

[Simon Fraser [no longer active]]

I don't like this either. The doc mod count should be incremented as the result 
of a transaction occurring, and not twiddled any other way. Is setting the doc 
charset an undoable transaction? Should it be?

Comment 36

[[e-mail] (away - not reading bugmail)]

>Is setting the doc charset an undoable transaction? Should it be?

I think it should be undoable. Making it undoable would also help us to
determine an appropriate moment to disable the charset menu (bug 42837). 

Momoi-san please correct me if I'm wrong, but most users don't expect or don't
care about the meta charset change in the document. They'll be tweaking the
presentation layer through the charset menu to make a document appear readable,
not to change it. I don't think they are expecting a document change to occur
and it wouldn't make sense to present it as such. We are overwriting the charset
to correct a mislabeled document and make it easier for us to preserve manually
adjusted presenation settings.

Comment 37

[Charles Manske]

Comment on attachment 51739 [details] [diff] [review]
Allow editor's "documentModified" flag to be writable so we can set it "true" when charset is changed.

Wrong way to do this.

Comment 38

[Yuying Long]

Nominate as nsbeta1

Comment 39

[Charles Manske]

Moving to 0.9.8

Comment 40

[Charles Manske]

changing milestone

Comment 41

[Syd Logan]

Resetting milestone, only nsbeta1+ bugs can have a milestone on them, these are
niminated, but not yet plussed.

Comment 42

[Charles Manske]

changing milestone
Sorry! no time.

Comment 43

[Kasumi]

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

Comment 44

[Charles Manske]

I doubt this will happen for next release.

Comment 45

[Jaime Rodriguez, Jr.]

Changing component to editor composer.

Comment 46

[Samir Gehani]

Composer triage team: nsbeta1_

Comment 47

[Serge Gautherie (:sgautherie)]

Charles,
Are you still working on this ?

Comment 48

[Phoenix]

Incorrect bug, View settings should only affect page representation in browser, not edit it, anyway, for code page changing now "Save and Change Character Encoding" menu item present, so wontfix