"print to file" checkbox ignored

RESOLVED FIXED in mozilla1.2beta

Status

()

Core
Printing: Output
RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: Biesinger, Assigned: rods (gone))

Tracking

Trunk
mozilla1.2beta
x86
Windows 98
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

ok, I wanted to try "print to file" today, but found that the checkbox gets ignored.

steps to reproduce:
file|print
check "print to file"
click OK

actual results: printer starts printing
expected: moz asks for filename

w98 build 2002092008

Comment 1

16 years ago
rv:1.2a Gecko/20020907, W2k
(Assignee)

Comment 2

16 years ago
Created attachment 100261 [details] [diff] [review]
patch v1

Apparently, there was some confusion on how to test for this when the
functionality was originally added.

This does two things:
1) When the it returns from the Print Dialog it checks to see if the
PD_PRINTTOFILE flags is set then then set the the info into the PrintSettings
2) IF we are printing to a file, but not a file driver, then check to see if
the file name is the special "FILE:" (as per the MS documentation)
(Assignee)

Updated

16 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.2beta
+    PRUnichar* toFileName;
+    aPS->GetToFileName(&toFileName);

suggestion: use an nsXPIDLString, which needs not be freed.
as in:
nsXPIDLString toFileName;
aPS->GetToFileName(getter_Copies(toFileName));
(also see http://www.mozilla.org/projects/xpcom/string-quickref.html)

+        toFile  = _tcscmp((char*)NS_ConvertUCS2toUTF8(toFileName).get(),
_T("FILE:")) == 0;

you are converting to UTF8 anyway. why bother with _tcscmp and _T then, instead
of using strcmp and "FILE:"?

as for AssignWithConversion, that's deprecated.
I would replace this code:
+        nsString filePrefix;
+        filePrefix.AssignWithConversion(fileName);
+        aPrintSettings->SetToFileName(filePrefix.get());
with this:
aPrintSettings->SetToFileName(NS_ConvertUTF8toUCS2(fileName).get());
(or ASCII instead of UTF8, I don't know which charset fileName is in)
(also see http://www.mozilla.org/projects/xpcom/string-guide.html#Guidelines)
(Assignee)

Comment 4

16 years ago
Created attachment 100271 [details] [diff] [review]
patch v2

Christian, thanks for the help, I didn't find a thing about nsXPIDLString in
the following reference:
(also see http://www.mozilla.org/projects/xpcom/string-quickref.html)

Here is a better patch with Christian's suggestions.

(Everytime I go to the effort to learn everything about the string classes,
they change, get deprecated, etc. In fact, I spend more time trying to figure
out the secret ninja code to our string classes than I do anything else)
Attachment #100261 - Attachment is obsolete: true
Comment on attachment 100271 [details] [diff] [review]
patch v2

ok, really sorry about missing that earlier, but this could be even more
improved:
+	 if (nsCRT::strcmp(NS_LossyConvertUCS2toASCII(toFileName).get(),
"FILE:") == 0) {
if written as:
if (toFileName.Equals(NS_LITERAL_STRING("FILE:")))

because the FILE: UCS2 conversion will be done at compile time, that will be
faster.
(Assignee)

Comment 6

16 years ago
Ok, I will check it in with that change, I am not going to put up aonther patch,
thanks for the review.
note, I do not know the printing stuff well enough to put an r= on that patch... 

however, I applied&tested the patch, and it does work well. thanks for the patch!

Comment 8

16 years ago
Comment on attachment 100271 [details] [diff] [review]
patch v2

are you sure the devnames->wOutputOffset is a UTF8 string? (I'm just surprised
its not a native string)
sr=alecf if you know this..

If you find out its native, then my recommendation would be to change
SetToFileName to take a native "const nsACString&" and use
nsIFile::InitWithNativePath to let nsLocalFile do the conversion for you.
(assuming that doesn't mean you have to change 100 consumers)
Attachment #100271 - Flags: superreview+

Comment 9

16 years ago
Comment on attachment 100271 [details] [diff] [review]
patch v2

rods sez it is actually a native string, not UTF8
Attachment #100271 - Flags: superreview+ → needs-work+
(Assignee)

Comment 10

16 years ago
Created attachment 100389 [details] [diff] [review]
patch v3

With Alec's suggestions
Attachment #100271 - Attachment is obsolete: true
(Assignee)

Comment 11

16 years ago
Created attachment 100390 [details] [diff] [review]
patch v4

Forgot extra comments explaining why we do "FILE:"
Attachment #100389 - Attachment is obsolete: true
Comment on attachment 100390 [details] [diff] [review]
patch v4

you still have this line:
+	 if (nsCRT::strcmp(NS_LossyConvertUCS2toASCII(toFileName).get(),
"FILE:") == 0) {
(Assignee)

Comment 13

16 years ago
Created attachment 100395 [details] [diff] [review]
patch v5

Bah! I should have made an entirely new patch. Here is what I have in my tree.
Attachment #100390 - Attachment is obsolete: true

Updated

16 years ago
Attachment #100395 - Flags: superreview+

Comment 14

16 years ago
Comment on attachment 100395 [details] [diff] [review]
patch v5

looks great. sr=alecf
Comment on attachment 100395 [details] [diff] [review]
patch v5

um wait a second...
+	 NS_ASSERTION(strcmp(fileName, "FILE:") == 0, "FileName must be
`FILE:`");
+	 aPrintSettings->SetToFileName(NS_ConvertASCIItoUCS2(fileName).get());

so if fileName must be "FILE:", why not use NS_LITERAL_STRING("FILE:") instead
of the NS_Convert* function?
(Assignee)

Comment 16

16 years ago
I guess because maybe I don't trust their documentation, this way we can be
notified if it isn't FILE: in debug builds, and we will use whatever they think
is the right thing.

Comment 17

16 years ago
Comment on attachment 100395 [details] [diff] [review]
patch v5

r=dcone
Attachment #100395 - Flags: review+
(Assignee)

Comment 18

16 years ago
fixed
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.