Closed Bug 663119 Opened 14 years ago Closed 13 years ago

Implement native vertical meters for MacOS X

Categories

(Core :: Widget: Cocoa, defect)

Product:
Core
Component:
Widget: Cocoa
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla16

People

(Reporter: mounir, Assigned: mounir)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

Patch ready to land
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.
Attached patch Patch v1 (obsolete) — — Splinter Review
Attachment #538314 - Flags: review?(joshmoz)
Whiteboard: [needs review]
Attached patch Patch v1 (obsolete) — — Splinter Review
Adding a test for RTL meter.
Attachment #538314 - Attachment is obsolete: true
Attachment #538314 - Flags: review?(joshmoz)
Attachment #538316 - Flags: review?(joshmoz)
Attached patch Patch v1 (obsolete) — — Splinter Review
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)
+ 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.
Attached patch Patch v1 (obsolete) — — Splinter Review
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)
Blocks: 663340
Comment on attachment 538461 [details] [diff] [review] Patch v1 Benoit - can you review this?
Attachment #538461 - Flags: review?(joshmoz) → review?(bgirard)
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+
Whiteboard: [needs review]
(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?
(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.
Attachment #538461 - Attachment is obsolete: true
Whiteboard: [ready to land][waits for dependencies]
https://hg.mozilla.org/mozilla-central/rev/0f2b9a6e36c7
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.

Attachment

General

Creator:
Created:
Updated:
Size: