Adding customisable visualisation support for the paws. #57

dirtydanee · 2018-02-04T20:21:24Z

Issue #9, creating possibility to define custom visualisation. Customisable properties:

look of the "paw" by injecting custom bezier path
colour, brightness and the maximum number of the paws being shown
the radiuses of the circle and cross elements

Default values are all falling back to the already defined ones. Let me know if you see anything how i could improve my PR.

wojciechczerski · 2018-02-11T16:38:26Z

You have made an awesome work here @dirtydanee! Thank you for your effort!
I have some minor change requests which I hope won't be difficult to fix.

dirtydanee · 2018-02-11T16:43:45Z

@wojciechczerski just hit me with the list you would like me to change.

wojciechczerski · 2018-02-11T16:38:50Z

SwiftMonkeyPaws/MonkeyPaws.swift

+            ///
+            /// - randomized: random colour for each paw
+            /// - constant: same colour for the paws
+            enum Colour {


Should be public.

wojciechczerski · 2018-02-11T16:40:14Z

SwiftMonkeyPaws/MonkeyPaws.swift

+
+    public struct Configuration {
+        // Customise the appearance of the paws
+        public struct Paws {


The default struct initializer is internal therefore it's not possible to instantiate this object without an explicit public init() defined.

wojciechczerski · 2018-02-11T16:46:30Z

SwiftMonkeyPaws/MonkeyPaws.swift

+            // Maximum visible paws at one time
+            let maxShown: Int
+
+            public static let `default`: Paws = Paws(colour: .randomized,


If you add the public init() for Paws you might as well provide default parameter values, like that:

public init(colour: Colour = .randomized, brightness: CGFloat = 0.5, maxShown: Int = 15) { self.colour = colour self.brightness = brightness self.maxShown = maxShown }

The result is that writing Paws.default is equal to Paws() but with default initializer parameter values I think the usage is slightly more convenient. What do you think?

wojciechczerski · 2018-02-11T16:47:55Z

SwiftMonkeyPaws/MonkeyPaws.swift

+                                                     maxShown: 15)
+        }
+
+        public struct Radius {


public init(...) should be added.

wojciechczerski · 2018-02-11T16:49:29Z

SwiftMonkeyPaws/MonkeyPaws.swift

+            /// Radius of the circle draw upon ending a touch event
+            let circle: CGFloat
+
+            public static let `default`: Radius = Radius(cross: 7,


Please consider replacing with a parameterless public initializer with all parameters having default values (just like it was suggested for the Paws object).

wojciechczerski · 2018-02-11T16:50:19Z

SwiftMonkeyPaws/MonkeyPaws.swift

+        let paws: Paws
+        let radius: Radius
+
+        public static let `default`: MonkeyPaws.Configuration = MonkeyPaws.Configuration(paws: Paws.default, radius: Radius.default)


Please consider replacing with a parameterless public initializer with all parameters having default values (just like it was suggested for the Paws object).

makes sense.

wojciechczerski · 2018-02-11T16:54:30Z

SwiftMonkeyPaws/MonkeyPaws.swift

 public class MonkeyPaws: NSObject, CALayerDelegate {
+
+    public struct Configuration {


Would it make sense to move this struct to a separate file? If you want the usage to stay the same (MonkeyPaws.Configuration) it can be added as an extension:

extension MonkeyPaws { public struct Configuration { ... } }

What do you think?

wojciechczerski · 2018-02-11T17:06:00Z

@dirtydanee If you will be fixing any of the PR comments you might just force-push the branch with new changes ;) Or if you prefer add some 'fix review comment' commits 😄

dirtydanee · 2018-02-11T17:09:29Z

@wojciechczerski my latest commit includes all the changes from your review 🤜🤛

wojciechczerski · 2018-02-11T22:38:47Z

SwiftMonkeyPaws/MonkeyPaws.swift


        let counter = Gesture.counter
        Gesture.counter += 1

        let angle = 45 * (CGFloat(fmod(Float(counter) * 0.279, 1)) * 2 - 1)
        let mirrored = counter % 2 == 0
-        let colour = UIColor(hue: CGFloat(fmod(Float(counter) * 0.391, 1)), saturation: 1, brightness: 0.5, alpha: 1)
-        startLayer.path = monkeyHandPath(angle: angle, scale: 1, mirrored: mirrored).cgPath
+        let colour: UIColor


Could we move the color computation to a separate function? For example:

func pawsColor(config: Configuration.Paws, seed: Int) -> UIColor { switch config.colour { case .randomized: return UIColor(hue: CGFloat(fmod(Float(seed) * 0.391, 1)), saturation: 1, brightness: config.brightness, alpha: 1) case .constant(let constantColour): return constantColour.colorWithBrightness(brightness: config.brightness) } }

wojciechczerski · 2018-02-11T22:42:18Z

SwiftMonkeyPaws/MonkeyPaws.swift

-    bezierPath.apply(CGAffineTransform(translationX: 0.5, y: 0))
-
-    bezierPath.apply(CGAffineTransform(scaleX: scale, y: scale))
+private func customizePath(_ path: UIBezierPath, angle: CGFloat, scale: CGFloat, mirrored: Bool) -> UIBezierPath {


The scale parameter is always 1 therefore I would just remove it. Also the remaining angle and mirrored could be computed inside based on a seed parameter (equal to Gesture.counter):

func customize(path: UIBezierPath, seed: Int) -> UIBezierPath

Would it make sense? We could then remove this code from the init:

let angle = 45 * (CGFloat(fmod(Float(counter) * 0.279, 1)) * 2 - 1) let mirrored = counter % 2 == 0

wojciechczerski · 2018-02-11T22:42:32Z

SwiftMonkeyPaws/MonkeyPaws.swift

-
-    bezierPath.apply(CGAffineTransform(scaleX: scale, y: scale))
+private func customizePath(_ path: UIBezierPath, angle: CGFloat, scale: CGFloat, mirrored: Bool) -> UIBezierPath {
+    path.apply(CGAffineTransform(translationX: 0.5, y: 0))


Is this needed?

i guess not. removed.

wojciechczerski · 2018-02-11T22:42:49Z

SwiftMonkeyPaws/MonkeyPaws.swift

+private func customizePath(_ path: UIBezierPath, angle: CGFloat, scale: CGFloat, mirrored: Bool) -> UIBezierPath {
+    path.apply(CGAffineTransform(translationX: 0.5, y: 0))
+
+    path.apply(CGAffineTransform(scaleX: scale, y: scale))


Like mentioned before, the scale is always 1 therefore I would just remove it.

wojciechczerski · 2018-02-11T22:50:55Z

SwiftMonkeyPaws/Sources/Extensions/MonkeyPaws+Configuration.swift

+
+import UIKit
+
+extension MonkeyPaws {


Now I see that putting it in an extension was a bad idea. I like the separate file but when you start to create your custom configuration you immediately see the annoying repeated. For example instead of this:

let pawsConfig = Configuration.Paws(colour: .constant(.cyan), brightness: 0.5, maxShown: 3) let radiusConfig = Configuration.Radius(cross: 10, circle: 10) let config = Configuration(paws: pawsConfig, radius: radiusConfig)

you have to write:

let pawsConfig = MonkeyPaws.Configuration.Paws(colour: .constant(.cyan), brightness: 0.5, maxShown: 3) let radiusConfig = MonkeyPaws.Configuration.Radius(cross: 10, circle: 10) let config = MonkeyPaws.Configuration(paws: pawsConfig, radius: radiusConfig)

So, another idea: how about we move the Configuration struct to a separate Configuration.swift file? It shouldn't be in the Extensions folder but in the same folder where MonkeyPaws.swift is. And it will be just Configuration and not MonkeyPaws.Configuration. What do you think?

I like to create namespaces, and for this configuration tight coupling with MonkeyPaws makes sense to me. However, going below 2 levels is not really encouraged, like MonkeyPaws.Configuration.Paws, so i understand the frustration. I will change it as requested.

wojciechczerski · 2018-02-11T23:20:26Z

SwiftMonkeyPaws/MonkeyPaws.swift

@@ -112,14 +124,16 @@ public class MonkeyPaws: NSObject, CALayerDelegate {
                gesture.extend(to: point)
            }
        } else {
-            if gestures.count > maxGesturesShown {
+            if gestures.count > self.configuration.paws.maxShown {


self not needed.

My rule of thumb with the usage of self is if a variable declared on the scope of the class, it must have the self prefix. I think this is a good rule, and it helps to avoid inconsistent usage of self.

But i understand you guys have a different coding style, happy to adjust.

This comes more from using SwiftFormat that automatically removes or extra self usages. I must definitely include it somehow in the build process so that the consistency check is done (and applied) automatically.

wojciechczerski · 2018-02-11T23:22:32Z

SwiftMonkeyPaws/MonkeyPaws.swift

@@ -211,9 +241,9 @@ private class Gesture {
        didSet {
            numberLayer.string = String(number)

-            let fraction = Float(number - 1) / Float(maxGesturesShown)
+            let fraction = Float(number - 1) / Float(self.configuration.paws.maxShown)


self not needed.

wojciechczerski · 2018-02-11T23:22:56Z

SwiftMonkeyPaws/MonkeyPaws.swift

            let alpha = sqrt(1 - fraction)
-            containerLayer.opacity = alpha
+            self.containerLayer.opacity = alpha


self not needed.

wojciechczerski · 2018-02-11T23:24:04Z

SwiftMonkeyPaws/MonkeyPaws.swift

@@ -269,7 +299,7 @@ private class Gesture {
        layer.fillColor = nil
        layer.position = at

-        let path = circlePath()
+        let path = circlePath(radius: self.configuration.radius.circle)


self not needed.

wojciechczerski · 2018-02-11T23:24:09Z

SwiftMonkeyPaws/MonkeyPaws.swift

@@ -289,56 +319,34 @@ private class Gesture {
        layer.fillColor = nil
        layer.position = at

-        let path = crossPath()
+        let path = crossPath(radius: self.configuration.radius.cross)


self not needed.

wojciechczerski · 2018-02-13T21:44:16Z

Thank you @dirtydanee for your work and for patiently fixing my picky comments ;) I'll try to somehow automate formatting and style checks in the future, so that it is more convenient when somebody wants to contribute.

In the mean time, expect a new release soon! 😃

Adding customisable visualisation support for the paws.

9d4c143

wojciechczerski mentioned this pull request Feb 8, 2018

Make schemes shared so that they can be built by carthage #56

Merged

wojciechczerski reviewed Feb 11, 2018

View reviewed changes

PR feedback

1d2ea3f

wojciechczerski suggested changes Feb 11, 2018

View reviewed changes

Second round of PR fixes

c40526c

dirtydanee force-pushed the master branch from 4595042 to c40526c Compare February 13, 2018 20:03

wojciechczerski approved these changes Feb 13, 2018

View reviewed changes

wojciechczerski merged commit 17801e7 into zalando:master Feb 13, 2018

wojciechczerski mentioned this pull request Sep 9, 2018

Add more customisability for MonkeyPaws visualisation #9

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding customisable visualisation support for the paws. #57

Adding customisable visualisation support for the paws. #57

dirtydanee commented Feb 4, 2018

wojciechczerski commented Feb 11, 2018

dirtydanee commented Feb 11, 2018

wojciechczerski Feb 11, 2018

wojciechczerski Feb 11, 2018

wojciechczerski Feb 11, 2018

wojciechczerski Feb 11, 2018

wojciechczerski Feb 11, 2018

wojciechczerski Feb 11, 2018

dirtydanee Feb 11, 2018

wojciechczerski Feb 11, 2018

wojciechczerski commented Feb 11, 2018

dirtydanee commented Feb 11, 2018

wojciechczerski Feb 11, 2018

wojciechczerski Feb 11, 2018

wojciechczerski Feb 11, 2018

dirtydanee Feb 13, 2018

wojciechczerski Feb 11, 2018

wojciechczerski Feb 11, 2018

dirtydanee Feb 13, 2018 •

edited

Loading

wojciechczerski Feb 11, 2018

dirtydanee Feb 13, 2018 •

edited

Loading

wojciechczerski Feb 13, 2018

wojciechczerski Feb 11, 2018

wojciechczerski Feb 11, 2018

wojciechczerski Feb 11, 2018

wojciechczerski Feb 11, 2018

wojciechczerski commented Feb 13, 2018

		public class MonkeyPaws: NSObject, CALayerDelegate {

		public struct Configuration {

Adding customisable visualisation support for the paws. #57

Adding customisable visualisation support for the paws. #57

Conversation

dirtydanee commented Feb 4, 2018

wojciechczerski commented Feb 11, 2018

dirtydanee commented Feb 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojciechczerski commented Feb 11, 2018

dirtydanee commented Feb 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirtydanee Feb 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirtydanee Feb 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojciechczerski commented Feb 13, 2018

dirtydanee Feb 13, 2018 •

edited

Loading

dirtydanee Feb 13, 2018 •

edited

Loading