Closed
Bug 663119
Opened 13 years ago
Closed 12 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•13 years ago
|
||
Attachment #538314 -
Flags: review?(joshmoz)
Assignee | ||
Updated•13 years ago
|
Whiteboard: [needs review]
Assignee | ||
Comment 2•13 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•13 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•13 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•13 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•13 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•13 years ago
|
Whiteboard: [needs review]
Assignee | ||
Comment 8•13 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•13 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•13 years ago
|
||
Attachment #538461 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Whiteboard: [ready to land][waits for dependencies]
Assignee | ||
Comment 11•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0f2b9a6e36c7
Status: ASSIGNED → RESOLVED
Closed: 12 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
•