Closed Bug 370106 Opened 17 years ago Closed 17 years ago

Need to translate from Cocoa to Gecko coordinates in nsScreen[Manager]Cocoa


(Core :: Widget: Cocoa, defect)

Not set





(Reporter: sylvain.pasche, Assigned: jaas)



(1 file, 2 obsolete files)

I don't know if I'm understanding correctly, but I think nsScreenCocoa::GetRect or nsScreenManagerCocoa::ScreenForRect should translate the Cocoa coordinates to Gecko ones. For instance, I'm getting negative values on my right screen (which goes higher that the left one).
I tried to look closer at the issue, and I found out that there is an inconsistency how the coordinates are managed between nsChildView and nsCocoaWindow.

nsChildView makes the assumption that the y=0 reference is the top of the first screen in the screens array:
That means that y<0 coordinates are possible if your second monitor is higher.

On the other hand, nsCocoaWindow takes the y=0 reference as the highest monitor:

My first suggestion would be to share the HighestPointOnAnyScreen/geckoRectToCocoaRect/cocoaRectToGeckoRect between nsCocoaWindow, nsScreenCocoa, nsScreenManagerCocoa and nsChildView. What do you think about it?
Flags: blocking1.9?
Attached patch first version (obsolete) — Splinter Review
This is simply a move of some functions from to a new file. I tried to identify all the places where to perform the translation. The popup menus are now appearing on the right place.
Attachment #255279 - Flags: review?(joshmoz)
Comment on attachment 255279 [details] [diff] [review]
first version

just a nit - the preferred format for patches is "diff -U", and my personal preference is "diff -U 8"
Attached patch first version, different format (obsolete) — Splinter Review
Attachment #255279 - Attachment is obsolete: true
Attachment #255382 - Flags: review?(joshmoz)
Attachment #255279 - Flags: review?(joshmoz)
Comment on attachment 255382 [details] [diff] [review]
first version, different format

+  // XXX maybe highestScreenPoint should be cached to avoid recomputing it everytime.

Please remove since caching is tricky when monitors change and the desire to cache that should be pretty clear without a comment.

+nsRect cocoaRectToGeckoRect(const NSRect &cocoaRect)
+  // We only need to change the Y coordinate by starting with the screen
+  // height, subtracting the gecko Y coordinate, and subtracting the
+  // height.
+  return nsRect((nscoord)cocoaRect.origin.x,
+                (nscoord)(HighestPointOnAnyScreen() - (cocoaRect.origin.y + cocoaRect.size.height)),
+                (nscoord)cocoaRect.size.width,
+                (nscoord)cocoaRect.size.height);

That comment isn't correct. We're not subtracting the gecko Y coordinate from anything.

Otherwise look good! Thanks.
Attachment #255382 - Flags: review?(joshmoz) → review+
Attachment #255382 - Flags: superreview?(pavlov)
Comment on attachment 255382 [details] [diff] [review]
first version, different format

you're probably better off not making a nsCocoaCoordsUtils and if you want a seperate file making a more general nsCocoaUtils or somesuch that can hold other shared code.
Comment on attachment 255382 [details] [diff] [review]
first version, different format

I agree, a more general name for the utility file would be good. I don't think we need to change the patch for that, leaving the sr request.
Attachment #255382 - Flags: superreview?(pavlov)
Attached patch fix v1.1Splinter Review
Makes the new utils file generic, fixes warnings, and addresses my own comments.
Attachment #255382 - Attachment is obsolete: true
Attachment #256073 - Flags: superreview?(mikepinkerton)
Attachment #256073 - Flags: superreview?(mikepinkerton) → superreview?(pavlov)
Attachment #256073 - Flags: superreview?(pavlov) → superreview+
fixed on trunk
Closed: 17 years ago
Resolution: --- → FIXED
Flags: blocking1.9? → in-testsuite?
You need to log in before you can comment on or make changes to this bug.