search

burhanrashid52 / PhotoEditor

A Photo Editor library with simple, easy support for image editing using paints,text,filters,emoji and Sticker like stories.
MIT License
4.16k stars 992 forks source link

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

Closed anderbytes closed 2 years ago

anderbytes commented 3 years ago

ASYNC is breaking a lot and probably not everyone would need to save a huge image that would need this async.

So now we let the user decide (with SaveSettings) if he wants to have a SYNC save instead.

anderbytes commented 3 years ago

I really can't understand the reason why the ui-tests are now failing to end.

Also no Github logs to help with this

burhanrashid52 commented 3 years ago

I really can't understand the reason why the ui-tests are now failing to end.

Also no Github logs to help with this

It's not because of the Async code. There is an issue with CI. I am looking into it.

anderbytes commented 3 years ago

Resolves #374

anderbytes commented 3 years ago

I really hope this will be a viable solution. I"ll take a look at the link but usually I'm very bad at multi-threading

anderbytes commented 3 years ago

@burhanrashid52 I just looked into this "Thread Pool Executor" in the URL and it's too much for me, just to replace something that may soon evolve into Coroutines.

ALSO... as this "solution" doesn't remove the ASYNC operation of the equation, new concurrency errors may still happen. My P.R. intended to let devs decide if they want to be sync or async.

So unfortunately I'm sticking to my local repository as it attends perfectly to my needs. Even if it has some "similar" code on the ASYNC and SYNC versions of the saver.

anderbytes commented 3 years ago

ps: as I stated above.... even if someday someone implements this Thread Pool Executor, we will also probably need the "sync" version of the save, so I understand that some similar code right now is viable, and in the future this "duplicity" will be gone when someone replaces the ASYNC Version (PhotoSaverTask) with the Executor.

anderbytes commented 3 years ago

Hi @burhanrashid52 , could you please consider including this in 1.5.1?

There's a BUG on the saving process that can't wait the whole refactor of the async method. At least this PR will let us use the safer version of the saving

burhanrashid52 commented 3 years ago

I think giving an option to save the file in sync will block the UI thread and will create different issues and I've personally experienced this in the past. So what I would like to do is, keep this PR open instead of merging it. We can point to this PR if anyone faces this issue and see if this fix works. If we have enough feedback we can invest some time to fix this with ThreadExecutor or merge this PR .

github-actions[bot] commented 3 years ago

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

github-actions[bot] commented 2 years ago

This PR was closed because it has been stalled for 5 days with no activity.