DrawCellWithSnapping should handle special NSZeroSize value in CellRenderSettings

RESOLVED FIXED in mozilla8

Status

()

Core
Widget: Cocoa
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

Trunk
mozilla8
All
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
The CellRenderSettings structure passed to DrawCellWithSnapping can contain a or multiple NSZeroSize to reflect that a given ControlSize isn't available for the widget.
(Assignee)

Comment 1

6 years ago
Created attachment 541418 [details] [diff] [review]
Patch v1
Attachment #541418 - Flags: review?(mstange)
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
Whiteboard: [needs review]
(Assignee)

Comment 2

6 years ago
Comment on attachment 541418 [details] [diff] [review]
Patch v1

Oups, sorry, small mistake...
Attachment #541418 - Flags: review?(mstange)
(Assignee)

Comment 3

6 years ago
Created attachment 541429 [details] [diff] [review]
Patch v1.1
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);
}
Created attachment 543749 [details]
test script
(Assignee)

Comment 7

6 years ago
(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.
(Assignee)

Comment 8

6 years ago
Created attachment 544230 [details] [diff] [review]
Patch v1.2

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+
(Assignee)

Updated

6 years ago
Whiteboard: [needs review] → [inbound]
(Assignee)

Updated

6 years ago
Attachment #544230 - Flags: checkin+
http://hg.mozilla.org/mozilla-central/rev/0bd82b867cf0
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
(Assignee)

Updated

6 years ago
Whiteboard: [inbound]
You need to log in before you can comment on or make changes to this bug.