Refactoring #352

burhanrashid52 · 2021-05-18T03:43:15Z

No description provided.

Remove getEmoji method and moved to EmojiBSFragment

burhanrashid52 · 2021-05-19T12:55:48Z

@lucianocheng @tanoDxyz Can you please review it.

tanoDxyz · 2021-05-19T16:48:46Z

@lucianocheng @tanoDxyz Can you please review it.

sure

lucianocheng · 2021-05-19T16:51:43Z

Will look this weekend!

tanoDxyz

All clear

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

tanoDxyz

Two method declarations needs attention

lucianocheng

Took a look. Overall looks good.

I do think we should stop and consider how the helpers and abstract classes should work re: Graphic et al. If we want to add functionality in the future, or begin to expose more things to the user, we should be clear where it should go.

lucianocheng · 2021-05-23T05:50:57Z

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

+ * @author <https://github.com/burhanrashid52>
+ */
+class GraphicHelper {
+ private final ViewGroup mViewGroup;


Clarifying Question: Could you clarify the general purpose of GraphicHelper? By my read, it could be any / all of the below:

To move operational logic out of Graphic abstract class and it's subclasses?

To eventually expose to the user as an interface?

To deal exclusively with the "helper" box?

I like the idea of having a "Graphic" object, but it looks like here we're using both an Abstract class (class Graphic) and a Helper class. This is essentially using both inheritance and composition.

In other projects, I tend to see people stick to either inheritance or composition for a set of types (here: Graphic and it's subclasses). Generally I prefer composition over inheritance (see: Effective Java, Item 16), or inheritance from an interface, but I could see the argument here for either.

Would like more information to render a opinion for the review. Thanks.

My initial thought was the same. I extracted GraphicHelper exclusively to deal with the helper box. But then I thought of giving a generic name that might be used in the future to handle more cases related to these boxes. I understand your point in mixing two concepts In graphics.

Maybe I can move the GraphicHelper outside the Graphics and rename it as BoxHelper. Would it make sense?

I was having two thoughts on whether to keep Graphic as an abstract class or interface. That's why I needed a third-person opinion. I personally like to have a Graphic interface. But we have a repetitive view creation and listener logic that can be converted into Template Pattern using abstract class.

I tried first with the interface and was not happy with the result and hence abstract class made sense to me. Just thinking out loud maybe we can use both abstract and interface Example: Sticker extends Graphics implements IGraphic Thoughts?

Maybe I can move the GraphicHelper outside the Graphics and rename it as BoxHelper. Would it make sense?

Thank you for doing this. BoxHelper is a better name IMO.

I was having two thoughts on whether to keep Graphic as an abstract class or interface. That's why I needed a third-person opinion. I personally like to have a Graphic interface. But we have a repetitive view creation and listener logic that can be converted into Template Pattern using abstract class.

Yep. Speaking strictly for myself, I've found using Abstract classes and a Template Pattern can degrade if the complexity grows. Specifically, if there is a lot of needed functionality, or a very deep tree of types, the abstract classes get cluttered with tons of functionality. Due to the nature of inheritance, the abstract class functionality gets included by default in every subclass. At some point the subclasses diverge on what they expect the superclass / abstract class functions to do, so there's proliferation. In the end it can result in a lot of tangling.

Note this isn't the case if the abstract class is an interface to a 3rd party, where the definitions are fixed and compliance with the template "contract" helps keep the user / library interface clean.

In this case, it's not complex enough to merit more work. Since this is strictly internal, we can switch the approach if it gets out of hand.

I tried first with the interface and was not happy with the result and hence abstract class made sense to me. Just thinking out loud maybe we can use both abstract and interface Example: Sticker extends Graphics implements IGraphic Thoughts?

Since none of this is exposed to the user, I'm fine leaving it either way. If it becomes unwieldy down the line we can switch it up.

Ah forgot to mention one thing: Generally speaking, Template Pattern / Abstract classes are harder to test, since inheritance trees cannot be replaced with mock objects.

Doesn't change my answer from above, just wanted to flag it.

lucianocheng · 2021-05-23T05:54:21Z

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

- });
- }
-
+public interface PhotoEditor {


Question: Is the purpose of turning this into an Interface to use a builder pattern, and expose limited functions to the end-user? Or is it to eventually implement different types of PhotoEditor objects?

The intention is to expose limited functionality to the user and to easily mock/fake out the PhotoEditor in the test if needed

lucianocheng · 2021-05-23T05:59:14Z

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

+ *
+ * @author <https://github.com/burhanrashid52>
+ */
+class PhotoSaverTask extends AsyncTask<String, String, PhotoSaverTask.SaveResult> {


Optional: Probably out of scope, but AsyncTask has been deprecated. On my local branch I switched to using ThreadPools.

https://blog.mindorks.com/threadpoolexecutor-in-android-8e9d22330ee3

Also stops the (highly unlikely) edge case of someone running this twice (in a threadpool, they would be queued).

You are correct. This needs to be changed. The intention was here more of refactoring without changing much of internal logic. We plan this after this release.

lucianocheng · 2021-05-23T06:01:28Z

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

+ return new SaveResult(null, mImagePath, null);
+ } catch (Exception e) {
+ e.printStackTrace();
+ Log.d(TAG, "Failed to save File");


Nit: I would log this as an error (Log.e(...))

lucianocheng · 2021-05-23T06:02:12Z

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

+ out.close();
+ Log.d(TAG, "Filed Saved Successfully");
+ return new SaveResult(null, mImagePath, null);
+ } catch (Exception e) {


Nit: I would narrow this catch block, either by catching IOException or creating a union of exception types using the | operator. Saves you from catching and squashing things like NullPointerException.

lucianocheng · 2021-05-23T06:04:27Z

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

+/**
+ * Created by Burhanuddin Rashid on 18/05/21.
+ *
+ * @author <https://github.com/burhanrashid52>


I'm not sure if we have any tests that execute the saving logic, but this might be a good opportunity to add one.

I research in the past how to achieve this. But could not find a solution for it. The closest thing I found was Facebook Screenshot testing and this Airbnb article.

👍 . Yea might require a lot of mocking to get this to work.

lucianocheng · 2021-05-23T06:10:04Z

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

+ *
+ * @author <https://github.com/burhanrashid52>
+ */
+abstract class Graphic {


Complement: This is a good name for this class.

Question: Is this meant to eventually contain the brush logic as well, or just emoji/text/sticker?

The brush view is created only once and stays on top of the Graphic stack. That cause a few issues #268 . So for now it's for emoji/text/sticker.

lucianocheng · 2021-05-23T06:12:21Z

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

+
+ @Override
+ ViewType getViewType() {
+ return ViewType.IMAGE;


Potentially dumb question: Any reason we're not changing this to be ViewType.STICKER? Out of scope? Maybe this is exposed to the user?

Yes, It's exposed to the user in OnPhotoEditorListener and hence breaks the client code.

lucianocheng · 2021-05-23T06:15:03Z

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

+ private final GraphicManager mGraphicManager;
+ private final ViewGroup mPhotoEditorView;
+ private final PhotoEditorViewState mViewState;
+ private TextView txtText;


Optional: Maybe textGraphicView or graphicTextView as a identifier? or just textView?

lucianocheng · 2021-05-23T06:15:59Z

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

+ @Override
+ void setupView(View rootView) {
+ txtText = rootView.findViewById(R.id.tvPhotoEditorText);
+ if (txtText != null && mDefaultTextTypeface != null) {


Complement: Glad we're adding these null checks. Gives us flexibility in the future to change the XML templates without worrying about breaking the code.

burhanrashid52 · 2021-05-24T01:57:06Z

Took a look. Overall looks good.

I do think we should stop and consider how the helpers and abstract classes should work re: Graphic et al. If we want to add functionality in the future, or begin to expose more things to the user, we should be clear where it should go.

First of all, thanks for the review. I've replied to comments. You can resolve the conversation as per the answer. Happy to continue the discussion on other points.

burhanrashid52 added 11 commits May 14, 2021 08:49

Extract PhotoEditor Interface

66b5107

Remove getEmoji method and moved to EmojiBSFragment

Move java docs to PhotoEditor interface

c4f0cc7

Extracted Sticker

bf233aa

Extracted Emoji

a793d4a

Extracted Text

dc11513

WIP: Extracted Graphic

f7893bd

Extracted Graphic Manager

8bf882c

Move Gesture logic to Graphic

7a4298f

Move brush listener to BrushAdapter

4a83bee

Graphic helper to clear view and helper box

363ae4d

Revert BrushAdapter to Brush State change listener

91fca04

burhanrashid52 marked this pull request as draft May 18, 2021 03:43

burhanrashid52 linked an issue May 18, 2021 that may be closed by this pull request

Refactoring #351

Closed

10 tasks

burhanrashid52 added 2 commits May 19, 2021 07:58

Extracted PhotoSaverTask

106982b

Update sample to add image dynamically

a363695

burhanrashid52 marked this pull request as ready for review May 19, 2021 12:54

tanoDxyz reviewed May 21, 2021

View reviewed changes

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

tanoDxyz reviewed May 21, 2021

View reviewed changes

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

tanoDxyz reviewed May 21, 2021

View reviewed changes

burhanrashid52 added 2 commits May 23, 2021 08:19

#351 Bumped the version to 1.1.3

c3a0134

#351 Remove not null annoation for int param

90de530

lucianocheng requested changes May 23, 2021

View reviewed changes

burhanrashid52 added 2 commits May 24, 2021 07:28

#351 Handle specific exception

92ecf6e

#351 Move GraphicHelper outside and rename it to BoxHelper

e25fcfb

lucianocheng approved these changes May 24, 2021

View reviewed changes

burhanrashid52 merged commit e25fcfb into master May 25, 2021

burhanrashid52 deleted the PE-351 branch June 2, 2021 01:41

burhanrashid52 mentioned this pull request Jul 21, 2021

PE-374 Created an option to do SYNC save instead of only the default ASYNC #381

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring #352

Refactoring #352

burhanrashid52 commented May 18, 2021

burhanrashid52 commented May 19, 2021

tanoDxyz commented May 19, 2021

lucianocheng commented May 19, 2021

tanoDxyz left a comment

tanoDxyz left a comment

lucianocheng left a comment

lucianocheng May 23, 2021

burhanrashid52 May 24, 2021

lucianocheng May 24, 2021

lucianocheng May 24, 2021

lucianocheng May 23, 2021

burhanrashid52 May 24, 2021

lucianocheng May 23, 2021

burhanrashid52 May 24, 2021

lucianocheng May 23, 2021

lucianocheng May 23, 2021

lucianocheng May 23, 2021

burhanrashid52 May 24, 2021

lucianocheng May 24, 2021

lucianocheng May 23, 2021

burhanrashid52 May 24, 2021

lucianocheng May 23, 2021

burhanrashid52 May 24, 2021

lucianocheng May 23, 2021

lucianocheng May 23, 2021

burhanrashid52 commented May 24, 2021 •

edited

Refactoring #352

Refactoring #352

Conversation

burhanrashid52 commented May 18, 2021

burhanrashid52 commented May 19, 2021

tanoDxyz commented May 19, 2021

lucianocheng commented May 19, 2021

tanoDxyz left a comment

Choose a reason for hiding this comment

tanoDxyz left a comment

Choose a reason for hiding this comment

lucianocheng left a 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

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

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

burhanrashid52 commented May 24, 2021 • edited

burhanrashid52 commented May 24, 2021 •

edited