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.

Sign up for GitHub

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

Jump to bottom

Support SDL color #99

Merged
merged 6 commits into from
Jul 7, 2017
Merged

Support SDL color #99

merged 6 commits into from
Jul 7, 2017

Conversation

Vraiment
Copy link
Contributor

@Vraiment Vraiment commented Jul 4, 2017

Hey, this is my first contribution to a project ever.

I saw you have issue #55 open and I realized it was something I could do, please let me know if I missed something, I tried to follow the style from Point and Rect classes.

AMDmi3
AMDmi3 requested changes Jul 4, 2017
View reviewed changes
Copy link
Member

@AMDmi3 AMDmi3 left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welcome to the club! 👍

Great job, though here's a dozen of minor improvements :)

You could also extend tests, but it can be done in separate PR.

Also please ignore AppVeyor failure, which is unrelated.

.gitignore Outdated
@@ -42,9 +42,14 @@ tests/live_*
tests/test_*
!tests/live_*.cc
!tests/test_*.cc
libSDL2pp.dylib
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good, but please submit these .gitignore changes as a separate PR, as they are unrelated.

SDL2pp/Color.cc Outdated
@@ -0,0 +1,27 @@
/*
libSDL2pp - C++11 bindings/wrapper for SDL2
Copyright (C) 2013-2015 Dmitry Marakasov <amdmi3@amdmi3.ru>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, it wasn't me who wrote it, so please put yourself here and in other new files :)

SDL2pp/Renderer.cc Outdated
@@ -197,6 +197,10 @@ Renderer& Renderer::SetDrawColor(Uint8 r, Uint8 g, Uint8 b, Uint8 a) {
return *this;
}

Renderer& Renderer::SetDrawColor(const Color color) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need const here

SDL2pp/Surface.cc Outdated
@@ -145,6 +145,13 @@ SDL_BlendMode Surface::GetBlendMode() const {
return blendMode;
}

Color Surface::GetColorAndAlphaMod() const {
Color color;
GetColorMod(color.r, color.g, color.b);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whitespace inconsistency, please use tabs

SDL2pp/Surface.cc Outdated
@@ -180,6 +187,10 @@ Surface& Surface::SetColorMod(Uint8 r, Uint8 g, Uint8 b) {
return *this;
}

Surface& Surface::SetColorAndAlphaMod(const Color color) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const

SDL2pp/Texture.hh Outdated
///
/// \throws SDL2pp::Exception
///
/// \see http://wiki.libsdl.org/SDL_GetTextureColorMod
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could reference SDL_GetTextureAlphaMod here too

SDL2pp/Texture.hh Outdated
///
/// \throws SDL2pp::Exception
///
/// \see http://wiki.libsdl.org/SDL_SetTextureColorMod
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could reference SDL_SetTextureAlphaMod here too

SDL2pp/Surface.hh Outdated
///
/// \throws SDL2pp::Exception
///
/// \see http://wiki.libsdl.org/SDL_SetSurfaceColorMod
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could reference SDL_SetSurfaceAlphaMod here too

SDL2pp/Surface.hh Outdated
///
/// \throws SDL2pp::Exception
///
/// \see http://wiki.libsdl.org/SDL_GetSurfaceColorMod
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could reference SDL_GetSurfaceAlphaMod here too

SDL2pp/Renderer.hh Outdated
/// \see http://wiki.libsdl.org/SDL_SetRenderDrawColor
///
////////////////////////////////////////////////////////////
Renderer& SetDrawColor(const Color color);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const

Vraiment added a commit to Vraiment/libSDL2pp that referenced this pull request Jul 4, 2017
@Vraiment
Addressed comments in pull request libSDL2pp#99
a3a78d2
@Vraiment
Copy link
Contributor Author

Vraiment commented Jul 4, 2017

As for the tests, I don't get them very well how they work. I guess they are not unit tests but rather like integration tests, right?

Have you seen Catch? Is an awesome very simple and elegant testing framework for C++

AMDmi3
AMDmi3 requested changes Jul 4, 2017
View reviewed changes
Copy link
Member

@AMDmi3 AMDmi3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also please rebase + push --force this to avoid unrelated changes in the history.

SDL2pp/Color.cc Outdated
@@ -1,6 +1,6 @@
/*
libSDL2pp - C++11 bindings/wrapper for SDL2
Copyright (C) 2013-2015 Dmitry Marakasov <amdmi3@amdmi3.ru>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please set correct date as well.

SDL2pp/Renderer.cc Outdated
@@ -197,7 +197,7 @@ Renderer& Renderer::SetDrawColor(Uint8 r, Uint8 g, Uint8 b, Uint8 a) {
return *this;
}

Renderer& Renderer::SetDrawColor(const Color color) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I've misguided you. It should better be const Color&. Before, I did benchmarks for Rect vs. const Rect& arguments, and the latter was better. We should do the same way here.

@AMDmi3
Copy link
Member

AMDmi3 commented Jul 4, 2017

As for the tests, I don't get them very well how they work. I guess they are not unit tests but rather like integration tests, right?

There are both.

Have you seen Catch? Is an awesome very simple and elegant testing framework for C++

No, but I've heard of it. From what I've heard, I'm using the very similar homegrown framework (grep testing.h)

Vraiment added a commit to Vraiment/libSDL2pp that referenced this pull request Jul 4, 2017
@Vraiment
Addressed comments in pull request libSDL2pp#99
510b679
Vraiment added a commit to Vraiment/libSDL2pp that referenced this pull request Jul 4, 2017
@Vraiment
Addressed comments in pull request libSDL2pp#99
02510d4
@Vraiment
Copy link
Contributor Author

Vraiment commented Jul 4, 2017

There, sorry for the mess, this should be good to go

@AMDmi3
Copy link
Member

AMDmi3 commented Jul 5, 2017

Looks good now apart from the Doxyfile.in changes. Since these were merged in separate PR they should go awat if you just rebase onto master.

@AMDmi3 AMDmi3 merged commit 6832359 into libSDL2pp:master Jul 7, 2017
@AMDmi3
Copy link
Member

AMDmi3 commented Jul 7, 2017

👍👍👍

Vraiment added a commit to Vraiment/libSDL2pp that referenced this pull request Jul 8, 2017
@Vraiment
Addressed comments in pull request libSDL2pp#99
5cb7438
Vraiment added a commit to Vraiment/libSDL2pp that referenced this pull request Jul 12, 2017
@Vraiment
Addressed comments in pull request libSDL2pp#99
d442924
Vraiment added a commit to Vraiment/libSDL2pp that referenced this pull request Jul 12, 2017
@Vraiment
Addressed comments in pull request libSDL2pp#99
1281d90
Vraiment added a commit to Vraiment/libSDL2pp that referenced this pull request Jul 12, 2017
@Vraiment
Addressed comments in pull request libSDL2pp#99
7da8b53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AMDmi3 AMDmi3 AMDmi3 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Vraiment @AMDmi3