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

Updated ImageLoader::saveImageToFile to support .jpeg #930

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@kikiotsuka

kikiotsuka commented Jul 23, 2015

Extract the extension after finding the dot, then compare it to the list of valid extensions

#929 (comment)

Updated ImageLoader::saveImageToFile to support .jpeg
Extract the extension after finding the dot, then compare it to the list of valid extensions
@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Jul 23, 2015

Contributor

Looks good. One thing I would change: you should use std::string::find_last_of() or std::string::rfind() instead of a raw for loop. Also assigning an empty string to extension right after instantiation (in line 229) is pointless. std::string extension; will do just fine.

Contributor

Foaly commented Jul 23, 2015

Looks good. One thing I would change: you should use std::string::find_last_of() or std::string::rfind() instead of a raw for loop. Also assigning an empty string to extension right after instantiation (in line 229) is pointless. std::string extension; will do just fine.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jul 23, 2015

Member

Duplicate of #931... guys no need to rush and make the work twice 😛

#931 is better (see comments above) so I think we should close this one.

Member

LaurentGomila commented Jul 23, 2015

Duplicate of #931... guys no need to rush and make the work twice 😛

#931 is better (see comments above) so I think we should close this one.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jul 23, 2015

Member

Duplicate? It more looks like the patch that originated from here. And it's PR vs. issue. IIRC we kept both open?

Member

TankOs commented Jul 23, 2015

Duplicate? It more looks like the patch that originated from here. And it's PR vs. issue. IIRC we kept both open?

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jul 23, 2015

Member

The issue is #929. From this issue, two PRs were created in parallel by two different persons, #930 (this one) and #931.

Member

LaurentGomila commented Jul 23, 2015

The issue is #929. From this issue, two PRs were created in parallel by two different persons, #930 (this one) and #931.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jul 24, 2015

Member

Oh Jesus, GitHub will always manage to confuse me. Sorry! :-)

Member

TankOs commented Jul 24, 2015

Oh Jesus, GitHub will always manage to confuse me. Sorry! :-)

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