Add arrow shape #492
Add arrow shape #492
Conversation
Change meaning of ArrowPointerLocation.START and ArrowPointerLocation.END. START point is first touched point and END is the last touched. This was inverted in initial commit. After few days of testing I realised that it's easier to draw arrow when pointer is placed on arrow's START, so I decided to change default pointer location. Because START and END meaning was changed in the commit, default pointer location value in ShapeType.Arrow() don't have to be changed. Also "pointer location" sounds better than "pointer position" so the name was also changed.
|
@wasky Are you planning any more changes or is it ready for review? |
|
It's ready! |
|
@wasky Need to resolve conflicts. |
| @@ -0,0 +1,3 @@ | |||
| package ja.burhanrashid52.photoeditor.shape | |||
|
|
|||
| enum class ArrowPointerLocation { START, END, BOTH } | |||
There was a problem hiding this comment.
Since this is just one Enum, Can we move this enum to LineShape file?
There was a problem hiding this comment.
As I understand the code correctly, LineShape is rather internal file for the library. We need to have access to ArrowPointerLocation from API so LineShape is not the best place for this enum:
ShapeBuilder.withShapeType(ShapeType.Arrow(ArrowPointerLocation.BOTH))
For that reason I put ArrowPointerLocation in separate file. But maybe I will put this enum in Arrow shape type and rename it to PointerLocation? This would look like this:
ShapeBuilder.withShapeType(ShapeType.Arrow(ShapeType.Arrow.PointerLocation.BOTH))
photoeditor/src/main/java/ja/burhanrashid52/photoeditor/shape/LineShape.kt
Outdated
Show resolved
Hide resolved
| @@ -11,11 +11,6 @@ class EnumTest { | |||
| assertEquals(ViewType.values().size.toLong(), 4) | |||
| } | |||
|
|
|||
| @Test | |||
| fun testNumberOfShapeTypes() { | |||
There was a problem hiding this comment.
Is there an alternative way to check how many shapes we support?
There was a problem hiding this comment.
In new Kotlin version (which I plan to update after #497) when expression must be exhaustive. That means if some shape will be added or removed, there will be compilation error in DrawingView:createShape():141 with message 'when' expression must be exhaustive, add necessary 'NewShapeName' branch or 'else' branch instead or in case of shape removal: Unresolved reference: Brush.
There is a possibility to obtain how many elements has sealed class but we need to use a reflection for this. To use reflection in Kotlin we need org.jetbrains.kotlin:kotlin-reflect dependency.
For this reasons I think this test is no longer needed and not worth the effort of adding new dependency only for this.
# Conflicts: # photoeditor/src/main/java/ja/burhanrashid52/photoeditor/DrawingView.kt
|
@wasky I tested this in my local. It works great!!. Thanks for the contribution you made. I really appreciated it. sealed interface ShapeType {
class Arrow(val pointerLocation: ArrowPointerLocation = ArrowPointerLocation.END) : ShapeType
} |
# Conflicts: # photoeditor/src/main/java/ja/burhanrashid52/photoeditor/DrawingView.kt # photoeditor/src/main/java/ja/burhanrashid52/photoeditor/shape/ShapeBuilder.kt
Thanks! :)
It was intentional. I'm using it for 2 weeks now in my app and I think it's easier with arrow pointer at Considering the above, are you sure do you want me to change default position to Also I don't know what about the |
Let's keep it as it is. If needed, we can point to this conversation.
Oh, I missed that. After reading |
|
So it's ready to merge :) |
No description provided.