Reduce costly operations, improve app flow and respond to all inputs on runtime

[loiddy]

Hi, it's me again. Sorry it took me a long time to get back to you and thank you for your patience. To recap, I’m slowly sending you changes to prepare for the image position service.

How to reproduce the issues

I’ve prepped the app and demo for testing. It’s in the first commit so you can run the app at that point and at the end before cleanup and compare. Make sure to read the console too.

Issues this PR fixes

When interacting with the app via a a pointer (mouse, touch, pinch, key) event and nothing has changed, crop is triggered (#571). It shouldn’t.

After a pointer event some unnecessary events, not just crop, are triggered again. They shouldn't.

• Try moving the img, resizing and moving, and see repeated methods in console.

The app doesn’t respond to all input changes that affect how the app is displayed/functions at runtime. It should account for changes in all inputs. Developers can then choose how much they let their users do.

The app can sometimes duplicate operations. In one case it triplicates them. It should be able to respond to however many input changes are sent at the same time and only call the needed operations once.

• Press the Random Test button to test this. Crop is called three times. Pressing reset at this point also calls three crops. Other methods are also duplicated.

Approach taken

Add a isNewPosition method and only doAutoCrop if it is a new position after pointer events. isNewPosition checks for differences in cropper and transform.translate before and after using moveStart and global variables. Fixes the first issue.

For the rest, change the flow of the app in how it responds to changes so operations are not repeated...

Considering that…

Changes that affect how the app is displayed can be divided by the steps they trigger. They are steps that… :

1. change the source of an image.
2. create a new image from a given source and render it.
3. change rules on how the cropper can be rendered.
4. change how the cropper is rendered.
5. change how the image is rendered.

They need to happen in this order, as each step will not be completed properly without the ones before it having been completed at least once at some point.

• Right now there isn't much of a relationship between 4 and 5 but there will be later on when image position service is added.

The new flow is…

ngOnChanges checks for changes that lead to the steps and creates three clear paths to avoid repeating operations:

1. change the source of an image.
   -> return if change in 1 (no loaded img) – end of path one.
2. create a new image from a given source and render it.
   -> return if change in 2 – end of path two.
3. change rules on how the cropper can be rendered.
4. change how the cropper is rendered.
5. change how the image is rendered.
   -> end of path three

A change in 1 and 2 eventually call steps 3-5 in checkImageMaxSizeRecursively. So if we prevent the same methods being called there and in ngOnChanges, there's no repeating operations.

The first two code paths definitely lead to doAutoCrop.

The third however, might not. Similarly to pointer events, create blockers using cropperPositionService.isNewPosition – only checks cropper – and isNewTransform – checks all of transform.

To prevent pointer events triggering steps in ngOnChanges, I added blockers wherever changes in cropper and transform are being dealt with in ngOnChanges. They are the only values pointer events update. I compare previous and current state and block if the same.

Angular's previousValue can't be used for this, as it's a reference to before the pointer event and it needs to reference the state at the end of the pointer event. I reset their settings’ values in doAutoCrop and saved that at the beginning of ngOnChanges to solve this.

• There's a commented out console log where you can test this.

More things you should know

I plan to tweak how static and scaled min and max are set in another PR, so I haven’t changed any of that here.

I tried my best not to add anything that will mean developers have to change the way they’re implementing the app, or any big behaviour changes. These will happen in the next PRs :D

I’ve left some TODOs but I’m happy to delete them. I wrote these and comments to explain possibly weird looking code.

There’s one TODO for you to answer.

I haven't forgotten to change maxSize to either of the options we talked about. I'm leaving that for when we get to the image position service.

[EDIT]: I Realised I'd made two mistakes after sending this. I've fixed them. Ignore the comment about "also covers if hidden" in startPinch in Fixes to input issues. It was silly. pinchStart will never be called if hidden.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[PowerKiKi]

This is not something I would do do myself, but I assume the relatively new Angular Signal could be a nice tool to solve "improve app flow and respond to all inputs". If this PR ever get revisited in the future...

[loiddy]

This is not something I would do do myself, but I assume the relatively new Angular Signal could be a nice tool to solve "improve app flow and respond to all inputs". If this PR ever get revisited in the future...

Thanks for the heads up :) From what I can remember and a quick look I don't think this PR needs it. Looking at state management and signals does sound interesting though so thanks.

@Mawi137 I can see you've been active and nothing here. Is that cos you don't want any of this?

[Mawi137]

I tried to review this PR a few times, but there are a lot of changes and I had trouble wrapping my head around it so I gave up. I just went through the code again. It isn't easy to review it because there are some changes which could have some unwanted side effects and there aren't any tests to verify that (my bad).

I like the idea for the changes, because indeed it seems that not every change of the options triggers a new crop. I'm not sure anyone will change more settings at once, but in case anyone would, it's indeed good that the cropper would take that into account to not crop the image multiple times.

What I don't understand is what checkCropperWithinCropperSizeBounds does when the cropper doesn't have to be reset?

I wouldn't save the cropper in the settings, I'm not sure whether the transform object needs to be there because you can now update it from within the cropper itself which means it needs to be kept up to date in the component and settings. Also, instead of cloning an object when "saving" it ({...this.cropper}), I'd treat it the cropper and transform objects as immutable, so create a new object each time a value changes. Like that the cropper value can be null when not set or defined yet instead of setting all values to 0 (or negative like I did).

When you say you block the pointer events in the ngOnChanges, what do you mean exactly?

[loiddy]

I was worried I had scared you away with so much text and what can look like weird changes. I also found it hard wrapping my head around all of this and communicating it all. So thank you for your determination and patience. Much appreciated :)

What I don't understand is what checkCropperWithinCropperSizeBounds does when the cropper doesn't have to be reset?

• When resetCropper is true, the cropper is positioned in the center of the img and it’s made as big as possible.
• When resetCropper is false, the cropper tries to keep the position it has and it “somewhat" keeps its shape, depending on the situation.

Behaviour examples when resetCropper is false:

• When cropper max/min/static bounds are changed:

  • If the cropper is outside of these, size will be pushed in or out to adapt.
  • If cropper is inside of these, size doesn’t change cos it’s ok.
  • Position stays the same unless the new size made it sit outside the cropper boundaries (outside the img).
  • If so the cropper is pushed inside, changing the position.
    Note: This behaviour currently works cos it new. It shouldn't be breaking how devs are implementing the library now.

• When maintain aspect ratio is set to true:

  • The new cropper size will be calculated using the current cropper size.
  • Position will be respected.
    Note: This is fully implemented in the next PR as it could break how some devs are using the app now, or at least in can be confusing because of resetCropOnAspectRatioChange.
    Note: This is nice cos say you have zoomed, moved and translated to frame whatever you desire. Then you decide to have an aspect ratio, or you want to loop through different aspect ratios. This way you don’t have to reposition each time. You might tweak a bit to make it look nicer, but that’s less hassle.

I think in future PRs it was also involved when rotating and containing within aspect ratio but I’m hoping these two examples are enough for now :)

Is resetCropperPositionAndSize a better name for resetCropper?

I wouldn't save the cropper in the settings, I'm not sure whether the transform object needs to be there because you can now update it from within the cropper itself which means it needs to be kept up to date in the component and settings.

Settings also works as a cache for oldCropper and oldTransform in ngOnChanges. Both will have even more importance when I add image position service. We need to know their previous values to always respect transform origin. If we move them to main, we have to copy them there. Would you like me to do this?

Also, instead of cloning an object when "saving" it ({...this.cropper}), I'd treat it the cropper and transform objects as immutable, so create a new object each time a value changes.

I thought I was creating new immutable objects using the spread operator {…}. I thought it was enough because transform and cropper don’t have sub-objects. Could you give me an example of how to do it? I’m confused.

Like that the cropper value can be null when not set or defined yet instead of setting all values to 0 (or negative like I did).

