Make only one view selectable at a time (PE-219) #223

lucianocheng · 2020-01-24T21:33:22Z

Fixes #219:

Consolidates editor state into an object, which is accessible by the editor and all multi-touch listeners. Follows Model-View-Controller pattern.
Changes logic to only select one sticker, text, or emoji at a time.
Updates some formatting.

lucianocheng · 2020-02-04T06:52:12Z

@burhanrashid52 friendly ping on this?

burhanrashid52 · 2020-02-05T08:34:08Z

@lucianocheng Since the PR is big and I am busy with some other work. It will take some time for me to review it.

lucianocheng · 2020-02-05T08:47:45Z

No problem. Thanks for letting me know. ᐧ

…

On Wed, Feb 5, 2020 at 12:34 AM Burhanuddin Rashid ***@***.***> wrote: @lucianocheng <https://github.com/lucianocheng> Since the PR is big and I am busy with some other work. It will take some time for me to review it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#223?email_source=notifications&email_token=AAH2GCTR3T6JITEHCH3VJFDRBJ2YDA5CNFSM4KLMMOZ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEK2SNIA#issuecomment-582297248>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAH2GCTS7KAUXFK4THIKO3LRBJ2YDANCNFSM4KLMMOZQ> .

-- Luciano Cheng Cell: (609) 577-1065 Twitter: @lucianocheng <http://www.twitter.com/lucianocheng> Linkedin: http://www.linkedin.com/in/lucianocheng

burhanrashid52 · 2020-02-21T05:06:48Z

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


