BPictureButton does not draw BPicture

[laplace]

The change in hrev37824 broke rendering of BPictureButtons.
Only the background color seems to be drawn in the BPictureButton.

For example the toolbar buttons in BePDF and the buttons in the print setup dialog are now empty.

[laplace]

Print setup dialog

[laplace]

In the attached screenshot I have added red rectangles where the buttons are.

This dialog is shown for example when you select "Print" from the StyledEdit menu.

The source code where the button is created (but not yet attached the the parent view):
​http://dev.haiku-os.org/browser/haiku/trunk/src/servers/print/ConfigWindow.cpp?rev=37824#L349

The source code where the button is attached to the parent view:
​http://dev.haiku-os.org/browser/haiku/trunk/src/servers/print/ConfigWindow.cpp?rev=37824#L156

[laplace]

Patch for review

[laplace]

I have added a patch (generated with svn diff) that fixes the problem.

Please review.

Also printing to "Preview" printer seems to work again.

[jackburton]

I reverted most of the problematic change in hrev37833.

[laplace]

This fixes the problem of this ticket.

However the test "Test Draw Scaled Picture" is again where it was before hrev37833 and clipping is wrong printing a text to "Preview" printer.

[jackburton]

Replying to laplace:

This fixes the problem of this ticket.

However the test "Test Draw Scaled Picture" is again where it was before hrev37833

Yes, of course. I'm having a deeper look to see how to fix the problem without causing other troubles.

and clipping is wrong printing a text to "Preview" printer.

Is this last symptom related to drawing a scaled picture ?

[laplace]

Preview using the patch

[laplace]

Preview hrev37833

[laplace]

Preview zoomed in one time hrev37833

[laplace]

Replying to jackburton:

Replying to laplace:

This fixes the problem of this ticket.

However the test "Test Draw Scaled Picture" is again where it was before hrev37833

Yes, of course. I'm having a deeper look to see how to fix the problem without causing other troubles.

Oh, I didn't realize that this was intended.

and clipping is wrong printing a text to "Preview" printer.

Is this last symptom related to drawing a scaled picture ?

I don't know.

My observation is that the patch attached to this ticket does not have the clipping problem:

With hrev37833 the text seems to be at the right position but the clipping is wrong:

And the clipping has changed when zoomed in:

[jackburton]

Replying to laplace:

Replying to jackburton:

Replying to laplace:

This fixes the problem of this ticket.

However the test "Test Draw Scaled Picture" is again where it was before hrev37833

Yes, of course. I'm having a deeper look to see how to fix the problem without causing other troubles.

Oh, I didn't realize that this was intended.

Sorry, totally my fault. I should have updated the other ticket.

My observation is that the patch attached to this ticket does not have the clipping problem:

I see. But your patch (the ServerWindow.cpp part) introduces again some code I had to remove (in hrev36933) to fix ticket #6070. I guess that somewhere (in ServerPicture.cpp and/or ServerWindow.cpp) we use View::SetOrigin() or/and View::Origin() when we should really use DrawState::DrawingOrigin(), or vice-versa.

[jackburton]

By the way: currently there is a lot of code duplication in ServerPicture and ServerWindow. For example, this is ServerPicture's draw_string():

tatic void
draw_string(View* view, const char* string, float deltaSpace,
	float deltaNonSpace)
{
	// NOTE: the picture data was recorded with a "set pen location"
	// command inserted before the "draw string" command, so we can
	// use PenLocation()
	BPoint location = view->CurrentState()->PenLocation();

	escapement_delta delta = {deltaSpace, deltaNonSpace };
	view->ConvertToScreenForDrawing(&location);
	view->Window()->GetDrawingEngine()->DrawString(string,
		strlen(string), location, &delta);

	view->ConvertFromScreenForDrawing(&location);
	view->CurrentState()->SetPenLocation(location);
	// the DrawingEngine/Painter does not need to be updated, since this
	// effects only the view->screen coord conversion, which is handled
	// by the view only
}

this is ServerWindow's DRAW_STRING case:

			ViewDrawStringInfo info;
			if (link.Read<ViewDrawStringInfo>(&info) != B_OK
				|| info.stringLength <= 0) {
				break;
			}

			const ssize_t kMaxStackStringSize = 4096;
			char stackString[kMaxStackStringSize];
			char* string = stackString;
			if (info.stringLength >= kMaxStackStringSize) {
				// NOTE: Careful, the + 1 is for termination!
				string = (char*)malloc((info.stringLength + 1 + 63) / 64 * 64);
				if (string == NULL)
					break;
			}

			escapement_delta* delta = NULL;
			if (code == AS_DRAW_STRING_WITH_DELTA) {
				// In this case, info.delta will contain valid values.
				delta = &info.delta;
			}

			if (link.Read(string, info.stringLength) != B_OK) {
				if (string != stackString)
					free(string);
				break;
			}
			// Terminate the string, if nothing else, it's important
			// for the DTRACE call below...
			string[info.stringLength] = '\0';

			DTRACE(("ServerWindow %s: Message AS_DRAW_STRING, View: %s "
				"-> %s\n", Title(), fCurrentView->Name(), string));

			fCurrentView->ConvertToScreenForDrawing(&info.location);
			BPoint penLocation = drawingEngine->DrawString(string,
				info.stringLength, info.location, delta);

			fCurrentView->ConvertFromScreenForDrawing(&penLocation);
			fCurrentView->CurrentState()->SetPenLocation(penLocation);

			if (string != stackString)
				free(string);
			break;

As you can see, the actual conversion and drawing part is the same. Keeping this code duplication will produce lots of bugs, since not always the code is in sync. I'd like to move some of this code into ServerWindow (or View) methods, called from ServerWindow and ServerPicture's global functions. Opinions ?

[jackburton]

Replying to laplace:

I have added a patch (generated with svn diff) that fixes the problem.

Please review.

Also printing to "Preview" printer seems to work again.

Michael, I obviously didn't read your patch carefully. I've re-read it now, and it doesn't introduce the problem I was thinking about. Please commit it.

[laplace]

Replying to jackburton:

Opinions ?

I am not familiar with the code, but in general it is a good coding practice to not repeat yourself for the reasons you mentioned.

[laplace]

Replying to jackburton:

Replying to laplace:

I have added a patch (generated with svn diff) that fixes the problem.

Please review.

Also printing to "Preview" printer seems to work again.

Michael, I obviously didn't read your patch carefully. I've re-read it now, and it doesn't introduce the problem I was thinking about. Please commit it.

OK.

[laplace]

Replying to jackburton:

Replying to laplace: Please commit it.

Committed the patch in hrev37939.