I don’t think we should set cropper to null or undefined. It’s needed allll over the place. Each time it’s used we’d have to check like we do with transform: (cropper.x1 ?? 0) or (cropper.x1 || 0). Set it to undefined or null and see how many errors appear, how much red there is on your screen haha. It’s scary.

When you say you block the pointer events in the ngOnChanges, what do you mean exactly?

I had to rediscover my own thoughts, could’ve been clearer here sorry.

When dragging the img, a new transform object is created (creating a new reference), which triggers a change in input, which then triggers ngOnChanges, which ends up calling setCssTransform. I stopped the process before the last step by using imagePositionIsNewTransform.

If transform properties were updated when dragging, this “change in input” process would never be triggered and we wouldn’t need the transformChanged output. However transform would have to be initialised in parent (demo), if not there’s undefined errors, also it’s where the “key-reference-linking" happens. Actually, the cropper property behaves like this. So you can compare them.

Changes in move or pinch don't trigger ngOnChanges cos they update cropper keys. A new object is never created. The cropper in parent (demo) and in child (library) share the same key references. There’s also no cropperChanged output.

You can test moving the cropper in the test method in demo. Move x1 by adding 10, a noticeable difference, for example. You’ll see it always works as long as you’re creating a new object in and you've initialised cropper.

BTW I HAVE AN IDEA ABOUT SETTINGS

I was planning on asking you about settings later but it might help now idk. I'm not sure I understand the full purpose of settings so if I misunderstood, apologies in advance.

I read a reply from you in a PR or issue were you said that you were trying to reduce the amount of inputs by using settings, or something like that. If that’s the case, I think you were very close!

You could have:

• Settings input and output, following interface Partial so all keys are optional, CropperSettings aka CropperOptions. Output has all of them, kinda like settings class has now.
• Private data in library – the full initialised settings, like in the settings class now. Calling it data cos it needs a different name and it's a short word – it would appear a gazillion times in the code. It would have an interface of settings where only the truly optional settings for the logic in the library to run are marked as optional.
• The output of settings would always be a deep copy, no shared keys anywhere, to prevent side effects.
• Private data could be an object and there could be a settingsUpdater class with the methods required to update data and clone it.
• Instead of settings input and output it could be settingsToUpdate input and settings output, to be super explicit for devs using the library. Also the interfaces could be Partial< CropperSettings > and (full) CropperSettings, which is even more explicit and nicer to code with (no type checking with ?? or ||).

The pros:

• There would only be one source of settings in the library.
• We could get rid of all the individual settings properties in main and parent, and have only one property.
• Future devs adding new settings to the library would know what to update (Cropper Settings interface and settings initialisation, plus whatever logic they add).
• They would also be forced to use that flow, so differences like the one mentioned in my answer to question 3 (transform vrs cropper) wouldn’t happen.
• Once the process of responding to a user event (pointers or changes in inputs) has finished, and before crop, we can emit the new settings and developers can then do whatever makes them happy with that info. Only one emit. They can update transform and cropper from there.
• Developers don’t have to worry about initialising anything, excluding a source img, unless they purposely want the app to start looking a certain way.

The cons:

• We have to do the copying twice, on input and output.
• It’s a big change

[loiddy]

Forgot to say that we could use the previousValue in ngOnChanges as a cache, but not as the code is now. Also it goes against what you said of creating new objects each time a value changes.

Because a new object is created when dragging the image, previousValue after this is the previous value in demo, not the one generated in the app, which is the one we want to cache to compare with the new value. Remeber ngOnChanges will be triggered after a drag cos it's a new object.

[Mawi137]

Thanks for the clarifications. Can you resolve the merge conflicts? Then I'll read through the code again to see if I have any remarks. After that I can merge and I'll do some improvements to the code as well.

What I meant with the immutable thing is, I see you use the spread operator a lot indeed, but you don't change any of the properties. Normally the rule in Angular is that if you want to change a property, you have to make a copy first. That's not the case at all at the moment and I'm going to fix that. So in your changes you have this: cropper: {...this.cropper} for example. Which is not necessary if we respect that rule. That's not the case right now, so I think it's fine for now. I'll look into getting that sorted once this PR is merged.