+    // Tracked state of user-added views (stickers, emoji, text, etc)
+    public static class ViewState {


We need to move this ViewState class out of the PhotoEditor because it getting bigger.

burhanrashid52 · 2020-02-21T05:07:44Z

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

+
+        // A listener for the image view that helps with the focus view logic.
+        // i.e when you press on an empty space without stickers, it will de-select the focused sticker.
+        class ImageViewListener extends GestureDetector.SimpleOnGestureListener {


We also need to move out this class passing the requires object through constructor.

burhanrashid52 · 2020-02-21T05:09:21Z

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

        }
    }*/

    private void viewUndo(View removedView, ViewType viewType) {
+        final List<View> addedViews = viewState.getAddedViews();


Why do we need List<View>. If we using this to handle undo and redo feature than better to use Stack<View>.

Changed redoViews to a Stack.

Can't do this for addedViews because a view needs to be replaced when editing text, which I do not believe Stack supports.

burhanrashid52

Also, it will be really helpful If you can write some tests for it.

burhanrashid52 · 2020-02-21T05:10:05Z

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

@@ -811,6 +922,8 @@ public void onViewAdd(BrushDrawingView brushDrawingView) {

    @Override
    public void onViewRemoved(BrushDrawingView brushDrawingView) {
+        final List<View> addedViews = viewState.getAddedViews();


Why we are adding this in every callback?

Changed so the logic for accessing addedViews is moved into PhotoEditorViewState.

lucianocheng · 2020-04-26T13:08:30Z

Also, it will be really helpful If you can write some tests for it.

@burhanrashid52 Added a test.

Note that the test will not pass unless the build.gradle file is updated to point at the repository-local PhotoEditor instance rather than the published maven / gradle instance (see app/build.gradle line 26. Let me know how you want this addressed.

lucianocheng · 2020-05-20T06:35:05Z

@burhanrashid52 apologies, friendly ping on this? Want to confirm this approach is correct before I update the 2 or 3 dependent PR's I have for transparent click-through, drag selection delegate, and constant sized-handles.

burhanrashid52 · 2020-05-28T11:01:00Z

The PR is big and it's getting little bit to get head around with the whole PR fix. Will review it soon. Sorry for the delay.

lucianocheng · 2020-07-18T05:15:53Z

@burhanrashid52 ping on this?

lucianocheng · 2020-10-15T17:27:08Z

@burhanrashid52 been 5 months since the last update on this.

burhanrashid52 · 2020-10-17T12:21:00Z

@lucianocheng I am extremely sorry for the delay. Can you please fix the conflicts and I'll merge and release a new version by this week.

burhanrashid52 · 2020-10-19T03:04:44Z

I know I am asking too much. But is it possible to add a flag in the builder to enable disabled this feature? i.e photoEditorBuilder.setMultipleViewSelection(true)

…into PE-219

lucianocheng · 2021-03-31T03:09:12Z

@burhanrashid52 the current espresso test (EditImageActivityTest) uses an android build with the publically published PhotoEditor library rather than the PhotoEditor library local to the repo.

Specifically, app/build.gradle points to 'ja.burhanrashid52:photoeditor:1.1.1' rather than implementation project(':photoeditor')

I made changes to the editor behavior in this PR. In order for my tests to pass, I need a way to run the espresso tests with the PhotoEditor library local to the repo.

Do you have a way you want this done? My first thought is to do it this way [1], by modifying the build.gradle with specific build configurations.

[1] https://developer.android.com/studio/build/gradle-tips#target-specific-builds-with-dependency-configurations

burhanrashid52 · 2021-03-31T11:26:05Z

Yes. I agree. The gradle approach is will more make sense if we are using the multiple flavors of the app which we are not doing it right now.
The simplest approach and what I've seen from other open-source projects is that the sample app for the library depends on the local version of lib rather than the hosted one. This will also reduce one step for the developer to make changes in the library and test it with the sample app.

Action Item

Need to comment on the hosted artifice // implementation ja.burhanrashid52:photoeditor:1.1.1 and use the project(':photoeditor')
Resolve the conflicts.
I've migrated from circle ci to GitHub action. Currently, this PR runs on the circle. Please recreate a new PR to run Github action on it.

burhanrashid52 · 2021-04-02T07:01:32Z

@lucianocheng Are there any updates on this. If you can fix the above point I can merge and release a new version of it.

lucianocheng · 2021-04-02T07:27:28Z

Actively working on it. Should be ready soon

…

On Fri, Apr 2, 2021, 12:01 AM Burhanuddin Rashid ***@***.***> wrote: @lucianocheng <https://github.com/lucianocheng> Are there any updates on this. If you can fix the above point I can merge and release a new version of it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#223 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAH2GCQGK2R55LOZMHUVQ6DTGVTVTANCNFSM4KLMMOZQ> .

burhanrashid52 · 2021-04-10T10:34:38Z

Are there any updates on this?

lucianocheng · 2021-04-13T23:08:31Z

@burhanrashid52 I forgot I need to put in that flag. Still working on it.

…sion 1.1.1

lucianocheng · 2021-04-14T01:04:05Z

@burhanrashid52 ok this flag is harder to wire than I thought, given it's off by default and I need to wire it into the tests.

If we really need it I probably can't get to it until next week.

burhanrashid52 · 2021-04-14T01:06:19Z

So the flag is implemented but we don't have a test for it? Correct?

lucianocheng · 2021-04-14T01:12:48Z

So the flag is implemented but we don't have a test for it? Correct?

@burhanrashid52 Kind of. As it is, tests don't pass since "multiple selection" is on by default, and a test I wrote assumes that "single selection" is on.

To test the new behavior with the flag on, I'd have to set the flag state before initializing the view in the test, which I don't know how to do in the current framework. Alternatively, i'd walk all the state in PhotoEditor / etc after the View is initiated, and set each internal boolean manually.

We can have it on single-selection by default but that kind of defeats the purpose of the flag, since my assumption is you want the current behavior preserved.

The flag itself is also finnicky since the PR includes a refactoring of ViewState. It's just a lot to check. To be confident about the flag I'd need more time to look at all of it.

lucianocheng · 2021-04-14T01:19:14Z

Also just to add a note: The current behavior is not true multiple selection, it just keeps the frame and the close box around each prior-selected not-unselected View.

If I were to implement true multiple selection, I would keep a list of currently selected views and manage them together (whereas in this PR, we track only a single selected view). In the current implementation, this state is not tracked.

IMO, I see this as fixing a bug and not in need of a flag. But I defer to you.

burhanrashid52 · 2021-04-14T10:09:13Z

Let me get back to you on this.

lucianocheng · 2021-04-20T07:38:05Z

@burhanrashid52 any word on this?

burhanrashid52 · 2021-04-20T11:07:07Z

If we want to just show a single selection at a time by showing the helper box then I think the simplest solution would be is, whenever we click on View then first clear all the boxes and then show the helper box on clicked one. Right? If that's the case then we don't need to have ViewState at all.

lucianocheng · 2021-04-20T14:51:35Z

We don't hold on to pointers to all the other boxes, so we have no way to clear them. This is one of the points of adding view state.

…

On Tue, Apr 20, 2021, 4:07 AM Burhanuddin Rashid ***@***.***> wrote: If we want to just show a single selection at a time by showing the helper box then I think the simplest solution would be is, whenever we click on View then first clear all the boxes and then show the helper box on clicked one. Right? If that's the case then we don't need to have ViewState at all. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#223 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAH2GCUTOLTXRG6NABZA6Q3TJVN67ANCNFSM4KLMMOZQ> .

lucianocheng · 2021-04-20T14:56:56Z

It is also non-standard to hold state within UI (view) objects. MVC / MVP best practices and patterns apply here.

burhanrashid52 · 2021-04-21T10:44:40Z

We use clearHelperBox() to get view reference to clear the boxes. Am I missing something here?

lucianocheng · 2021-04-21T13:50:38Z

@burhanrashid52

We use clearHelperBox() to get view reference to clear the boxes. Am I missing something here?

That doesn't help with referencing the view state de-selection by clicking on no sticker (a empty spot), which this class does:

https://github.com/burhanrashid52/PhotoEditor/pull/223/files#diff-a3faf7c6027b1236e577b12be3c9b09d157510e407758c48b6aef6981ed14349R8

This class needs to be able to read the view state, without having a circular pointer back to the PhotoEditor object. All the current "view state", which are the lists of views for undo, such as addedViews, exist on the PhotoEditor object and can't be accessed elsewhere without everyone having a pointer to PhotoEditor.

Going back to Model-View-Controller, this is one of the purposes of the separation between the state (the model) and the view (the UI widgets). Having one live inside the other (as it is in the current codebase with addedViews, etc), limits flexibility for us to add classes such as PhotoEditorImageViewListener that can act on the model (ViewState).

burhanrashid52 · 2021-04-22T09:36:07Z

Okay, what do you suggest? Should we refactor to add this behavior properly? If yes then how the design will look like?
And with this number of changes, it does not look like a bug to me. Is there any better way of resolving this with minimum changes?

lucianocheng · 2021-04-22T12:58:09Z

Okay, what do you suggest? Should we refactor to add this behavior properly? If yes then how the design will look like?

I'm really sorry, I don't know how to answer this question?

This pullrequest ... is my suggestion. The refactoring in this PR ... is how I would refactor to add this behavior properly. What the design would look like ... is the design I wrote in the PR? From what I can tell, I've answered all your questions about the approach, but none of the comments thus far refute my approach on the merits. I've reached this local minima of an approach logically. I've had it in production in my own apps for years.

And with this number of changes, it does not look like a bug to me.

Honestly I don't see the number of changes being that large. Java is a verbose language, there is sometimes a need for something like this if you want to get the behavior right.

Is there any better way of resolving this with minimum changes?

I'm trying to do what's best for the community around this editor and contribute my time and work back for free. But honestly, this work is now from two January's ago. I've done everything you've asked (including adding a non-trivial test) except wire in a flag, which I offered to do it would just delay it further. But if you want to solicit material and non-trivial changes from the community to make this editor match the PhotoEditorSDK, sometimes changes of this size will need to be made.

Candidly, if you want it done a certain way, at this point in time I'll need you to describe it to me exactly. Then commit to taking the change in that state.

If that's not what you want, this is your project and I totally get it. But after 16 months of good faith work and waiting, I think I'll just wish you well and move on to maintaining a public fork instead. That might work better. Then you can pull in the changes as you see fit.

burhanrashid52 · 2021-04-23T06:26:58Z

Yes. my bad. I should have closed this PR in that January month itself. This is one of the lessons I learned it hard way in open source. And I really appreciated you for being there for these 16 months.

The reason behind my last comment was to understand if you have any alternative solution for this. Anyway, I've already approved this PR 3 weeks back. All I needed was some changes to resolve conflicts and to run the CI in GitHub action.

I still need those two things to able to merge this PR.

Resolve conflicts
Created a new PR to run Github action (This is optional for you.)

Thank you for your patience and contribution. Looking forward to merging it.

… of the published maven package, to facilitate local testing

lucianocheng · 2021-04-23T19:50:40Z

@burhanrashid52 👍 . Thank you for understanding.

Conflicts should be resolved.

burhanrashid52 · 2021-04-24T10:29:16Z

@lucianocheng Is it possible for you to create a separate issue or PR to add a flag for this behavior?

lucianocheng · 2021-04-25T00:32:06Z

@lucianocheng Is it possible for you to create a separate issue or PR to add a flag for this behavior?

I would recommend first that we switch to a singleton for flag management. The way the flags work right now, you need the wire the boolean into every class constructor.

burhanrashid52 · 2021-04-25T00:40:40Z

Like a global config. Sounds good to me.

lucianocheng · 2021-04-26T05:37:25Z

@burhanrashid52 found a bug in the "do not allow pinch to scale text" flag. Going to address that first. Will follow up in another issue.

lucianocheng added 2 commits January 23, 2020 17:00

Add ViewState

0131d04

Fix behavior

877361f

lucianocheng requested a review from burhanrashid52 January 24, 2020 21:33

burhanrashid52 reviewed Feb 21, 2020

View reviewed changes

burhanrashid52 requested changes Feb 21, 2020

View reviewed changes

Address comments

832d103

Update remaining files from comments

0f6a8cc

lucianocheng requested review from burhanrashid52 May 1, 2020 06:21

burhanrashid52 changed the base branch from master to release.1.1.0 October 18, 2020 01:23

burhanrashid52 force-pushed the release.1.1.0 branch from 241345e to 6bff748 Compare November 1, 2020 02:42

burhanrashid52 changed the base branch from release.1.1.0 to master March 24, 2021 06:28

lucianocheng changed the title ~~Make only one view selectable at a time.~~ Make only one view selectable at a time (PE-219) Mar 30, 2021

lucianocheng added 2 commits March 29, 2021 23:22

Pull in change from main branch

78e8a18

Merge branch 'master' of git://github.com/burhanrashid52/PhotoEditor …

4245a2a

…into PE-219

burhanrashid52 approved these changes Mar 31, 2021

View reviewed changes

Switch default photoeditor implementation to local from published ver…

d12871d

…sion 1.1.1

lucianocheng added 2 commits April 23, 2021 12:36

Switch to using the local photoeditor library on the demo app instead…

c33ffbc

… of the published maven package, to facilitate local testing

Resolve conflicts

3fc16f8

burhanrashid52 merged commit 644af37 into burhanrashid52:master Apr 24, 2021

lucianocheng deleted the PE-219 branch February 24, 2022 07:54


		// Tracked state of user-added views (stickers, emoji, text, etc)
		public static class ViewState {

Make only one view selectable at a time (PE-219) #223

Make only one view selectable at a time (PE-219) #223

Conversation

lucianocheng commented Jan 24, 2020

lucianocheng commented Feb 4, 2020

burhanrashid52 commented Feb 5, 2020

lucianocheng commented Feb 5, 2020 via email

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucianocheng commented Apr 26, 2020

lucianocheng commented May 20, 2020

burhanrashid52 commented May 28, 2020

lucianocheng commented Jul 18, 2020

lucianocheng commented Oct 15, 2020

burhanrashid52 commented Oct 17, 2020

burhanrashid52 commented Oct 19, 2020 • edited

lucianocheng commented Mar 31, 2021

burhanrashid52 commented Mar 31, 2021

burhanrashid52 commented Apr 2, 2021

lucianocheng commented Apr 2, 2021 via email

burhanrashid52 commented Apr 10, 2021

lucianocheng commented Apr 13, 2021

lucianocheng commented Apr 14, 2021

burhanrashid52 commented Apr 14, 2021

lucianocheng commented Apr 14, 2021 • edited

lucianocheng commented Apr 14, 2021

burhanrashid52 commented Apr 14, 2021

lucianocheng commented Apr 20, 2021

burhanrashid52 commented Apr 20, 2021

lucianocheng commented Apr 20, 2021 via email

lucianocheng commented Apr 20, 2021

burhanrashid52 commented Apr 21, 2021

lucianocheng commented Apr 21, 2021 • edited

burhanrashid52 commented Apr 22, 2021

lucianocheng commented Apr 22, 2021 • edited

burhanrashid52 commented Apr 23, 2021 • edited

lucianocheng commented Apr 23, 2021

burhanrashid52 commented Apr 24, 2021

lucianocheng commented Apr 25, 2021

burhanrashid52 commented Apr 25, 2021

lucianocheng commented Apr 26, 2021

burhanrashid52 commented Oct 19, 2020 •

edited

lucianocheng commented Apr 14, 2021 •

edited

lucianocheng commented Apr 21, 2021 •

edited

lucianocheng commented Apr 22, 2021 •

edited

burhanrashid52 commented Apr 23, 2021 •

edited