Skip to content

Fix scale “display” percent - #651

Open
ashiagr wants to merge 2 commits into
Yalantis:developfrom
ashiagr:fix-scale-display-text
Open

Fix scale “display” percent#651
ashiagr wants to merge 2 commits into
Yalantis:developfrom
ashiagr:fix-scale-display-text

Conversation

@ashiagr

@ashiagr ashiagr commented May 4, 2020

Copy link
Copy Markdown

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

ashiagr commented Nov 3, 2020

Copy link
Copy Markdown
Author

👋 @shliama , @p1nkydev , @warko-san

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

@shliama

shliama commented Nov 3, 2020

Copy link
Copy Markdown
Contributor

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

@fabio-blanco

fabio-blanco commented Jan 10, 2021

Copy link
Copy Markdown

@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

shliama commented Jan 11, 2021

Copy link
Copy Markdown
Contributor

@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

fabio-blanco commented Jan 17, 2021

Copy link
Copy Markdown

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


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

Copy link
Copy Markdown
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