Good idea for the settings, in that case the cropper position and transform can indeed be in that object. It should then become be the single source of truth for cropping and to render the HTML. The @inputs and internal mouse moves etc can then modify the settings. Like that we always have the previous and new value. We could do something clever with annotations to track changes in the settings to determine whether the cropper position needs to be recalculated.

[loiddy]

Would you like me to do the settings thing? If so I'd do it after you do the immutable thing. I'm still not 100% I understand that so I'm very excited to see and copy :)

Also, from now on all I have are breaking changes and I'm not sure how to proceed.

The changes are:

• Changes to cropper position service (ready but happy to wait till you've done the immutable thing)
• The settings change if you want me to do it
• New image position service (Still needs brushing up a bit. It works but the code can be cleaner. It will most likely be a brain bender and require time to review. There's a lot of math and manual testing.)

Would you like to go through all the changes and, assuming you like everything, make a major change to version 9.0.0 once it's all okay?

If so, how would you like me to send all of this to you? For example, would you like to create a feature branch and I PR to it each time I complete a section, you review it and then we move to the next one till they're all checked and then you merge?

Or how would you like to do this?

[Mawi137]

Should it be called here? I'd think setCssTransform can be called in the ngOnChanges when the transform property changed, no need to wait for the image to load.

[loiddy]

Right now yeah. When I add the img position service, no. So you can change it if you want. I'll be changing this.setCssTransfom() to this.imagePositionService.transform() in one of the future PRs.

The image position service will make the img transform from the center of the cropper. The center of the cropper will be the transform origin css property. To do this we need to do some math with the cropper position and transform. Any changes to cropper position need to be dealt with first for the math to work.

[Mawi137]

Ok, I'm starting to see all the pieces of the puzzle now.
I don't know how you'll implement it, but my guess is that you'll use ngOnChanges to detect whether the flipH flag was changed for example. Then you calculate the new image position and update the transform object accordingly.
I don't think that is a good idea. If you pass something to the transform @input, that's the state we want, we don't want something to change what we requested.
Instead, I'd add public methods to the cropper which can be used through @ViewChild in the parent component. Methods like, flipHorizontal, flipVertical... They could also take an extra parameter so the user can define whether the image should flip from the image center or cropper center.
That way we don't impose any breaking changes and we have clean API without side effects.
Your image position service probably doesn't have to change, only the way that it is triggered.

[loiddy]

Yes that's how I did it.

I don't think that is a good idea. If you pass something to the transform @input, that's the state we want, we don't want something to change what we requested.

I don't agree with this :D you pass data to the child component then the component does whatever it wants with it (i.e. determines what the state is). If not, why would there be two-way binding? I understand the creating new objects to not share references but sometimes that can work to your advantage. Also, currently both transform and cropper don't follow this, as both are updated within the lib.

Instead, I'd add public methods to the cropper which can be used through @ViewChild in the parent component. Methods like, flipHorizontal, flipVertical...

Sounds awesome! I'll look into it. This way cropper and transform don't need to be in parent settings at all. If I remember correctly, they're the only objects in settings so all the problems mentioned above would be gone. Also they're the only ones that need to be updated by the lib.

They could also take an extra parameter so the user can define whether the image should flip from the image center or cropper center.

haha okay so we do need cropper and transform. Right now this sounds confusing but I'll look into it.

That way we don't impose any breaking changes and we have clean API without side effects.
Your image position service probably doesn't have to change, only the way that it is triggered.

Yeah I think it'll be tweaking too. Also, I'm confused. Doing the settings thing will be a breaking change. THE breaking change. Instead of having all the individual inputs, there'll be one input and one output. All apps using the lib will have to be updated when upgrading.

I'm happy to leave my confusions aside for now if you want. I can wait until after you've done the changes you want to do and maybe things will be clearer then. I've also got to finish preparing the image position service.

[Mawi137]

You are indeed right about using 2-way databinding like that. For the cropper position, if an invalid position is passed, the cropper will correct it and could emit that changed value.
However, in the case of the transform object, I don't find that a very clean way of working. The transform @Input is then misused as a command to trigger a method inside the cropper component.
The transform object should only have 1 responsibility and that is describing the transformation. It shouldn't be a command as well. If so, when flipV or flipH changed, it probably ignores translateH and translateV and overwrites them. If flipV or flipH didn't change, it will use the translateH and translateV properties as is. I find that confusing.
Instead a method can be used as the command which will modify the transform object, so that the transform object itself sticks to its single responsibility. It will also be easier to maintain.

The flip from cropper center is nice if you have zoomed in, but if you are a scale 1 and the dev doesn't allow changing the image position, we still need the flip from image center. If not the image will be partially hidden.

[loiddy]

Okay so I understand what you mean now. Still not fully convinced about the different treatment of cropper and input. Ultimately I'll go with whatever you say, but I think discussing this more now is inefficient because you need to see the image position service. It doesn't quite work how you described.

The transform origin and translate properties are always updated, because these two are what position the img. Okay flip alone might not update these, but it could under some circumstances. I feel like you really need to play with the img position service to see all of the use-cases.

I'm yet to finish it. But I can share what I have so far with you. Would you rather see it in one big chunk or for me to divide it into commits/smaller building blocks? I'm asking cos the latter is more work and it's silly for me to spend time on it if you prefer reading the bigger chunks. :)

[loiddy]

Hi, please ignore this that I asked in my last comment:

Would you rather see it in one big chunk or for me to divide it into commits/smaller building blocks?

I've looked at the code again. It's too complicted to read as one big chunk. I'll do commits that build up the service.

[Mawi137]

Ok, lets work with a feature branch then.

I finished my changes btw. Also added a options input already because with the way it now works, that was pretty simple to do. #637
I extracted many things to the CropperSettings which is now called CropperState. Like that there's a lot less code in the component itself.

[Mawi137]

Would you like me to do the settings thing? If so I'd do it after you do the immutable thing. I'm still not 100% I understand that so I'm very excited to see and copy :)

Also, from now on all I have are breaking changes and I'm not sure how to proceed.

The changes are:

• Changes to cropper position service (ready but happy to wait till you've done the immutable thing)
• The settings change if you want me to do it
• New image position service (Still needs brushing up a bit. It works but the code can be cleaner. It will most likely be a brain bender and require time to review. There's a lot of math and manual testing.)

Would you like to go through all the changes and, assuming you like everything, make a major change to version 9.0.0 once it's all okay?

If so, how would you like me to send all of this to you? For example, would you like to create a feature branch and I PR to it each time I complete a section, you review it and then we move to the next one till they're all checked and then you merge?

Or how would you like to do this?

Ideally each PR can be released, but we can do a major release once everything is ready. So PRs can be to master. If a bugfix needs to be done I can start a branch from the last release tag.

[loiddy]

You forgot to answer the settings question.

Would you like me to do the settings thing? If so I'd do it after you do the immutable thing. I'm still not 100% I understand that so I'm very excited to see and copy :)

[Mawi137]

You forgot to answer the settings question.

Would you like me to do the settings thing? If so I'd do it after you do the immutable thing. I'm still not 100% I understand that so I'm very excited to see and copy :)

Sure, go ahead after I made my changes :)

[loiddy]

Hi,

Sorry for the super late reply. I struggled to find time to work on this. I saw your changes, some I'm not so sure about and some I really like.

I have created a feat branch in my repository in my account. I've started adding things. My idea is to add big commits and then once everything is down and I know it works well divided it into smaller commits with more specific commit messages.

I think everything needs to be down to be able to discuss if one or the other approach is better.

So for the mean time you can have a peak whenever you want :)

[EDIT]: Forgot to say, I'm building it first without some of the things we spoke about (keeping all inputs and transform as a function for example). I'll add them at the end.