[haiku-commits] Re: haiku: hrev47085 - src/kits/interface

  • From: Ingo Weinhold <ingo_weinhold@xxxxxx>
  • To: haiku-commits@xxxxxxxxxxxxx
  • Date: Thu, 03 Apr 2014 21:09:10 +0200

On 01.04.2014 23:25, jscipione@xxxxxxxxx wrote:
  This reverts a portion of hrev46580 concerning placement of a label on a 
button.
  The label was draw too low on the button in some cases, notably in Keymap.
  Restoring the old code for the icon-less case fixes the problem.

Can you elaborate on what you mean by "too low"? It is obvious that a "q" or a "y" will not appear vertically centered, since they use only part of the ascent but possibly the full descent.

diff --git a/src/kits/interface/ControlLook.cpp 
b/src/kits/interface/ControlLook.cpp
index fc5b68c..631efa2 100644
--- a/src/kits/interface/ControlLook.cpp
+++ b/src/kits/interface/ControlLook.cpp
@@ -1914,15 +1914,17 @@ BControlLook::DrawLabel(BView* view, const char* label, 
const BBitmap* icon,

        font_height fontHeight;
        font.GetHeight(&fontHeight);
-       float textHeight = ceilf(fontHeight.ascent) + ceilf(fontHeight.descent);
+       float textHeight = ceilf(fontHeight.ascent + fontHeight.descent);

Strictly speaking, this is incorrect. We want to compute the maximum number of pixels covered (NB this code is meant for pixel aligned drawing), which the code no longer does in all cases. E.g. ascent 10.2 and descent 4.2 now result in a text height of 15 while actually 16 pixels are covered.

@@ -1930,12 +1932,45 @@ BControlLook::DrawLabel(BView* view, const char* label, 
const BBitmap* icon,
                view->SetDrawingMode(B_OP_OVER);
                view->DrawBitmap(icon, location);
                view->SetDrawingMode(oldMode);
+
+               // set the location to draw the label
+               location = BPoint(alignedRect.left + textOffset,
+                       alignedRect.top + ceilf(fontHeight.ascent));
+               if (textHeight < height)
+                       location.y += ceilf((height - textHeight) / 2);
+       } else {
+               switch (alignment.horizontal) {
+                       case B_ALIGN_LEFT:
+                       default:
+                               location.x = rect.left;
+                               break;
+
+                       case B_ALIGN_RIGHT:
+                               location.x = rect.right - width;
+                               break;
+
+                       case B_ALIGN_CENTER:
+                               location.x = (rect.left + rect.right - width) / 
2.0f;

This may result in a non-integer -- I assume that the app server rounds in some way in this case. Also note that the result is incorrect. E.g. rect.left == 0, rect.right == 9, and width == 2 should result in location 4. Here the result is 3.5, though.

+                               break;
+               }
+
+               switch (alignment.vertical) {
+                       case B_ALIGN_TOP:
+                               location.y = rect.top + 
ceilf(fontHeight.ascent);
+                               break;
+
+                       case B_ALIGN_MIDDLE:
+                       default:
+                               location.y = floorf((rect.top + rect.bottom - 
height)
+                                       / 2.0f + 0.5f) + 
ceilf(fontHeight.ascent);
+                               break;

Same here. Due to the + 0.5f the error is compensated (so this not just a complicated roundf() of the correct value as one might expect at the first glance). Unless height differs, the BLayoutUtils::AlignInFrame() algorithm would compute the same value, since it always rounds down. If height differs, the previous algorithm would, however, compute a smaller value. That would contradict your observation, though.

Some actual numbers would be nice.

+                       case B_ALIGN_BOTTOM:
+                               location.y = rect.bottom - 
ceilf(fontHeight.descent);

Since BView::DrawString() draws above the specified baseline (at least that's how it is documented in the BeBook), this leaves a gap of one pixel.

CU, Ingo


Other related posts: