290213 - Printer select dialog does not always match printers to correct descriptions.

Status: RESOLVED FIXED

(Core Graveyard :: GFX: OS/2, defect)

Description

[Steven H. Levine]

User-Agent:       Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.8b) Gecko/20050301
Build Identifier: Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.8b) Gecko/20050301

The code in mozilla\gfx\src\os2\nsDeviceContextSpecOS2.cpp uses both printer
indexes and logical printer numbers to specify a specific printer.  These
elements are not always used in the correct context.  The result is that the
printer queue names and descriptions are incorrectly matched depending on the
order the printers are defined.

Reproducible: Always

Steps to Reproduce:
1.  Request print
2.  Print dialog displays incorrect printer selection list
3.

Comment 1

[Steven H. Levine]

Attachment: Patch nsDeviceContextSpecOS2.cpp to match names and descriptions

Comment 2

[Steven H. Levine]

Comment on attachment 180610 [details] [diff] [review]
Patch nsDeviceContextSpecOS2.cpp to match names and descriptions

I have obsoleted this version of the patch.  It will not correct the reported
probem because I did not understand the intent of GetIndex

Comment 3

[Steven H. Levine]

Attachment: 2nd try at patching this to sync names and descriptions

Comment 4

[Paul Smedley]

I'm also seeing this on a system with multiple printers defined.  Status can be 
changed to Confirmed.

Comment 5

[u29221]

when you make a diff, you should use cvs diff -u which will give context to the
diff.

Thanks!

Comment 6

[Steven H. Levine]

Attachment: Same fix as above with -u this time.

