Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fixes #68 (Improper usage of -backingAlignedRect:options:) #71

Merged
merged 9 commits into from Mar 26, 2013

Conversation

Projects
None yet
3 participants
Contributor

indragiek commented Mar 3, 2013

No description provided.

Contributor

indragiek commented Mar 3, 2013

I'll wait till you guys merge this one in and submit another PR to use notifications to set the frame inside of inside -drawRect (#69).

@indragiek you could always PR that into this branch. Or submit the PR but indicate that it depends on this being merged.

No need to synchronously wait on us :).

@dannygreg dannygreg commented on an outdated diff Mar 4, 2013

Rebel/NSView+RBLAlignmentAdditions.h
@@ -0,0 +1,19 @@
+//
+// NSView+RBLAlignmentAdditions.h
+// Rebel
+//
+// Created by Indragie Karunaratne on 2013-03-02.
+// Copyright (c) 2013 GitHub. All rights reserved.
+//
+
+#import <Cocoa/Cocoa.h>
+
+@interface NSView (RBLAlignmentAdditions)
+
+// Returns a backing store pixel aligned rectangle in view coordinates
+//
+// Uses -backingAlignedRect:options: internally and converts between window coordinates
@dannygreg

dannygreg Mar 4, 2013

Chuck some back-ticks around that -backingAligned… badboy.

@dannygreg dannygreg commented on an outdated diff Mar 4, 2013

Rebel/NSView+RBLAlignmentAdditions.h
+// NSView+RBLAlignmentAdditions.h
+// Rebel
+//
+// Created by Indragie Karunaratne on 2013-03-02.
+// Copyright (c) 2013 GitHub. All rights reserved.
+//
+
+#import <Cocoa/Cocoa.h>
+
+@interface NSView (RBLAlignmentAdditions)
+
+// Returns a backing store pixel aligned rectangle in view coordinates
+//
+// Uses -backingAlignedRect:options: internally and converts between window coordinates
+// and view coordinates.
+- (NSRect)rbl_viewBackingAlignedRect:(NSRect)rect options:(NSAlignmentOptions)options;
@dannygreg

dannygreg Mar 4, 2013

How about using alignmentOptions: rather than just options: for the second parameter name here.

Maybe it's just me, but I usually think of options as being a generic dictionary.

Just a couple comments.

Going to assign to @jspahrsummers for the final sign-off as he wrote the original code.

Much ❤️

@jspahrsummers jspahrsummers was assigned Mar 4, 2013

@jspahrsummers jspahrsummers and 2 others commented on an outdated diff Mar 4, 2013

Rebel/NSView+RBLAlignmentAdditions.h
+// NSView+RBLAlignmentAdditions.h
+// Rebel
+//
+// Created by Indragie Karunaratne on 2013-03-02.
+// Copyright (c) 2013 GitHub. All rights reserved.
+//
+
+#import <Cocoa/Cocoa.h>
+
+@interface NSView (RBLAlignmentAdditions)
+
+// Returns a backing store pixel aligned rectangle in view coordinates
+//
+// Uses `-backingAlignedRect:options:` internally and converts between window coordinates
+// and view coordinates.
+- (NSRect)rbl_viewBackingAlignedRect:(NSRect)rect alignmentOptions:(NSAlignmentOptions)options;
@jspahrsummers

jspahrsummers Mar 4, 2013

Contributor

Collapsed comment:

How about using alignmentOptions: rather than just options: for the second parameter name here.

Maybe it's just me, but I usually think of options as being a generic dictionary.

@dannygreg That doesn't match the base method name, though, which is -backingAlignedRect:options:.

@dannygreg

dannygreg Mar 4, 2013

That's a fair shout… I was bringing it up as a conversation point rather than a "must have".

@indragiek

indragiek Mar 4, 2013

Contributor

What's the decision here? alignmentOptions or options?

@dannygreg

dannygreg Mar 4, 2013

@indragiek you tell us, it's your PR :).

Which do you prefer?

@indragiek

indragiek Mar 4, 2013

Contributor

@dannygreg I think options might be a better idea, to stay consistent with method naming. I'll change that back.

@jspahrsummers jspahrsummers and 1 other commented on an outdated diff Mar 4, 2013

