Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Image preview in menus #1162

Merged
merged 1 commit into from Dec 9, 2014
Merged

Conversation

@extrajordanary
Copy link
Contributor

@extrajordanary extrajordanary commented Nov 22, 2014

No description provided.

@extrajordanary
Copy link
Contributor Author

@extrajordanary extrajordanary commented Nov 22, 2014

[image setScalesWhenResized:YES];

// Report an error if the source isn't a valid image
if (![image isValid])

This comment has been minimized.

@NickyWeber

NickyWeber Nov 24, 2014
Contributor

You could improve readability a bit if you move this "isValid" statement up a bit and return nil after logging the error. The long else part could be indented back one level so you don't have to think in more code branches. In case isValid can be determined already after the image assignment move it to line 301.

+ (NSImage*) thumbnailImageForResource:(RMResource*)res {
NSImage *image = [res previewForResolution:nil];

// shrink image to appropriate size

This comment has been minimized.

@NickyWeber

NickyWeber Nov 24, 2014
Contributor

Redundant comment

// shrink image to appropriate size
[image setScalesWhenResized:YES];

// Report an error if the source isn't a valid image

This comment has been minimized.

@NickyWeber

NickyWeber Nov 24, 2014
Contributor

Redundant comment

float oldWidth = image.size.width;
float oldHeight = image.size.height;

// set the scale factor

This comment has been minimized.

@NickyWeber

NickyWeber Nov 24, 2014
Contributor

Redundant comment

{
NSLog(@"Invalid Image");
} else {
float maxWidth = 30.0; // edit value for larger/smaller thumbnails

This comment has been minimized.

@NickyWeber

NickyWeber Nov 24, 2014
Contributor

Comment is not informative. Please remove or rephrase if this is really needed. Consider renaming the var if it helps.

@NickyWeber
Copy link
Contributor

@NickyWeber NickyWeber commented Nov 24, 2014

Looks good.

This pull request looks a bit like #1163. Please incorporated the comments of that pull request into this one.

About my comment remarks: There's some good literature about commenting and good reasons to not do it. For example Robert C. Martin "Clean Code" chapter 4.

@cocojoe
Copy link
Member

@cocojoe cocojoe commented Dec 3, 2014

@extrajordanary Do you have your final tweaks ready? I would like to merge this soon.

@extrajordanary
Copy link
Contributor Author

@extrajordanary extrajordanary commented Dec 3, 2014

Okay, should be good!

@extrajordanary extrajordanary force-pushed the extrajordanary:image-preview branch from 0f057bc to c4fee53 Dec 4, 2014
@cocojoe
Copy link
Member

@cocojoe cocojoe commented Dec 9, 2014

@extrajordanary Thanks for your contribution.

cocojoe added a commit that referenced this pull request Dec 9, 2014
Image preview in menus
@cocojoe cocojoe merged commit fcc7cdf into spritebuilder:develop Dec 9, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.