Add basic shapes: oval and rectangle

[stephanepechard]

I propose a basic implementation of adding oval and rectangle to an image. I added this as alternative shapes to the BrushDrawingView (default being the existing free hand). Paths are simply drawn depending on the selected tool.

Maybe you thought about this feature differently, please do comment if I need to revise my implementation.

I updated the demo app and added tests and documentation as well. I attach a view of the new tools in the toolbar and the rendering of such shapes. Icons are kinda trivial but understandable :-)

Resolves #72

[burhanrashid52]

Thanks for PR. I had few things in mind. Let me review it and get back to you.

[burhanrashid52]

First of all thanks for taking time and creating this PR. I really appreciated it.

Initially I had an idea of allowing to add a custom view to the editor using API photoEditor.addCustomView(View view)

And I think we can start this idea with shapes.

My ideas was to have a ShapeView which takes Shape interface and draw the shape based on the implementation.

Pseudo code look like this.

  class ShapeView extends View {
    // All the touch listener and setup goes here
    void setShape(Shape shape)

    void draw(){
      shape.draw();
    }
  }

  interface Shape{
    void draw();
  }

  class Rectangle implements Shape
  class Oval implements Shape

Then we add a addShape(Shape shape) API in PhotoEditor interface which will allow client to add there on custom shape. The library will provide the most common implementation of shapes like you are did Rectangle andOval.

When client add the shape, we will add it into undo/redo stack as well as we do with addText() and addImage()

I would like to not mixup the shape into BrushDrawingView for few reasons.

1. Undo only drawing bursh #268
2. BrushDrawingView will become GOD class if we need to add more shapes.
3. In future we want replace this BrushDrawingView to independent brush drawing item.

The challenge I see is on eraser implementation with each ShapeView.

This was thought. Let me know your thoughts on this.

[stephanepechard]

Hi @burhanrashid52,

I like your proposal, for several reasons. First, it's cleaner, and as you said, I felt that adding this to BrushDrawingView was not the best place to do it. As I said, it was a bit hacky, but a good way to initiate this conversation :-) Then, I like the idea of having individual shape objects to manipulate. I think that this pattern could be the foundations of independent drawing items and help for the eraser implementation you talk about. For me, views added should be individually removable, as the text. As a consequence, it is not "erasable" but deletable, as a whole, which seems more logical, as it is added as a whole as well. Then, we would be able to select them and change their style even after first addition. What do you think of this?

I can try to propose a first implementation of this in the next few days. I'll modify this PR.

[burhanrashid52]

Yeah I like the idea of deletion since it's added as whole. Let's see how first draft look like and then we can discuss more.

[stephanepechard]

I'll begin to work on this as soon as possible.

I have some questions though. I'm not a Canvas specialist, so I'm not sure if I should have a Canvas (and a Path) for each Shape, or if I should have a unique one for all Shapes. The latter seems to prevent ease of creating individual items. What do you think of this?

I didn't dive into your GraphicManager and what it does. Do you think I should use it or should I follow the DrawingView paradigm?

Last question: would you consider starting to introduce Kotlin code in the lib or not? If OK, I'll write new code and classes in Kotlin.

Thanks

[burhanrashid52]

I have some questions though. I'm not a Canvas specialist, so I'm not sure if I should have a Canvas (and a Path) for each Shape, or if I should have a unique one for all Shapes. The latter seems to prevent ease of creating individual items. What do you think of this?

Yes having path for each shape will be more flexible. As I already mentioned that we can have common ShapeView which can take care of basic plumbing like inflating view and creating paints and stuff. And Shape interface will use to draw path which in our case would Rectangle and Oval.

I didn't dive into your GraphicManager and what it does. Do you think I should use it or should I follow the DrawingView paradigm?

GraphicManager is basically used to add/remove and undo/redo graphics. It does not know about the graphic details. For creating view we better use Graphic abstract class. You can take an example of how Sticker , Text and Image internal classes are implemented.

Last question: would you consider starting to introduce Kotlin code in the lib or not? If OK, I'll write new code and classes in Kotlin.