Rebel/NSView+RBLAlignmentAdditions.m
@@ -0,0 +1,18 @@
+//
+// NSView+RBLAlignmentAdditions.m
+// Rebel
+//
+// Created by Indragie Karunaratne on 2013-03-02.
+// Copyright (c) 2013 GitHub. All rights reserved.
+//
+
+#import "NSView+RBLAlignmentAdditions.h"
+
+@implementation NSView (RBLAlignmentAdditions)
+
+- (NSRect)rbl_viewBackingAlignedRect:(NSRect)rect alignmentOptions:(NSAlignmentOptions)options {
+ NSRect windowBackingRect = [self backingAlignedRect:rect options:options];
@jspahrsummers

jspahrsummers Mar 4, 2013

Contributor

The rectangle needs to be converted to window coordinates before passing it in to this method.

Also, what happens if this view doesn't have a window?

@indragiek

indragiek Mar 4, 2013

Contributor

Unless this is an Apple documentation mistake, the docs state:

aRect
The rectangle in view coordinates.

And returns a rectangle in window coordinates.

@indragiek

indragiek Mar 4, 2013

Contributor

And if it doesn't have a window, I'm guessing -backingAlignedRect:options: wouldn't work. Maybe worth adding a comment about that?

@jspahrsummers

jspahrsummers Mar 4, 2013

Contributor

The documentation also says:

Uses the NSIntegralRectWithOptions function to produce a backing store pixel aligned rectangle from the given input rectangle in window coordinates.

I'm pretty sure I recall running into a bug where it accepted window coordinates. There's an easy way to find out, though – we just need a unit test. :P

And if it doesn't have a window, I'm guessing -backingAlignedRect:options: wouldn't work. Maybe worth adding a comment about that?

There should either be a fallback mechanism or an assertion. In either case the behavior should be reflected in the documentation.

Contributor

jspahrsummers commented Mar 4, 2013

Can you please add unit tests for this?

Contributor

indragiek commented Mar 4, 2013

Sure, I'll add those soon.

Contributor

indragiek commented Mar 23, 2013

Sorry this took so long, added unit tests and @jspahrsummers was right about it needing the rectangle in window coordinates (documentation bug).

@jspahrsummers jspahrsummers commented on an outdated diff Mar 25, 2013

Rebel/NSView+RBLAlignmentAdditions.h
@@ -0,0 +1,19 @@
+//
+// NSView+RBLAlignmentAdditions.h
+// Rebel
+//
+// Created by Indragie Karunaratne on 2013-03-02.
+// Copyright (c) 2013 GitHub. All rights reserved.
+//
+
+#import <Cocoa/Cocoa.h>
+
+@interface NSView (RBLAlignmentAdditions)
+
+// Returns a backing store pixel aligned rectangle in view coordinates
@jspahrsummers

jspahrsummers Mar 25, 2013

Contributor

Can you please move this line to the end? ("Returns …" should always be at the end of a method's documentation).

@jspahrsummers jspahrsummers commented on an outdated diff Mar 25, 2013

Rebel/NSView+RBLAlignmentAdditions.m
@@ -0,0 +1,20 @@
+//
+// NSView+RBLAlignmentAdditions.m
+// Rebel
+//
+// Created by Indragie Karunaratne on 2013-03-02.
+// Copyright (c) 2013 GitHub. All rights reserved.
+//
+
+#import "NSView+RBLAlignmentAdditions.h"
+
+@implementation NSView (RBLAlignmentAdditions)
+
+- (NSRect)rbl_viewBackingAlignedRect:(NSRect)rect options:(NSAlignmentOptions)options {
+ NSAssert(self.window != nil, @"View must have a window in order to obtain a rectangle aligned to the backing.");
@jspahrsummers

jspahrsummers Mar 25, 2013

Contributor

Thinking about it some more, I don't really like this behavior (though I know I suggested it). It's way too easy for views to become temporarily detached from windows – the results of using this method while that's happening could be disastrous.

I'd prefer to fall back to a best guess, a la https://github.com/jspahrsummers/Velvet/blob/master/Velvet/VELView.m#L1023-L1039.

Contributor

jspahrsummers commented Mar 25, 2013

@indragiek No worries, and thanks for fixing it up! Just a couple more notes.

Contributor

indragiek commented Mar 25, 2013

🏁

@jspahrsummers jspahrsummers merged commit 2062ad4 into github:master Mar 26, 2013

Contributor

jspahrsummers commented Mar 26, 2013

Thanks! 💥

@jspahrsummers jspahrsummers removed their assignment May 22, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment