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

[lucianocheng]

Fixes https://github.com/burhanrashid52/PhotoEditor/issues/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]

@burhanrashid52 friendly ping on this?

[burhanrashid52]

@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]

No problem. Thanks for letting me know. ᐧ

On Wed, Feb 5, 2020 at 12:34 AM Burhanuddin Rashid notifications@github.com 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 https://github.com/burhanrashid52/PhotoEditor/pull/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

[lucianocheng]

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]

@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]

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]

@burhanrashid52 ping on this?

[lucianocheng]

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

[burhanrashid52]

@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]

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)

[lucianocheng]

@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]

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

1. Need to comment on the hosted artifice // implementation ja.burhanrashid52:photoeditor:1.1.1 and use the project(':photoeditor')
2. Resolve the conflicts.
3. 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]

@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]

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 https://github.com/burhanrashid52/PhotoEditor/pull/223#issuecomment-812360339, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAH2GCQGK2R55LOZMHUVQ6DTGVTVTANCNFSM4KLMMOZQ .

[burhanrashid52]

Are there any updates on this?

[lucianocheng]

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

[lucianocheng]

@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]

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

[lucianocheng]

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]

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]

Let me get back to you on this.

[lucianocheng]

@burhanrashid52 any word on this?

[burhanrashid52]

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]

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 https://github.com/burhanrashid52/PhotoEditor/pull/223#issuecomment-823186660, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAH2GCUTOLTXRG6NABZA6Q3TJVN67ANCNFSM4KLMMOZQ .

[lucianocheng]

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

[burhanrashid52]

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

[lucianocheng]

@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]

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]

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]

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.

1. Resolve conflicts
2. 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.

[lucianocheng]

@burhanrashid52 👍 . Thank you for understanding.

Conflicts should be resolved.

[burhanrashid52]

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

[lucianocheng]

@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]

Like a global config. Sounds good to me.

[lucianocheng]

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