I really want too. But I am avoiding this right now because.

1. The library will need additional kotlin library
2. Need to update java doc for kotlin.
3. Create kotlin friendly API's This requires a lot of work and I de prioritize because of my limit bandwidth.

I would happy if someone wants to take this. #360 Also coming back to this feature we can focus to make this work and release it and then we can start the conversation for Kotlin migration.

[burhanrashid52]

Also the ui tests are failing.

[stephanepechard]

Hi @burhanrashid52,

No problem for not using Kotlin for now, I was just asking. I know it is a bit of work, that can wait for now.

I began to work on the subject, following what we said last week. With a bit more background on this, I now don't think we should have a Canvas for each Shape, but a Path for each Shape instead, all being drawn on a unique Canvas. In case of using multiple Canvas, we would have as many Views, then we should handle children of the parentView and their indexes, which is overkill (or maybe the GraphicManager may help here, I'm not sure). I think it would be simpler to just use a Path per Shape and generalize the current BrushDrawingView (the new ShapeView) to handle all the Shapes (brush could then be a Shape too). Are we on the same page here?

[burhanrashid52]

In case of using multiple Canvas, we would have as many Views, then we should handle children of the parentView and their indexes

I did not get it. Can you elaborate or share a pseudo example.

[stephanepechard]

Here is what I did, which follows what we said earlier:

First is ShapeView:

public class ShapeView extends View {
    private final Paint paint = new Paint();
    private Shape shape;

    private void setupView() {
        paint.setAntiAlias(true); // and so on...
    }

    @Override
    protected void onDraw(Canvas canvas) {
        shape.draw(canvas, paint);
    }

    @Override
    public boolean onTouchEvent(@NonNull MotionEvent event) {
        switch (event.getAction()) {
            case MotionEvent.ACTION_DOWN:
                shape.startShape(touchX, touchY);
            case MotionEvent.ACTION_MOVE:
                shape.moveShape(touchX, touchY);
            case MotionEvent.ACTION_UP:
                shape.stopShape();
        }
        invalidate();
        return true;
    }

    // Setters/Getters
    public void setShape(Shape shape) {
        this.shape = shape;
    }
    public void setSize(float size) {...}
    public void setOpacity(int opacity) {...}
    public void setColor(int color) {...}
}

The Shape interface:

public interface Shape {
    void draw(Canvas canvas, Paint paint);
    void startShape(float x, float y);
    void moveShape(float x, float y);
    void stopShape();
}

An example of Shape:

public class RectangleShape implements Shape {
    private float left, top, right, bottom, lastX, lastY;
    private Path path = new Path();

    @Override
    public void draw(Canvas canvas, Paint paint) {
        canvas.drawPath(path, paint);
    }

    @Override
    public void startShape(float x, float y) {
        left = x;
        top = y;
    }

    @Override
    public void moveShape(float x, float y) {
        path = createRectanglePath();
    }

    private Path createRectanglePath() {
        Path path = new Path();
        path.moveTo(left, top);
        path.lineTo(left, bottom);
        path.lineTo(right, bottom);
        path.lineTo(right, top);
        path.close();
        return path;
    }

    @Override
    public void stopShape() {}
}

Then into PhotoEditorImpl.java, I added the following method:

    @Override
    public void addShape(Shape shape) {
        ShapeView view = new ShapeView(context);
        view.setShape(shape);
        shapeView = view;
        setShapeSize(DEFAULT_BRUSH_SIZE);
        parentView.addView(view);
    }

As I add the ShapeView to the parentView (as it is done for the BrushDrawingView), I have as many View as Shape, and I always draw on the last added one. Therefore my concern is on handling so many Views and their Z positions (if I want to edit the first added one, I have to put it at the front first). In this case, maybe we need the notion of layer. The alternative would be to always use only one Canvas in a unique View, with as many Path as Shape, but then I'm not sure how to add the decoration for pinch and deletion.

It it clearer?

I have updated the PR with my current work, it will help you to understand. I also fixed the tests.

[burhanrashid52]

If add the view the way you have currently implemented then

