Skip to content

support black and white and grayscale between - #4

Open
dredenba wants to merge 6 commits into
davecom:masterfrom
dredenba:addBlackAndWhiteSupport
Open

support black and white and grayscale between#4
dredenba wants to merge 6 commits into
davecom:masterfrom
dredenba:addBlackAndWhiteSupport

Conversation

@dredenba

@dredenba dredenba commented Sep 4, 2015

Copy link
Copy Markdown
  • support black and white and grayscale between in addition to color hues
  • add a few simple tests
  • change delegate method to indicate touch type

@davecom

davecom commented Sep 6, 2015

Copy link
Copy Markdown
Owner

Thanks for the contribution, I really do appreciate it, but unfortunately I can't accept this pull request for a few reasons:

  • It crashed for me when testing with EXEC_BAD_ACCESS in the iOS 9 simulator for iPhone 6 under Xcode 7 Beta 6
  • I think the black & white lopped onto the top looks a bit awkward, and should be optional at best
  • I don't understand the use case for the delegate wanting to know the type of touch; it seems to add unnecessary complexity

That said it looks like there are some other nice cleanups in here.

@dredenba

dredenba commented Sep 6, 2015

Copy link
Copy Markdown
Author

Thanks

  1. Don't have the Xcode beta yet, but not sure where the bad access would be coming from based on the code I added since it doesn't add any unusual memory usages ( just a few local variables). Also I would be surprised if any memory issue was introduced by iOS 9 but certainly is possible. Can you explain the steps to reproduce the problem?
  2. not sure if you are suggesting you don't want a black and white added at all? or if you would prefer it look different? or just that you would like the current pull request code to also make the color spectrum configurable?
  3. the added argument on the optional delegate method does serve a purpose (and could be ignored if not needed). For my particular use case the UI would do a different visual transition of the color change when the touch has finished versus during the rapid changes while the user is dragging their finger across the control. This is not unlike having the touch events broken out as the native api does with "touchesBegan", "touchesMoved", and "touchesEnded". The downside of this that I can see is that it means that any codebase using this control would want to update to the new delegate method when they upgrade to the new control code (though this should be a very simple change to do). Another variant on the solution that could provide some backwards compatibility but with the downside of introducing the support "legacy" code would be to have the code check for the original delegate method and use that otherwise use the new delegate method.

Would like to hear your thoughts

@davecom

davecom commented Sep 6, 2015

Copy link
Copy Markdown
Owner

Hi @dredenba

  1. I simply tried running the test project in the iPhone 6 simulator in Xcode 7 Beta 6 and after trying to change the color the EXEC_BAD_ACCESS immediately occurred.
  2. Yeah I don't really like the way it looks - it seems forced rather than a natural transition.
  3. I understand. That's an interesting use case.

@davecom

davecom commented Sep 11, 2015

Copy link
Copy Markdown
Owner

What do you think about the idea of making the additional grayscale bar optional?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants