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

Fix scale “display” percent #651

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

ashiagr
Copy link

@ashiagr ashiagr commented May 4, 2020

Fixes #650

This PR attempts to fix the scale "display" text such that it displays the scale in which the image is going to be saved.

@ashiagr
Copy link
Author

ashiagr commented Nov 3, 2020

👋 @shliama , @p1nkydev , @warko-san

Could it be merged anytime soon? Is there anything that is putting it on hold?

@shliama
Copy link
Contributor

shliama commented Nov 3, 2020

@ashiagr dunno 🤷‍♂️ I don't have "write" access to this repository anymore.

@fabio-blanco
Copy link

fabio-blanco commented Jan 10, 2021

@ashiagr dunno I don't have "write" access to this repository anymore.

But aren't you the creator of this lib? I see your name mentioned in the sources as the creator of many important classes of it.
Will this project stops receiving support?

@shliama
Copy link
Contributor

shliama commented Jan 11, 2021

@ashiagr Correct, I've created this library while working at Yalantis — it's been almost 5 years ago. I'm fine contributing to the repo occasionally, if given write access.

@fabio-blanco
Copy link

fabio-blanco commented Jan 17, 2021

@ashiagr Correct, I've created this library while working at Yalantis — it's been almost 5 years ago. I'm fine contributing to the repo occasionally, if given write access.

Well... I was struggling with the same problems reported on the issue #668 (that seems to be opened since the middle of 2020 with no interaction from the Yalantis folks) and since I saw no solution I've forked the uCrop repository and made the corrections myself. I've opened a pull request #732. Now I will wait for the Yalantis developers to review my work and if it takes too long for then on doing this, I will simply change my forked repository name, make some changes and keep the source as a different lib as I intend to use it and don't want to keep with a dead repository as a dependency lib on the sources of my app. But that's not what I want to do. I think it is better if the solution emerges from this original repository. Let's see what will happen.

@dmitriy1morozov
Copy link
Contributor

@ashiagr , thank you for the PR. Sorry for such a huge delay in response. I reviewed your code and found one place for improvement. It is better not to show the fractional part in scaleView at all. Due to the floating-point calculations, it is possible to scale to 99.99% sometimes which seems to be a bug. I would suggest using Math.round() and showing only the resulting integer value as a scale.

mTextViewScalePercent.setText(String.format(Locale.getDefault(), "%d%%", (int) (scale * 100)));
float initialScale = mGestureCropImageView.getInitialMinScale();
mTextViewScalePercent.setText(
String.format(Locale.getDefault(), "%.2f%%", (scale/ initialScale * 100))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ashiagr , I would suggest using

            String.format(Locale.getDefault(), "%d%%", Math.round(scale/ initialScale * 100))

This will prevent incorrect value like 99.99% when it should be 100%. Also integer value should be enough to display. Fractional part is redundant.

@@ -566,7 +566,10 @@ private void setAngleTextColor(int textColor) {

private void setScaleText(float scale) {
if (mTextViewScalePercent != null) {
mTextViewScalePercent.setText(String.format(Locale.getDefault(), "%d%%", (int) (scale * 100)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ashiagr , I would suggest using

            String.format(Locale.getDefault(), "%d%%", Math.round(scale/ initialScale * 100))

This will prevent incorrect value like 99.99% when it should be 100%. Also integer value should be enough to display. Fractional part is redundant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scale “display” text appears misleading.
4 participants