1. How we draw the shape ? By click an drag like brush ?
2. Can erase the specific part or do i need to undo it ?
3. Can delete the entire shape like we do with sticker and image ?

[burhanrashid52]

If add the view the way you have currently implemented then

1. How we draw the shape ? By click an drag like brush ?

2. Can erase the specific part or do i need to undo it ?

3. Can delete the entire shape like we do with sticker and image ?

@stephanepechard Any updates on this ?

[stephanepechard]

Here are my thoughts on this:

1. How we draw the shape ? By click an drag like brush ?

2. Can erase the specific part or do i need to undo it ?

3. Can delete the entire shape like we do with sticker and image ?

1. Mostly yes, except there's a need for an additional tap (and UI) to select the shape. Default color, size and opacity are the same ;
2. Undo it is more natural. Maybe not all options though (color, opacity, etc.) ;
3. That's what I want to achieve, thus my current interrogations on how to add and handle multiple views. Will probably try the multiple paths option, to ease the global management.

[burhanrashid52]

Here are my thoughts on this:

1. How we draw the shape ? By click an drag like brush ?
2. Can erase the specific part or do i need to undo it ?
3. Can delete the entire shape like we do with sticker and image ?

1. Mostly yes, except there's a need for an additional tap (and UI) to select the shape. Default color, size and opacity are the same ;

2. Undo it is more natural. Maybe not all options though (color, opacity, etc.) ;

3. That's what I want to achieve, thus my current interrogations on how to add and handle multiple views. Will probably try the multiple paths option, to ease the global management.

Sounds good 👍

[stephanepechard]

Hi @burhanrashid52,

Here is a more consistant contribution. I've made the Shapes to be drawn on a unique dedicated ShapeView, as brush is. Each Shape is made of an internal Path and is configured/styled through a ShapeBuilder. To regroup common Shape code, I've added an abstract class that implements the Shape interface. Things could be even tighter around here, but it's not bad already. I've also added a new Shape, to draw lines. It's easy to add one with this implementation, you just need to specify how it is drawn in its own class. Style is handled the same way for all Shapes.

I consider moving the Brush as a Shape in itself and therefore get rid of the BrushDrawingView as it is. What would think of it? It would be simpler to maintain, the Z-order of the views would be proper and the undo-redo feature would be easier to do.

[burhanrashid52]

Here is a more consistant contribution. I've made the Shapes to be drawn on a unique dedicated ShapeView, as brush is. Each Shape is made of an internal Path and is configured/styled through a ShapeBuilder. To regroup common Shape code, I've added an abstract class that implements the Shape interface. Things could be even tighter around here, but it's not bad already. I've also added a new Shape, to draw lines. It's easy to add one with this implementation, you just need to specify how it is drawn in its own class. Style is handled the same way for all Shapes.

Sounds great. Will try to review by this week.

I consider moving the Brush as a Shape in itself and therefore get rid of the BrushDrawingView as it is. What would think of it? It would be simpler to maintain, the Z-order of the views would be proper and the undo-redo feature would be easier to do.

Yeah it make sense. We can treat Brush as independent Shape which will make undo-redo easier and consistent.

[burhanrashid52]

I tested the code and found the undo/redo is not working with shapes. I think after reviewing the code I want to change my opinion on having a separate ShapeView.

Now I see that why you preferred the Free shape in the beginning. I think you were right on keeping the BrushDrawingView same and just add FreeHandShape for drawing free and keep other shapes adding to the same BrushDrawingView.

If we can move the ShapeView logic in BrushDrawingView as you did with your first commit by keeping the shape API same AbstractShape, Shape, Line and ShapeBuilder etc then the undo/redo will just work fine.

The only thing we need to make sure of is the BrushDrawingStateListener emitls callback events properly on drawing other shapes like Line, Oval, and Rectangle.

We don't need to concern about #268 issue I mentioned in the previous comment.

[stephanepechard]

Yes, the merge of both views seem necessary, to keep all features and have a consistant code. I will probably change names of some components though, to reflect new features.

[burhanrashid52]