Sorry about the missing -u.  The first attachment was -u, but was also the
wrong fix. :-(

Comment 7

[Peter Weilbacher]

The patch looks fine to me but as I cannot build on OS/2 currently, I cannot
really verify that it work...

(I think this really belongs into Core and GFX OS/2.)

Comment 8

[Mike Kaply [:mkaply]]

Can someone give me a straightforward way to reproduce this - I have no printers
on my OS/2 machien right now.

Comment 9

[Steven H. Levine]

Attachment: Screenshoot of failing dialog

Here's a screen shot showing the problem.  You don't need installed printers. 
Just install a couple of printer objects.  To make the problem come and go,
just change the default printer.  The problem only shows up if the default is
not the first printer enumerated ::SplEnumQueue()

Comment 10

[Peter Weilbacher]

It's getting more and more interesting: what did you do to have the first OKI
queue identify itself as ONMI instead of OMNI?

Comment 11

[Peter Weilbacher]

Attachment: Working patch

Finally got the chance to test this, it was indeed easy to reproduce. Steven's
patch was almost there, but to get access to GetIndex it needs to be moved from
private to public and add the complete class in front. I confirmed that it
solves the problem.

Comment 12

[Rich Walsh]

Comment on attachment 184953 [details] [diff] [review]
Working patch

At first I wasn't sure about making a private method public, but given the
nature of PRINTDLG, it seems perfectly reasonable.  Review+

Comment 13

[Mike Kaply [:mkaply]]

Comment on attachment 184953 [details] [diff] [review]
Working patch

sr=mkaply (OS/2 specific)

Comment 14

[Peter Weilbacher]

Comment on attachment 184953 [details] [diff] [review]
Working patch

This is OS/2 only, fixes a rather obvious bug, and should not have any impact
on stability. Would be great to get it into the next releases.

Comment 15

[Peter Weilbacher]

Please hold any checkins. Now that all needed reviews are there I by chance
discovered that the two displayed printer info lines match but the printer that
is actually printed to might still be a different one. Not sure yet what causes it.

Comment 16

[Peter Weilbacher]

I spent some time on the on and off over the last week but couldn't make any
sense of the behavior. I found out that in
GlobalPrinters::InitializeGlobalPrinters the calls GetPrintDriver(i) and
GetDriverType(i) should also have the i replaced by GetIndex(i) as those
function access mPQBuf[] directly without calling GetIndex themselves just as
Steven discovered for GetPrinter.

But still, depending on how I arrange my printer queues (that means which of the
printers I make default) it depends if I get the data sent to the right printer
or not.

Well, I will investigate further, this is a really annoying problem!

Comment 17

[Peter Weilbacher]

Attachment: Correctly working patch + cleanup

I think now I have the correct solution. I finally realized that we don't need
to correct the printer name that is shown but the driver name and type, so we
don't need an extra GetIndex() call but the reverse. As there is no such
function I just invented two new functions GetPrintDriver2() and
GetDriverType2() which do exactly the same as the corresponding functions
without 2 in their names apart from not calling GetIndex() on the argument.
(This also removes Rich's reservation from comment 12, GetIndex() does not need
to be made public any more.)

Additionally, I made a small cleanup of the PRINTDLG class which mainly renames
every variable that is used as index of mPQBuf[] to "index" and every variable
that is converted to this "index" by calling GetIndex() to "numPrinter". This
makes it much easier to see which function needs to be called in which way.

Then I also cleaned up a bit of whitespace to make PRINTDLG more consistent in
that respect. If you need a non-whitespace cleaned version for review, please
complain!

I tested this patch now with five different queues with four different drivers
and it works on my machine, no matter which queue I make default.

Comment 18

[Mike Kaply [:mkaply]]

instead of using "2" can we make those function names more obvious? It's still
not clear to me why we need two function to do this. I probably just don't
understand the code.

Comment 19

[Peter Weilbacher]

Hmm, I could name them GetPrintDriverIndex and GetDriverTypeIndex or something
like that.

The problem is that there are two counters in PRINTDLG. 1. Some functions take a
counter I called "numPrinter" which is just running from 0 to n-1 where n is the
number of printer queues on the system. I believe that this is in the order they
are listed in the INI file. 2. Mozilla wants the order to be such that the
default printer queue is first, so that this is the counter I called "index".
GetIndex converts the order from "numPrinter" to "index".

Now "index" is always needed when accessing the mPQBuf[] array (I didn't fully
understand why), so the functions GetPrintDriver/GetDriverType use GetIndex
internally as they are called with "numPrinter". The "i" index in the
GlobalPrinters::InitializeGlobalPrinters function seems to be ordered as index,
but I still wanted to get the driver and type in there. So either we need one
other function that could convert back from "index" to "numPrinter" before
calling those functions or we need similar functions but without the internal
use of GetIndex. I went with the second option here.

But as you notice, I don't understand the code well myself, so I could be wrong.
But I tested this quite a bit and for me it now works. I could make a
gfx_os2.dll with those changes and distribute for wider testing if you want.

Comment 20

[Rich Walsh]

Comment on attachment 192031 [details] [diff] [review]
Correctly working patch + cleanup

Sorry, but this patch doesn't address the underlying problem.  If the original
code were slightly more clever, GetIndex() would be totally unnecessary.

IIUC, the indices used by mozapps to identify a printer don't match those used
to access the corresponding entries in mPQBuf[].  Instead of using GetIndex()
to remap them, why not just build mPQBuf[] so they match?

If a mozapp expects printer #0 to be the default, then the code in PRINTDLG() &
RefreshPrintQueue() should leave slot #0 in mPQBuf empty until it encounters
the default printer.  Putting the default where it "belongs" would eliminate
the mismatch, GetIndex(), and mDefaultQueue (which would always be 0).

Comment 21

[Rich Walsh]

Attachment: eliminates GetIndex()

After the existing code constructs mPQBuf[], this patch swaps the entry for the
default printer with the entry at index 0.  This eliminates the need for
GetIndex() & mDefaultQueue.  I've retained Peter's whitespace changes and added
a few of my own.

FYI... I've compiled this but have no way to test it.

Comment 22

[Peter Weilbacher]

Comment on attachment 192909 [details] [diff] [review]
eliminates GetIndex()

Good idea, I was already content having solved the problem at hand...

While we are at it, should we not also get rid of GetDefaultPrinter()
completely, now that it is useless? It is called twice in GlobalPrinters where
one could just replace defaultQueue with 0 now.

Anyway, will test it tonight.

Comment 23

[Rich Walsh]

Attachment: eliminates GetIndex() & GetDefaultPrinter()

Comment 24

[Peter Weilbacher]

Comment on attachment 192964 [details] [diff] [review]
eliminates GetIndex() & GetDefaultPrinter()

This newest patch works fine here.

I would have preferred that in every instance the index of mPQBuf has the same
name (printerNdx). The lines are changed anyway because of whitespace, but
that's up to you.

Comment 25

[Mike Kaply [:mkaply]]

Comment on attachment 192964 [details] [diff] [review]
eliminates GetIndex() & GetDefaultPrinter()

sr=mkaply
a=mkaply for 1.8b4 (OS/2 only)

Comment 26

[Mike Kaply [:mkaply]]

Thanks!