Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#6415 closed bug (fixed)

BPictureButton does not draw BPicture

Reported by: laplace Owned by: stippi
Priority: normal Milestone: R1
Component: User Interface Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

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.

Attachments (5)

PrintSetup.png (13.8 KB) - added by laplace 4 years ago.
Print setup dialog
draw_picture.patch (1.6 KB) - added by laplace 4 years ago.
Patch for review
Preview-patch.png (40.8 KB) - added by laplace 4 years ago.
Preview using the patch
Preview-r37833.png (44.8 KB) - added by laplace 4 years ago.
Preview hrev37833
PreviewZoomedIn-r37833.png (68.0 KB) - added by laplace 4 years ago.
Preview zoomed in one time hrev37833

Download all attachments as: .zip

Change History (18)

Changed 4 years ago by laplace

Print setup dialog

comment:1 Changed 4 years ago by laplace

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

Print setup dialog

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

Last edited 4 years ago by laplace (previous) (diff)

Changed 4 years ago by laplace

Patch for review

comment:2 Changed 4 years ago by laplace

  • Has a Patch set

comment:3 follow-up: Changed 4 years ago by 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.

comment:4 Changed 4 years ago by jackburton

  • Resolution set to fixed
  • Status changed from new to closed

I reverted most of the problematic change in hrev37833.

comment:5 follow-up: Changed 4 years ago by 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.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 4 years ago by 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 ?

Changed 4 years ago by laplace

Preview using the patch

Changed 4 years ago by laplace

Preview hrev37833

Changed 4 years ago by laplace

Preview zoomed in one time hrev37833

comment:7 in reply to: ↑ 6 ; follow-up: Changed 4 years ago by 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:
Preview using the patch

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

Preview r37833

And the clipping has changed when zoomed in:

Preview zoomed in one time r37833

comment:8 in reply to: ↑ 7 Changed 4 years ago by 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.

comment:9 follow-up: Changed 4 years ago by 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 ?

comment:10 in reply to: ↑ 3 ; follow-ups: Changed 4 years ago by 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.

comment:11 in reply to: ↑ 9 Changed 4 years ago by 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.

comment:12 in reply to: ↑ 10 Changed 4 years ago by 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.

comment:13 in reply to: ↑ 10 Changed 4 years ago by laplace

Replying to jackburton:

Replying to laplace:
Please commit it.

Committed the patch in hrev37939.

Note: See TracTickets for help on using tickets.