Add basic shapes: oval and rectangle #358

stephanepechard · 2021-06-09T08:07:04Z

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 · 2021-06-09T10:38:06Z

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

burhanrashid52 · 2021-06-10T11:25:53Z

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.

Undo only drawing bursh Undo only drawing bursh #268
BrushDrawingView will become GOD class if we need to add more shapes.
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 · 2021-06-10T11:51:40Z

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 · 2021-06-10T12:03:03Z

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 · 2021-06-10T13:20:35Z

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 · 2021-06-11T11:10:29Z

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.

The library will need additional kotlin library
Need to update java doc for kotlin.
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 · 2021-06-11T11:11:04Z

Also the ui tests are failing.

stephanepechard · 2021-06-15T15:20:38Z

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 · 2021-06-16T10:25:36Z

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 · 2021-06-16T12:10:55Z

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 · 2021-06-22T10:41:05Z

photoeditor/src/main/java/ja/burhanrashid52/photoeditor/ShapeView.java

+import androidx.annotation.Nullable;
+
+
+public class ShapeView extends View {


Need to make package-private. We usually don't expose the internal representation of the view to client.

burhanrashid52 · 2021-06-22T10:42:41Z

photoeditor/src/main/java/ja/burhanrashid52/photoeditor/PhotoEditor.java

-    enum BrushDrawingShape {
-        FREE_HAND, OVAL, RECTANGLE
-    }
+    void addShape(Shape shape);


We can use ShapeBuilder pattern as we did for TextStyleBuilder which provide more flexibility to add more properties to shape. And will also avoid to have this huge interface with too many methods.

photoeditor/src/main/java/ja/burhanrashid52/photoeditor/PhotoEditorImpl.java

photoeditor/src/main/java/ja/burhanrashid52/photoeditor/OvalShape.java

burhanrashid52 · 2021-06-22T10:46:40Z

app/src/main/java/com/burhanrashid52/photoediting/tools/ToolType.java

@@ -7,6 +7,8 @@
 */
 public enum ToolType {
    BRUSH,
+    OVAL,


Can we use just Shape here since this is part of public it will break more often if we add/remove new Shapes

I guess we can ventilate Shape choice directly from the Builder, so why not. Still, here we are on the app that makes use of the library, so it's less important to my opinion.
How would make the Shape selection then? A different enum?

For now we just use ToolType.Shape to indicate a Shape not the shape details. I am fine with another enum as well.

The thing is this enum is tightly coupled with the UI, as it is aligned with the tool selector. I will propose something simple to select the shape in the UI, but it is up to each developer to make its own implementation.

burhanrashid52 · 2021-06-22T10:51:30Z

If add the view the way you have currently implemented then

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

burhanrashid52 · 2021-06-28T10:19:33Z

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 · 2021-06-28T10:29:54Z

Here are my thoughts on this:

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

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

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

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 ;
Undo it is more natural. Maybe not all options though (color, opacity, etc.) ;
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 · 2021-06-28T10:32:25Z

Here are my thoughts on this:

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

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

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 👍

+ adapt current demo UI + add a new Shape: Line

stephanepechard · 2021-06-30T13:23:04Z

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 · 2021-07-01T11:04:02Z

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.

photoeditor/src/main/java/ja/burhanrashid52/photoeditor/PhotoEditorImpl.java

burhanrashid52 · 2021-07-03T02:47:56Z

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 · 2021-07-05T08:41:43Z

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 · 2021-07-05T10:04:23Z

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 · 2021-07-05T13:59:40Z

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 · 2021-07-06T07:42:13Z

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?

- BrushDrawingView is now called DrawingView - draw parameters (color, opacity, thickness) goes through a ShapeBuilder - undo-redo performs the same for all Shapes - fixed tests

stephanepechard · 2021-07-06T09:22:39Z

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 · 2021-07-07T11:55:26Z

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 · 2021-07-07T12:00:59Z

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

stephanepechard · 2021-07-07T15:07:54Z

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 · 2021-07-08T02:41:30Z

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 · 2021-07-08T08:10:09Z

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

…ethods

stephanepechard · 2021-07-08T09:05:47Z

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 · 2021-07-09T11:26:09Z

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

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.
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)

…order to undo/redo them as it was primarily

stephanepechard · 2021-07-09T15:05:16Z

@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 · 2021-07-09T15:13:05Z

@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. 👍

stephanepechard added 3 commits June 9, 2021 09:22

PE-72 Add basic shapes (oval and rectangle)

1294c1b

PE-72 Restore free hand form for eraser

1a50ab9

PE-72 Rename form to shape

b6d47ce

PE-72 Create independent Shape objects to be added to the view

38662f2

burhanrashid52 reviewed Jun 22, 2021

View reviewed changes

photoeditor/src/main/java/ja/burhanrashid52/photoeditor/PhotoEditorImpl.java Outdated Show resolved Hide resolved

burhanrashid52 reviewed Jun 22, 2021

View reviewed changes

photoeditor/src/main/java/ja/burhanrashid52/photoeditor/OvalShape.java Outdated Show resolved Hide resolved

burhanrashid52 requested changes Jun 22, 2021

View reviewed changes

PE-72 Create Shapes as independent Paths on a single View

21f4556

+ adapt current demo UI + add a new Shape: Line

burhanrashid52 requested changes Jul 3, 2021

View reviewed changes

photoeditor/src/main/java/ja/burhanrashid52/photoeditor/PhotoEditorImpl.java Outdated Show resolved Hide resolved

PE-72 Transform original brush drawing into a Shape

ff57d31

- BrushDrawingView is now called DrawingView - draw parameters (color, opacity, thickness) goes through a ShapeBuilder - undo-redo performs the same for all Shapes - fixed tests

PE-72 Set the eraser back and set @deprecated annotations to legacy m…

29746ae

…ethods

burhanrashid52 added the 1.5.0 label Jul 9, 2021

stephanepechard added 2 commits July 9, 2021 16:31

PE-72 Remove useBrushView

707004a

PE-72 Reset the eraser as multiple Shapes in the stack of Shapes, in …

c9f6ff7

…order to undo/redo them as it was primarily

PE-72 Fix tests

aee11c1

burhanrashid52 approved these changes Jul 10, 2021

View reviewed changes

burhanrashid52 merged commit 2b47c30 into burhanrashid52:master Jul 10, 2021

burhanrashid52 mentioned this pull request Dec 5, 2022

WIP: Add functionality to track bitmap information and events #484

Closed

		import androidx.annotation.Nullable;


		public class ShapeView extends View {

Add basic shapes: oval and rectangle #358

Add basic shapes: oval and rectangle #358

Conversation

stephanepechard commented Jun 9, 2021 • edited Loading

burhanrashid52 commented Jun 9, 2021

burhanrashid52 commented Jun 10, 2021 • edited Loading

stephanepechard commented Jun 10, 2021

burhanrashid52 commented Jun 10, 2021

stephanepechard commented Jun 10, 2021 • edited Loading

burhanrashid52 commented Jun 11, 2021

burhanrashid52 commented Jun 11, 2021

stephanepechard commented Jun 15, 2021 • edited Loading

burhanrashid52 commented Jun 16, 2021 • edited Loading

stephanepechard commented Jun 16, 2021 • edited Loading

burhanrashid52 Jun 22, 2021

Choose a reason for hiding this comment

burhanrashid52 Jun 22, 2021

Choose a reason for hiding this comment

burhanrashid52 Jun 22, 2021

Choose a reason for hiding this comment

stephanepechard Jun 28, 2021 • edited Loading

Choose a reason for hiding this comment

burhanrashid52 Jun 28, 2021

Choose a reason for hiding this comment

stephanepechard Jun 28, 2021

Choose a reason for hiding this comment

burhanrashid52 commented Jun 22, 2021

burhanrashid52 commented Jun 28, 2021

stephanepechard commented Jun 28, 2021

burhanrashid52 commented Jun 28, 2021

stephanepechard commented Jun 30, 2021

burhanrashid52 commented Jul 1, 2021

burhanrashid52 commented Jul 3, 2021

stephanepechard commented Jul 5, 2021

burhanrashid52 commented Jul 5, 2021

stephanepechard commented Jul 5, 2021 • edited Loading

stephanepechard commented Jul 6, 2021

stephanepechard commented Jul 6, 2021

burhanrashid52 commented Jul 7, 2021

burhanrashid52 commented Jul 7, 2021

stephanepechard commented Jul 7, 2021

burhanrashid52 commented Jul 8, 2021

stephanepechard commented Jul 8, 2021

stephanepechard commented Jul 8, 2021

burhanrashid52 commented Jul 9, 2021

stephanepechard commented Jul 9, 2021

burhanrashid52 commented Jul 9, 2021

stephanepechard commented Jun 9, 2021 •

edited

Loading

burhanrashid52 commented Jun 10, 2021 •

edited

Loading

stephanepechard commented Jun 10, 2021 •

edited

Loading

stephanepechard commented Jun 15, 2021 •

edited

Loading

burhanrashid52 commented Jun 16, 2021 •

edited

Loading

stephanepechard commented Jun 16, 2021 •

edited

Loading

stephanepechard Jun 28, 2021 •

edited

Loading

stephanepechard commented Jul 5, 2021 •

edited

Loading