Comment on attachment 729488
Save resolution (dpi) and duplex settings v1

I think this patch generally makes sense.  Looks like this is just two
pieces of data that are configurable in the print dialog, which we don't
currently save between print jobs, but which we easily can by storing
them in about:config like we do for other data. (and that's what the
patch does)

This probably needs review from roc or karlt, the Widget:GTK module
owner / peer.  (Or someone who's actively working on printing, but
unfortunately there have been very few people in that category recently.
:))

Feedback below:

># HG changeset patch
># Parent fd5463b65693b5808cd3cde19f8d4c9d0a2c199b
># User Jan Horak <jho...@redhat.com>
># Bug #539427 - The Print dialog does not remember any settings, unlike other 
>Gnome/GTK apps
># This patch saves print resolution (dpi) and duplex settings (two sided 
>printing). It only
># stores settings only if the settings is available for printer.

Mozilla commit message convention to describe the change, not the bug,
and if possible keep it all on one line (since our RelEng infrastructure
generally only prints the first line).

So -- this should look like:
> Bug 539427: Save print resolution (DPI) and duplex settings between print 
> jobs, if they're available.
or something along those lines.

>diff --git a/widget/gtk2/nsPrintSettingsGTK.cpp 
>b/widget/gtk2/nsPrintSettingsGTK.cpp
[...]
>+NS_IMETHODIMP 
>+nsPrintSettingsGTK::GetResolution(int32_t *aResolution)

Remove space-at-end-of-line after "NS_IMETHODIMP" here (and in all of
your other added "NS_IMETHODIMP" lines in this file.)

>+nsPrintSettingsGTK::GetDuplex(int32_t *aDuplex)
>+{
>+  if (!gtk_print_settings_has_key(mPrintSettings, GTK_PRINT_SETTINGS_DUPLEX))
>+    return NS_ERROR_FAILURE;
>+  *aDuplex = (int16_t) gtk_print_settings_get_duplex(mPrintSettings);
>+  return NS_OK;

Why cast to int16_t here? The outparam is int32_t, not int16_t.

Also: use static_cast, not a c-style cast.

>+NS_IMETHODIMP 
>+nsPrintSettingsGTK::SetDuplex(int32_t aDuplex)
>+{
>+  gtk_print_settings_set_duplex(mPrintSettings, (GtkPrintDuplex) aDuplex);

Use a static_cast instead of C-style cast.

Also: assert that we're in-range for a GtkPrintDuplex before doing this cast. 
e.g.:
 MOZ_ASSERT(aDuplex >= GTK_PRINT_DUPLEX_SIMPLEX &&
            aDuplex <= GTK_PRINT_DUPLEX_VERTICAL,
            "value is out of bounds for GtkPrintDuplex enum");

>@@ -244,16 +244,20 @@ interface nsIPrintSettings : nsISupports
>   attribute long    printPageDelay; /* in milliseconds */
>   
>+  attribute long    resolution;     /* print resolution (dpi) */
>+
>+  attribute long    duplex;         /* print in duplex or simplex mode */
>+  
  ^^ Drop the 2 space characters on this blank line here.

Also: that last comment makes it sound like "duplex" is a boolean value -- but 
it's not. (There are 3 possibilities, not 2.)
Maybe replace that comment with just "/* duplex mode */"?

-- 
You received this bug notification because you are a member of Desktop
Packages, which is subscribed to firefox in Ubuntu.
https://bugs.launchpad.net/bugs/526482

Title:
  Firefox don't save print settings

Status in The Mozilla Firefox Browser:
  New
Status in Mozilla Thunderbird Mail and News:
  Confirmed
Status in “firefox” package in Ubuntu:
  Confirmed

Bug description:
  Binary package hint: firefox

  Environment:
  OS: Ubuntu 9.10  (Karmic Koala)
  Package: firefox-3.5 (3.5.8+build1+nobinonly-0ubuntu0.9.10.1)
  Problem:
  I print from firefox and change print settings, like select destination 
printer, margins, header, footer and so on. In about:config option 
'print.save_print_settings' is set to true. Finally do print. 
  Immediately after previous print I do print again and I suspect to get same 
settings (destination printer, margins, header, footer ...) that I set for last 
print, but unfortunately I get default print settings (destination printer, 
margins, header, footer ...).
  All this is happen in same browser session.
  Problem also reflect on our extension that we developed for Firefox 
(jsPrintSetup https://addons.mozilla.org/en-US/firefox/addon/8966).
  The extension using nsIPrintSettings interface to manage print settings also 
encounter problem with select destination printer. 
  Independently from selected nsIPrintSettings.printerName Firefox always print 
to default for operating system printer.

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/526482/+subscriptions

-- 
Mailing list: https://launchpad.net/~desktop-packages
Post to     : desktop-packages@lists.launchpad.net
Unsubscribe : https://launchpad.net/~desktop-packages
More help   : https://help.launchpad.net/ListHelp

Reply via email to