save doesn't work; always prompted for file name

VERIFIED FIXED in mozilla1.3alpha

Status

--
critical
VERIFIED FIXED
16 years ago
14 years ago

People

(Reporter: Brade, Assigned: cmanske)

Tracking

({regression})

Trunk
mozilla1.3alpha
regression

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: EDITORBASE+)

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

16 years ago
Save doesn't work; I am prompted for a filename with each accel-S.

1) create new composer window
2) choose Save
3) pick filename, location, click Save button
4) type some text
5) choose Save

expectation: file saved to previously picked location
actual result: prompted to pick filename and location
(Reporter)

Updated

16 years ago
Whiteboard: EDITORBASE
(Assignee)

Comment 1

16 years ago
I updated my tree early this morning and I don't see this problem (Windows2K)

Comment 2

16 years ago
I just filed a probably related bug, bug 181153.
Keywords: nsbeta1 → nsbeta1+
(Assignee)

Updated

16 years ago
Blocks: 181153
(Assignee)

Comment 3

16 years ago
the new URL is being set on the docShell, but not on the DOM document.
That is supposed to happen in nsEditingShell::OnLocationChange, but we forgot
to implement that in the conversion from editorShell!
No longer blocks: 181153
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.3alpha
(Assignee)

Updated

16 years ago
Blocks: 181153
(Assignee)

Comment 4

16 years ago
Created attachment 106955 [details] [diff] [review]
fix v1
(Assignee)

Updated

16 years ago
Attachment #106955 - Flags: superreview?(sfraser)
Attachment #106955 - Flags: review?(akkana)
(Assignee)

Comment 5

16 years ago
Created attachment 106966 [details] [diff] [review]
fix v2

We decided it was not a good idea for nsEditingSession to know anything about
the HTML "base" tag, thus I added a composer command to set the document 
and base urls.
Attachment #106955 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #106966 - Flags: superreview?(sfraser)
Attachment #106966 - Flags: review?(akkana)
(Assignee)

Comment 6

16 years ago
Created attachment 106969 [details] [diff] [review]
fix v3

oops! I deleted a couple of bogus lines from last patch that didn't affect 
behavior
Attachment #106966 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #106969 - Flags: superreview?(sfraser)
Attachment #106969 - Flags: review?(akkana)

Comment 7

16 years ago
With fix v2, it's still not rememembering the filename for me, either on new
files, or existing files where I "save as" to a new name.

Updated

16 years ago
Attachment #106955 - Flags: superreview?(sfraser) → superreview-

Updated

16 years ago
Attachment #106966 - Flags: superreview?(sfraser) → superreview-
(Assignee)

Updated

16 years ago
Whiteboard: EDITORBASE → EDITORBASE+
(Assignee)

Comment 8

16 years ago
Created attachment 107048 [details] [diff] [review]
fix v4

Previous patch was missing command registration line
Attachment #106969 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #106969 - Flags: superreview?(sfraser)
(Assignee)

Comment 9

16 years ago
Created attachment 107094 [details] [diff] [review]
fix v5

Now that *everyone* has had some input, we decided to do a document state 
observer notification instead of a command to change the document url.
Note that the observer "obs_documentLocationChanged" will give you the new 
URL as an nsIURI object using getCommandState().
But I decided that letting the nsIHTMLEditor command "UpdateBaseURL()" get the 

uri from the current document made better sense for that interface.
This allowed the Composer JS code to be simpler.
Attachment #107048 - Attachment is obsolete: true

Comment 10

16 years ago
This patch, again, doesn't work for me.  I save to a file, it saves but doesn't
update the titlebar and doesn't remember its name.

Some questions on the code:
- Since you're already wrapping in a try {}, do you need the if (editor)?

-  "Set the documents title" should have an apostrophe (possessive).

- You do GetElementsByTagName for "base", then NS_ENSURE_SUCCESS(rv, rv);
Is that right?  I thought base was optional, and we would just ignore it if not
present? Then shortly after that you have an "If no base tag" clause.  I'm
confused.  If there's no base tag, then haven't we already returned an error?
(Assignee)

Updated

16 years ago
Whiteboard: EDITORBASE+ → EDITORBASE+[FIX IN HAND] need r=,sr=
(Assignee)

Comment 11

16 years ago
1. Doh! It doesn't work because of small error in editor.js: 
    "updateBaseURL(uri)" should be "updateBaseURL()"
2. Yes, I can drop the test for "if (editor) " inside the try, but then I 
probably shouldn't dump the exception, since we will hit this often.
3. If no nodelist is returned for "base" tags, rv is not set to an error,
so that check is for other errors. Thus we still have to check for nodelist
(which does exist even if empty) and get it's count, which will be 0 if no
base element exists.
New patch comming...

Comment 12

16 years ago
> Yes, I can drop the test for "if (editor) " inside the try, but then I 
> probably shouldn't dump the exception, since we will hit this often.

You can test for that specific exception (NullPointerException?) and not dump
that one, but dump others.

But that's as much code as just testing for null in the first place, so maybe
it's not worth it ...
(Assignee)

Comment 13

16 years ago
Created attachment 107171 [details] [diff] [review]
fix v6
Attachment #107094 - Attachment is obsolete: true

Comment 14

16 years ago
Comment on attachment 107171 [details] [diff] [review]
fix v6

We talked on IRC and decided Charley should go back to the null test he had
before -- apparently it's more efficient than throwing an exception.

>+  nsresult rv = domDoc->GetElementsByTagName(NS_LITERAL_STRING("base"), getter_AddRefs(nodeList));

Long line, please trim (I'd suggest breaking after the comma).

Fix those, and r=akkana.
Attachment #107171 - Flags: review+
(Assignee)

Comment 15

16 years ago
Comment on attachment 107171 [details] [diff] [review]
fix v6

The code in editor.js will be restored to previous version:
	// Ignore this when editor doesn't exist,
	//   which happens once when page load starts
	if (editor)
	  try {
	    editor.updateBaseURL();
	  } catch(e) { dump (e); }
	break;
Attachment #107171 - Flags: superreview?(alecf)

Comment 16

16 years ago
Comment on attachment 107171 [details] [diff] [review]
fix v6

sr=sfraser
Attachment #107171 - Flags: superreview?(alecf) → superreview+
(Assignee)

Comment 17

16 years ago
checked into 1.3a trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Whiteboard: EDITORBASE+[FIX IN HAND] need r=,sr= → EDITORBASE+

Comment 18

16 years ago
that if() above is redundant:

if (editor) 
  try {
    editor.foo()
  } catch (e) {}

you should just catch this with the try{} block, and not do an if() beforehand.

Updated

16 years ago
Attachment #106955 - Flags: review?(akkana) → review-

Updated

16 years ago
Attachment #106966 - Flags: review?(akkana) → review-

Updated

16 years ago
Attachment #106969 - Flags: review?(akkana) → review-
(Reporter)

Comment 19

16 years ago
*** Bug 185988 has been marked as a duplicate of this bug. ***
no longer happening (whew).
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.