Closed Bug 665571 Opened 10 years ago Closed 10 years ago

DrawCellWithSnapping should handle special NSZeroSize value in CellRenderSettings

Categories

(Core :: Widget: Cocoa, defect)

All
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: mounir, Assigned: mounir)

References

Details

Attachments

(2 files, 2 obsolete files)

The CellRenderSettings structure passed to DrawCellWithSnapping can contain a or multiple NSZeroSize to reflect that a given ControlSize isn't available for the widget.
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #541418 - Flags: review?(mstange)
Status: NEW → ASSIGNED
Whiteboard: [needs review]
Comment on attachment 541418 [details] [diff] [review]
Patch v1

Oups, sorry, small mistake...
Attachment #541418 - Flags: review?(mstange)
Attached patch Patch v1.1 (obsolete) — Splinter Review
Attachment #541418 - Attachment is obsolete: true
Attachment #541429 - Flags: review?(mstange)
I'll review it tomorrow.
Comment on attachment 541429 [details] [diff] [review]
Patch v1.1

>diff --git a/widget/src/cocoa/nsNativeThemeCocoa.mm b/widget/src/cocoa/nsNativeThemeCocoa.mm
>--- a/widget/src/cocoa/nsNativeThemeCocoa.mm
>+++ b/widget/src/cocoa/nsNativeThemeCocoa.mm
>@@ -275,16 +275,29 @@ static int EnumSizeForCocoaSize(NSContro
>   if (cocoaControlSize == NSMiniControlSize)
>     return miniControlSize;
>   else if (cocoaControlSize == NSSmallControlSize)
>     return smallControlSize;
>   else
>     return regularControlSize;
> }
> 
>+static NSControlSize CocoaSizeForEnum(PRInt32 enumControlSize) {
>+  if (enumControlSize == miniControlSize) {
>+    return NSMiniControlSize;
>+  }
>+  if (enumControlSize == smallControlSize) {
>+    return smallControlSize;

return NSSmallControlSize;

>+  }
>+
>+  NS_ASSERTION(enumControlSize == regularControlSize,
>+               "enumControlSize isn't valid!");
>+  return NSRegularControlSize;
>+}

I don't think the assertion and the braces are necessary. Established coding style in the same file overrides global coding style, so I think you should make this function look like EnumSizeForCocoaSize above. Shorter == more readable in this case.

> /*
>+ * This is a helper method that returns the required NSControlSize given a size
>+ * and the size of the three constrols plus a tolerance.

constrols -> controls

>+ * size - The width or the height of the element to draw.
>+ * sizes - An array with the all the width/height of the element for its
>+ *         different sizes.
>+ * tolerance - The tolerance as passed to DrawCellWithSnapping.

Please mention that it returns NSRegularControlSize when all sizes are zero.

>+ */
>+static NSControlSize FindControlSize(CGFloat size, CGFloat* sizes, CGFloat tolerance)
>+{
>+  for (PRUint32 i = miniControlSize; i <= regularControlSize; ++i) {
>+    if (sizes[i] == 0) {
>+      continue;
>+    }
>+
>+    CGFloat next = 0;
>+    // Find next value.
>+    for (PRUint32 j = i+1; j <= regularControlSize; ++j) {
>+      if (sizes[j] != 0) {
>+        next = sizes[j];
>+        break;
>+      }
>+    }
>+
>+    // If it's the last value, we pick it.
>+    if (next == 0) {
>+      return CocoaSizeForEnum(i);
>+    }
>+
>+    if (size <= sizes[i] + tolerance && size < next) {
>+      return CocoaSizeForEnum(i);
>+    }
>+  }
>+
>+  // If we are here, that means sizes[] was an array with only empty values
>+  // or the algorithm above is wrong.
>+  // The former can happen but the later would be wrong.
>+  NS_ASSERTION(sizes[0] == 0 && sizes[1] == 0 && sizes[2] == 0,
>+               "We found no control! We shouldn't be there!");
>+  return CocoaSizeForEnum(regularControlSize);
>+}

How about this:

static NSControlSize FindControlSize(CGFloat size, CGFloat* sizes, CGFloat tolerance)
{
  PRInt32 controlSize = regularControlSize;
  CGFloat minKeepControlSize = CGFLOAT_MAX;
  for (PRInt32 i = regularControlSize; i >= miniControlSize; --i) {
    if (sizes[i] == 0)
      continue;

    if (size < minKeepControlSize && size <= sizes[i] + tolerance) {
      controlSize = i;
      minKeepControlSize = sizes[i];
    }
  }

  return CocoaSizeForEnum(controlSize);
}
Attached file test script
(In reply to comment #5)
> Comment on attachment 541429 [details] [diff] [review] [review]
> >+  }
> >+
> >+  NS_ASSERTION(enumControlSize == regularControlSize,
> >+               "enumControlSize isn't valid!");
> >+  return NSRegularControlSize;
> >+}
> 
> I don't think the assertion and the braces are necessary. Established coding
> style in the same file overrides global coding style, so I think you should
> make this function look like EnumSizeForCocoaSize above. Shorter == more
> readable in this case.

If you want.

> How about this:
> 
> static NSControlSize FindControlSize(CGFloat size, CGFloat* sizes, CGFloat
> tolerance)
> {
>   PRInt32 controlSize = regularControlSize;
>   CGFloat minKeepControlSize = CGFLOAT_MAX;
>   for (PRInt32 i = regularControlSize; i >= miniControlSize; --i) {
>     if (sizes[i] == 0)
>       continue;
> 
>     if (size < minKeepControlSize && size <= sizes[i] + tolerance) {
>       controlSize = i;
>       minKeepControlSize = sizes[i];
>     }
>   }
> 
>   return CocoaSizeForEnum(controlSize);
> }

It seems correct too but I don't think it's easier to understand. I can use it if you want to but I would prefer to add an assert at the end in that case. Seems safer.
Attached patch Patch v1.2Splinter Review
With all comments applied except the last one, see previous comment.
Attachment #541429 - Attachment is obsolete: true
Attachment #544230 - Flags: review?(mstange)
Attachment #541429 - Flags: review?(mstange)
Attachment #544230 - Flags: review?(mstange) → review+
Whiteboard: [needs review] → [inbound]
Attachment #544230 - Flags: checkin+
http://hg.mozilla.org/mozilla-central/rev/0bd82b867cf0
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Whiteboard: [inbound]
You need to log in before you can comment on or make changes to this bug.