120211 - Hook-up a dialog which warns users of the broken Xfree86 Xprt

Status: RESOLVED FIXED

(Core Graveyard :: Printing: Xprint, enhancement)

Description

[Roland Mainz]

The Xprt binaries shipped with current versions of Xfree86 is broken in many
ways.
We should look at the "vendor string"/"vendor release number" (see xdpyinfo
output) and warn the user with a dialog if he/she uses such a "thing"...

Comment 1

[Roland Mainz]

Just for the log:
Known working versions of Xprt (tested):
- /usr/openwin/bin/Xprt in Solaris >= 2.7
- Xprt build from X11R6.5.1 sources
- Xprt binary shipped with various versions of HP-UX

Comment 2

[Roland Mainz]

Swapping Owner<-->QA ...

Comment 3

[Masaki Katakai]

Not sure that showing dialog is the best way because users
who want to use Xprt will set XPSERVERLIST environment
manually, so users can understand the capability of installed Xprt.
but anyway, we need to document such limitation.

Comment 4

[Roland Mainz]

I just saw that fire-up-a-dialog-approach in the MathML code.

Background:
MathML only works properly if the MathML fonts from Wolfram are installed on the
system (e.g. in the font path on X11 systems). rbs (the MathML-in-Mozilla
author) decided to hook-up such a dialog in this case to get rid of the huge
amounts of "MathML renders only 'rubbish'"-bugs.

I think we have a similar situation here. Xprt build from Xfree86 sources is
broken... very broken - and those binaries are very widespread (I would say
nearly every Linux system shipps with this broken crap).
I'd like to avoid this issue at the earliest possible time - and that's AFAIK on
the user side (Linux users are usually their own admins... :-)

I agree that this is an item for the release notes, but I'd like to catch those
people who simply do not read those documents completely, too...

Comment 5

[Masaki Katakai]

Thanks for detail, I understand now.

Btw, MathML will be built by default?

Also we need to take care localization messages.

Comment 6

[Roland Mainz]

Masaki Katakai wrote:
> Btw, MathML will be built by default?

Not _yet_. But there is bug 109826 ("Turn on MathML in default builds") and I am
trying to push this a little (well, Xprint is the only module which can print
MathML properly... :-)), too. Most tinderboxen are already running with
--enable-mathml ...

> Also we need to take care localization messages.

I know... :)
I'll take a look at the MathML-needs-fonts-dialog implementation this week and
try to "port" the code over to Xprint module...

Comment 7

[Roland Mainz]

for reference:
Bonsai URL of the MathML warning dialog checkin:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&branch=&branchtype=match&dir=&file=&filetype=match&who=rbs%25maths.uq.edu.au&whotype=match&sortby=Who&hours=2&date=explicit&mindate=01%2F10%2F2002+18%3A00%3A00&maxdate=01%2F10%2F2002+19%3A00%3A00&cvsroot=%2Fcvsroot

Comment 8

[Roland Mainz]

xdpyinfo "Signature" from the "evil" version of Xprt:
-- snip --
% xdpyinfo -display :5
name of display:    :5.0
version number:    11.0
vendor string:    The XFree86 Project, Inc
vendor release number:    40100000
-- snip --

Comment 9

[Roland Mainz]

Attachment: Patch for 2002-01-19-08-trunk

Comment 10

[Roland Mainz]

Requesting r=/sr= ...

Comment 11

[hacker formerly known as [e-mail]]

r=cls on the Makefile.in change.

Comment 12

[Roland Mainz]

Attachment: New patch for 2002-01-23-08-trunk to match r=timeless

Comment 13

[timeless]

r=timeless if jag says that this NS_LITERAL_STRING usage is ok. jag, would you 
like to sr?

Comment 14

[jag (Peter Annema)]

Comment on attachment 66431 [details] [diff] [review]
New patch for 2002-01-23-08-trunk to match r=timeless

>diff -N -r -u original/mozilla/gfx/src/xprint/nsXPrintContext.cpp mozilla/gfx/src/xprint/nsXPrintContext.cpp
>--- original/mozilla/gfx/src/xprint/nsXPrintContext.cpp	Wed Jan 16 00:30:14 2002
>+++ mozilla/gfx/src/xprint/nsXPrintContext.cpp	Fri Jan 25 09:57:27 2002
>@@ -147,6 +156,69 @@
> 
> NS_IMPL_ISUPPORTS1(nsXPrintContext, nsIDrawingSurfaceXlib)
> 
>+/* Hardcoded fallback message */
>+#define BROKEN_XPRT_MSG \
>+    "A broken version of the X print server (Xprt) has been detected. " \
>+    "Note that printing using this Xprt server may not work properly. " \
>+    "Please contact the server vendor for a fixed version."
>+
>+static nsresult
>+AlertBrokenXprt(Display *pdpy)
>+{
>+  PRUnichar *msg   = nsnull,
>+            *title = nsnull;

Make these nsXPIDLStrings (you're leaking these currently) and move them closer
to where you initialize them.

>+  nsresult   rv = NS_OK;
>+
>+  /* Check for broken Xprt
>+   * Xfree86 Xprt is the only known broken version of Xprt (see bug 120211
>+   * for a list of tested&working Xprt servers) ...
>+   * FixMe: We should look at XVendorRelease(), too - but there is no feedback
>+   * from XFree86.org when this issue will be fixed... ;-(
>+   */
>+  if (!(strstr(XServerVendor(pdpy), "XFree86") /*&& XVendorRelease(pdpy) < 45000000L*/))
>+    return NS_OK;
>+  
>+  /* Bad version found... log the issue and show the dialog and warn the user... */
>+  PR_LOG(nsXPrintContextLM, PR_LOG_DEBUG, 
>+         ("nsXPrintContext::AlertBrokenXprt: vendor: '%s', release=%ld\n", 
>+          XServerVendor(pdpy), (long)XVendorRelease(pdpy)));
>+
>+  /* Dialog disabled ? */
>+  if (PR_GetEnv("MOZILLA_XPRINT_DISABLE_BROKEN_XFREE86_WARNING") != nsnull)
>+    return NS_OK;
>+
>+  static NS_DEFINE_CID(kCStringBundleServiceCID,  NS_STRINGBUNDLESERVICE_CID);

Use NS_STRINGBUNDLE_CONTRACTID instead of this CID.

>+  nsCOMPtr<nsIStringBundleService> stringBundleService = do_GetService(kCStringBundleServiceCID);
>+  nsCOMPtr<nsIStringBundle> myStringBundle;

You could move this variable inside the next |if| and limit its scope some
more.

>+  if (stringBundleService) { 
>+    rv = stringBundleService->CreateBundle(NS_ERROR_GFX_PRINTER_BUNDLE_URL, getter_AddRefs(myStringBundle));    
>+    if (NS_SUCCEEDED(rv)) {     
>+      myStringBundle->GetStringFromName(NS_LITERAL_STRING("print_error_dialog_title").get(),     &title);
>+      myStringBundle->GetStringFromName(NS_LITERAL_STRING("print_xprint_broken_xprt_msg").get(), &msg);
>+    }
>+  }  
>+
>+  nsCOMPtr<nsIWindowWatcher> wwatch = do_GetService("@mozilla.org/embedcomp/window-watcher;1");
>+  if (wwatch) {
>+    nsCOMPtr<nsIDOMWindow> active;
>+    wwatch->GetActiveWindow(getter_AddRefs(active));
>+
>+    nsCOMPtr<nsIDOMWindowInternal> parent = do_QueryInterface(active);
>+    
>+    if (parent) {
>+      nsCOMPtr<nsIPrompt> dialog; 
>+      parent->GetPrompter(getter_AddRefs(dialog)); 
>+      if (dialog) {
>+        dialog->Alert(title, 
>+                      msg?msg:(NS_LITERAL_STRING(BROKEN_XPRT_MSG).get()));
>+      }
>+    }
>+  }
>+
>+  return rv;  

Trailing spaces after this return. Also some elsewhere in this code, do a s/
*$// on the
lines you've added :-)

Also, what constitutes success/failure of this method? Currently you set rv
from the code
that tries to get the strings from the stringbundles, but don't really seem to
mind if
that fails, where you'll display an empty title in that case. If it's okay for
the
stringbundle code to "fail", then please use a different variable for that or
reset rv
to NS_OK before the window watcher code.

Is this fail-safe of |msg?msg:(NS_LITERAL_STRING(BROKEN_XPRT_MSG).get())|
really necessary?
If you just add that file to the tree and make sure it's installed, you can
drop the
fail-safe, bail early if the string bundle code fails, and get rid of that
hardcoded
English.

You may also want to make sure when this is checked in that the release note
people (both
Mozilla and Netscape) know about the ENV var to disable this alert.

Comment 15

[jag (Peter Annema)]

*sigh* I'll have to work on wrapping my text in patch manager :-)

And I just saw that printing.properties is already in the tree, so why didn't
you just add print_xprint_broken_xprt_msg to that file?

Comment 16

[Roland Mainz]

Attachment: Better patch for 2002-01-23-08-trunk per jag's comments

Comment 17

[Roland Mainz]

jag wrote:
> >+  PRUnichar *msg   = nsnull,
> >+            *title = nsnull;
> 
> Make these nsXPIDLStrings (you're leaking these currently) and move them
closer
> to where you initialize them.

Fixed.

[snip]
> >+  static NS_DEFINE_CID(kCStringBundleServiceCID, 
NS_STRINGBUNDLESERVICE_CID);
> 
> Use NS_STRINGBUNDLE_CONTRACTID instead of this CID.
> 
> >+  nsCOMPtr<nsIStringBundleService> stringBundleService =
do_GetService(kCStringBundleServiceCID);
> >+  nsCOMPtr<nsIStringBundle> myStringBundle;
> 
> You could move this variable inside the next |if| and limit its scope some
> more.

Fixed.

> >+  if (stringBundleService) { 
> >+    rv = stringBundleService->CreateBundle(NS_ERROR_GFX_PRINTER_BUNDLE_URL,
getter_AddRefs(myStringBundle));    
> >+    if (NS_SUCCEEDED(rv)) {     
> >+     
myStringBundle->GetStringFromName(NS_LITERAL_STRING("print_error_dialog_title").get(),    
&title);
> >+     
myStringBundle->GetStringFromName(NS_LITERAL_STRING("print_xprint_broken_xprt_msg").get(),
&msg);
> >+    }
> >+  }  
> >+
> >+  nsCOMPtr<nsIWindowWatcher> wwatch =
do_GetService("@mozilla.org/embedcomp/window-watcher;1");
> >+  if (wwatch) {
> >+    nsCOMPtr<nsIDOMWindow> active;
> >+    wwatch->GetActiveWindow(getter_AddRefs(active));
> >+
> >+    nsCOMPtr<nsIDOMWindowInternal> parent = do_QueryInterface(active);
> >+    
> >+    if (parent) {
> >+      nsCOMPtr<nsIPrompt> dialog; 
> >+      parent->GetPrompter(getter_AddRefs(dialog)); 
> >+      if (dialog) {
> >+        dialog->Alert(title, 
> >+                      msg?msg:(NS_LITERAL_STRING(BROKEN_XPRT_MSG).get()));
> >+      }
> >+    }
> >+  }
> >+
> >+  return rv;  
> 
> Trailing spaces after this return. Also some elsewhere in this code, do a s/
> *$// on the
> lines you've added :-)

Fixed.

> Also, what constitutes success/failure of this method? Currently you set rv
> from the code
> that tries to get the strings from the stringbundles, but don't really seem to
> mind if
> that fails,

Because I have the "hardcoded" fallback message.

> where you'll display an empty title in that case. If it's okay for
> the
> stringbundle code to "fail", then please use a different variable for that or
> reset rv
> to NS_OK before the window watcher code.

Fixed. I am resetting |rv| to NS_OK now...

> Is this fail-safe of |msg?msg:(NS_LITERAL_STRING(BROKEN_XPRT_MSG).get())|
> really necessary?

Well, _yes_. I'd like to be on the "safe" side - always. I'd like to avoid
getting complaints for that broken monster if possible...

[snip]
> You may also want to make sure when this is checked in that the release note
> people (both
> Mozilla and Netscape) know about the ENV var to disable this alert.

Good idea. I'll add the "relnote" keyword and email asa once we're done with
this patch...

-------
 
> And I just saw that printing.properties is already in the tree, so why didn't
> you just add print_xprint_broken_xprt_msg to that file?

Well... I forgot to run gdiff over layout/, too. Sorry...

Comment 18

[jag (Peter Annema)]

I think it is safe to trust that printer.properties and the keys in it will be
available, and if not, you've got bigger things to worry about than broken Xprt
support. Just put that text in printer.properties and get rid of the fallback.

Comment 19

[jag (Peter Annema)]

I should add that putting this in printer.properties (regardless of whether you
keep a backup copy hardcoded) also makes it clear to the translation folks that
there is text for them to translate. If you leave it out, it will most likely
not be discovered and therefore not be translated.

Comment 20

[Roland Mainz]

Attachment: New patch for 2002-01-23-08-trunk

Comment 21

[Roland Mainz]

Comment on attachment 66842 [details] [diff] [review]
New patch for 2002-01-23-08-trunk

minor mistake, I'll file a new patch

Comment 22

[Roland Mainz]

Comment on attachment 66842 [details] [diff] [review]
New patch for 2002-01-23-08-trunk

minor mistake, I'll file a new patch

Comment 23

[Roland Mainz]

Attachment: New patch for 2002-01-23-08-trunk (the correct one)

Comment 24

[jag (Peter Annema)]

Comment on attachment 66843 [details] [diff] [review]
New patch for 2002-01-23-08-trunk (the correct one)

sr=jag

Comment 25

[Roland Mainz]

Fix checked in, marking bug as FIXED.

Comment 26

[Roland Mainz]

Fixed, really!