Yes, the merge of both views seem necessary, to keep all features and have a consistant code. I will probably change names of some components though, to reflect new features.

Great 👍 Looking forward to it. I am excited to release this feature soon 😃

[stephanepechard]

I'm almost done with the creation of a unique View (that I renamed DrawingView) with a functioning undo/redo. I remove the eraser as it was, as it does not reflect the new Shape architecture. I'm currently thinking about if whether we need a brushDrawindMode or not.

[stephanepechard]

I realized that with the new features, the removal of the eraser and some renaming, this new version will break the API. Can we consider updating the version number to 2.0.0 to reflect that?

[stephanepechard]

Here is the commit. Quite a big one again, but we're close to the end I think. Undo-redo performs the same for all Shapes now, and eraser has been erased :-) I fixed the tests as well, but I'll try to find the time to add some more.

[burhanrashid52]

Ah!! We cannot remove the eraser. We need that for erasing with free hand on drawing and shapes. Currently I am not in state to manage big release and breaking changes. We can go the first approach on your first commit as we discussed.

[burhanrashid52]

We need add Eraser back and rename DrawingView to BrushDrawingView which I think will fix most of the breaking API changes.

[stephanepechard]

I can't go back to my first commit, as Shapes are not removable at this point. I'll set the eraser back but renaming DrawingView to BrushDrawingView is not necessary as it not used in the API as is. The only remaining API breaking changes is the removal of setBrushSize, setOpacity, setBrushColor, setBrushEraserSize, getEraserSize, getBrushSize and getBrushColor due to the use of ShapeBuilder now but I can set them back, annotate them as @Deprecated and apply them to the current Shape if any. When it is done, the only change is the addition of updateShape, which is not breaking. What do you think of this strategy?

[burhanrashid52]

I can't go back to my first commit, as Shapes are not removable at this point. I'll set the eraser back but renaming DrawingView to BrushDrawingView is not necessary as it not used in the API as is. The only remaining API breaking changes is the removal of setBrushSize, setOpacity, setBrushColor, setBrushEraserSize, getEraserSize, getBrushSize and getBrushColor due to the use of ShapeBuilder now but I can set them back, annotate them as @Deprecated and apply them to the current Shape if any. When it is done, the only change is the addition of updateShape, which is not breaking. What do you think of this strategy?

Sounds good to me 👍

Also, can we rename updateShape to setCurrentShape or just setShape This makes more sense from API readability. The updateShape sounds like I am updating the existing Shape with different attributes.

[stephanepechard]

No problem with renaming updateShape to setShape, it sounds way better.

[stephanepechard]

Here is a commit with the come back of the eraser and the addition of @Deprecated annotations on some setters (not the getters after all, as they use the new model). Eraser is now an independent dedicated Shape in itself. Therefore, it is not related with the undo/redo feature. Tests should be ok too. Tell me if it's ok to merge this version.

[burhanrashid52]

I've tested the changes and it's working great but found few issues.

1. When we set brush mode it bring the brush in front which is breaking the previous behavior. The DrawringView always stay behind. Reverting parent.useBrushView() will solve the issue.
2. This is not a bug. Removing eraser from undo/redo will also break the previous behavior. I would still prefer to have undo/redo for eraser or add a configurable parameter from PhotoEditor.Builder which allows undo/redo eraser. Example photoEditor.Builder().setUndoRedoForEraser(true/false)

[stephanepechard]

@burhanrashid52 1 was easy. It was a remainder that I forgot to remove. Nicely caught! 2 was a bit more work, but finally, we're back on the original behaviour, as eraser is treated as multiple Shapes, in the stack of Shapes, do undo-redo is functional again.

Tell me if you see anything else.

[burhanrashid52]

@burhanrashid52 1 was easy. It was a remainder that I forgot to remove. Nicely caught! 2 was a bit more work, but finally, we're back on the original behaviour, as eraser is treated as multiple Shapes, in the stack of Shapes, do undo-redo is functional again.

Tell me if you see anything else.

That's it. I will do just one round of testing and will merge it. Thank you so much for this effort and making it all works. 👍