Closed
Bug 663119
Opened 14 years ago
Closed 13 years ago
Implement native vertical meters for MacOS X
Categories
(Core :: Widget: Cocoa, defect)
Core
Widget: Cocoa
Tracking
()
RESOLVED
FIXED
mozilla16
People
(Reporter: mounir, Assigned: mounir)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 4 obsolete files)
6.52 KB,
patch
|
Details | Diff | Splinter Review |
Given that there is no native vertical meter, we will have to use the native horizontal meter and rotate it.
Assignee | ||
Comment 1•14 years ago
|
||
Attachment #538314 -
Flags: review?(joshmoz)
Assignee | ||
Updated•14 years ago
|
Whiteboard: [needs review]
Assignee | ||
Comment 2•14 years ago
|
||
Adding a test for RTL meter.
Attachment #538314 -
Attachment is obsolete: true
Attachment #538314 -
Flags: review?(joshmoz)
Attachment #538316 -
Flags: review?(joshmoz)
Assignee | ||
Comment 3•14 years ago
|
||
That was not for this bug... Re-attaching the correct file.
Attachment #538316 -
Attachment is obsolete: true
Attachment #538316 -
Flags: review?(joshmoz)
Attachment #538319 -
Flags: review?(joshmoz)
Comment 4•14 years ago
|
||
+ CGContextTranslateCTM(cgContext, CGRectGetMidX(rect), CGRectGetMidY(rect));
+ CGContextRotateCTM(cgContext, -M_PI / 2.f);
+ CGContextTranslateCTM(cgContext, -CGRectGetMidX(rect), -CGRectGetMidY(rect));
+ }
+
+ DrawCellWithSnapping(cell, cgContext, rect,
meterSetting, VerticalAlignFactor(aFrame),
- mCellDrawView, IsFrameRTL(aFrame));
+ mCellDrawView, !vertical && IsFrameRTL(aFrame));
I think you need to wrap this all into CGContextSaveGState / CGContextRestoreGState, otherwise the rotation transform might affect things that are painted after the meter.
Assignee | ||
Comment 5•14 years ago
|
||
Adding save/restore state. Though, I didn't add that because it was working well without it.
By the way, Steven and Markus, feel free to still this review. I asked Josh because I'm used to ask him to review my Cocoa widget patches but I would bet he wouldn't mind to save some time :)
Attachment #538319 -
Attachment is obsolete: true
Attachment #538319 -
Flags: review?(joshmoz)
Attachment #538461 -
Flags: review?(joshmoz)
Comment on attachment 538461 [details] [diff] [review]
Patch v1
Benoit - can you review this?
Attachment #538461 -
Flags: review?(joshmoz) → review?(bgirard)
Comment 7•14 years ago
|
||
Comment on attachment 538461 [details] [diff] [review]
Patch v1
Review of attachment 538461 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good, r+ with these fixed.
::: widget/reftests/meter-vertical-native-style.html
@@ +5,5 @@
> + <body>
> +<!-- For some reasons, the ref has a small offset when there are spaces between meters.
> + Given that we don't want to test -moz-transform and even a perfect match but just
> + the general rendering, we are going to keep this dirty test.
> + It's very similar to the non-vertical test with a difference, for the RTL: RTL
This acronym wasn't obvious to me at first, I suggest writing out Right to Left.
::: widget/src/cocoa/nsNativeThemeCocoa.mm
@@ +1329,5 @@
>
> + HIRect rect = CGRectStandardize(inBoxRect);
> + BOOL vertical = IsVerticalMeter(aFrame);
> +
> + CGContextSaveGState(cgContext);
System calls should be prefixed with |::|. All the CGContext*/CGRect* calls here. For example:
|::CGContextSaveGState(cgContext);|
@@ +1342,5 @@
> + * to get a rectangle with horizontal dimensions.
> + * Finally, we want to show a vertical meter so we want to rotate the result
> + * so it is vertical. We do that by changing the context.
> + */
> + float tmp = rect.size.width;
A bit of a gotcha here, CGFloat is a typedef for either float (32) or double (64) so I suggest replacing your usage of float here and '.f' by CGFloat here. This has caused us a few bugs when porting to 64-bit so I suggest we use it consistently. In this case we would be assigning a double to a float on 64-bit.
Attachment #538461 -
Flags: review?(bgirard) → review+
Updated•14 years ago
|
Whiteboard: [needs review]
Assignee | ||
Comment 8•14 years ago
|
||
(In reply to comment #7)
> ::: widget/reftests/meter-vertical-native-style.html
> @@ +5,5 @@
> > + <body>
> > +<!-- For some reasons, the ref has a small offset when there are spaces between meters.
> > + Given that we don't want to test -moz-transform and even a perfect match but just
> > + the general rendering, we are going to keep this dirty test.
> > + It's very similar to the non-vertical test with a difference, for the RTL: RTL
>
> This acronym wasn't obvious to me at first, I suggest writing out Right to
> Left.
RTL is the same acronym used for the HTML attribute 'dir' (|dir="rtl"|). I think most devs know what it means and if they don't they will learn about it at some point. In the widget code, for example, RTL appears multiple time.
> ::: widget/src/cocoa/nsNativeThemeCocoa.mm
> @@ +1329,5 @@
> >
> > + HIRect rect = CGRectStandardize(inBoxRect);
> > + BOOL vertical = IsVerticalMeter(aFrame);
> > +
> > + CGContextSaveGState(cgContext);
>
> System calls should be prefixed with |::|. All the CGContext*/CGRect* calls
> here. For example:
> |::CGContextSaveGState(cgContext);|
In this file, only one method is prefixed with |::|, it's |GetThemeMetric|. Obviously, this rule doesn't apply for this file.
> @@ +1342,5 @@
> > + * to get a rectangle with horizontal dimensions.
> > + * Finally, we want to show a vertical meter so we want to rotate the result
> > + * so it is vertical. We do that by changing the context.
> > + */
> > + float tmp = rect.size.width;
>
> A bit of a gotcha here, CGFloat is a typedef for either float (32) or double
> (64) so I suggest replacing your usage of float here and '.f' by CGFloat
> here. This has caused us a few bugs when porting to 64-bit so I suggest we
> use it consistently. In this case we would be assigning a double to a float
> on 64-bit.
Indeed, I should use CGFloat. Though, I do not understand what you meant by "I suggest replacing your usage of float here *and '.f'* by CGFloat here". Do you want me to use CGFloat(2) instead of 2.f?
Comment 9•14 years ago
|
||
(In reply to comment #8)
> Indeed, I should use CGFloat. Though, I do not understand what you meant by
> "I suggest replacing your usage of float here *and '.f'* by CGFloat here".
> Do you want me to use CGFloat(2) instead of 2.f?
Yes, that's what I meant but now that I think of it I don't think that is required. We just need float -> CGFloat.
Assignee | ||
Comment 10•14 years ago
|
||
Attachment #538461 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Whiteboard: [ready to land][waits for dependencies]
Assignee | ||
Comment 11•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [ready to land][waits for dependencies]
Target Milestone: --- → mozilla16
You need to log in
before you can comment on or make changes to this bug.
Description
•