Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add background picker dependency to use the phone screen size as aspect ratio for cropping #1743

Closed
wants to merge 1 commit into from

Conversation

Simon-Laux
Copy link
Member

@Simon-Laux Simon-Laux commented Nov 10, 2022

fixes #1502
fixes #1688

that uses the phone screen size as aspect ratio for croping

fixes #1502
fixes #1688
@r10s
Copy link
Member

r10s commented Nov 10, 2022

i see the gist of this pr, and maybe that makes sense.
i try to sum up the discussions from the corresponding issue first: nice that one can select the shown portion in more detail. also nice that selection in landscape is possible as well (ipad) so that this mode is not deteriorated. drawback is that rotating devices is less smart with this pr: the background looks better in the portait/landscape mode that it was selected, it looks worse in the other mode.
so, for the changed functionality i am +/-0, maybe even slightly positive :)

what is more questionable is that we pull in a bigger dependency - even though it is used by bigtech and also looks maintained somehow, there are quite obvious bugs (rotate two times, pixels are lost), and probably some more hidden ones, this can cause quite some work wrt support, bugs etc. also for round-cropping, one idea where we could use the dependency in the future, there are bugs filed.

EDIT: in general, we try to get rid of dependencies - adding new ones should make things much better and i do not see this is the case here.

apart from that, we would have two crop thingies, which is not nice wrt UX and maintainability.

i am not totally sure where the existing crop mechanism comes from and why this crops always squares - but we should at least consider to just tweak that part. maybe we can get the same functionality with much less code and potentially fewer new bugs @cyBerta do you know more about that?

EDIT: after av with @cyBerta: the existing crop seem to be a system component, so i think, it is better to stay with that, that seems to have a better set of tradeoffs.

@Simon-Laux i did not dived through the code in detail - but the rendering inside the viewController is not changed, right?

@Simon-Laux
Copy link
Member Author

there are quite obvious bugs (rotate two times, pixels are lost)

probably because it zooms in to keep the fixed aspect ratio?

apart from that, we would have two crop thingies, which is not nice wrt UX and maintainability.

I plan to replace the other one as well eventually, this lib has even a round one that would fit well with avatar picture selection.

i am not totally sure where the existing crop mechanism comes from and why this crops always squares

it comes from the systems image picker and it is not possible to change the aspect ratio, at least my search resulted in nothing, just people telling it's impossible on stackoverflow.

but the rendering inside the viewController is not changed, right?

it's a modal that is presented on top of the background screen if that is what you mean? though we could eventually do a custom one like telegram that represents a chat screen?

@r10s
Copy link
Member

r10s commented Nov 11, 2022

nb: what whatsapp & co are doing here, is to skip the cropper view completely - and say in the selection view that you can pinch-to-zoom.

@r10s r10s changed the title better background picker, that uses the phone screen size as aspect ratio for croping [postponed] better background picker, that uses the phone screen size as aspect ratio for croping Nov 11, 2022
@r10s
Copy link
Member

r10s commented Dec 8, 2022

I plan to replace the other one as well eventually, this lib has even a round one that would fit well with avatar picture selection.

there are but reports with that as well, TimOliver/TOCropViewController#530

also quite some other bugs sound serious, freeze on ios, m1 building issues - and also the bug of rotating i mentioned above is filed but not commented on since Aug22 (maybe new with some ios update).

so, i do not think, adding this dependency hits the correct set of tradeoffs, as discussed above. using the system library seem to be better here: less bugs, no dependency, more secure, future proof. also, it looks more integrated. in general, we try to get rid of dependencies and add new ones only if it makes things much better - and not better at one point and worse at others.

but it was still good to check the library out, definitely.

@r10s r10s closed this Dec 8, 2022
@r10s r10s changed the title [postponed] better background picker, that uses the phone screen size as aspect ratio for croping [postponed] add background picker dependency to use the phone screen size as aspect ratio for cropping Dec 8, 2022
@r10s r10s changed the title [postponed] add background picker dependency to use the phone screen size as aspect ratio for cropping add background picker dependency to use the phone screen size as aspect ratio for cropping Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

setting chat background crops to square Chat-background-selection is not intuitive
2 participants