FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feat(ios-image-source): standardize quality scale in image-source by sudhanshu-15 · Pull Request #5517 · NativeScript/NativeScript · GitHub

/ NativeScript Public

feat(ios-image-source): standardize quality scale in image-source - #5517

Merged
vakrilov merged 2 commits into
NativeScript:masterfrom
sudhanshu-15:5474-ImageQualityApi-Normalize-For-iOS
Mar 13, 2018
ConversationCommits (2)ChecksFiles changed
Merged

feat(ios-image-source): standardize quality scale in image-source #5517
vakrilov merged 2 commits into
NativeScript:masterfrom
sudhanshu-15:5474-ImageQualityApi-Normalize-For-iOS

Conversation

sudhanshu-15 commented Mar 9, 2018

Copy link
Copy Markdown
Contributor

Normalize quality in saveToFile and toBase64String to follow 0-100 scale - standardize implementation on both platforms

closes #5474

PR Checklist

What is the current behavior?

quality parameter in image-source follows different scales for both the platforms. On iOS it goes from 0.0-1.0 and on Android it goes from 0-100. Developer needs to manually check for platform when using quality.

What is the new behavior?

iOS implementation of image-source toBase64String and saveToFile now normalizes the quality scale so that both the platforms follow the same scale from 0-100. For iOS the values of 0-100 are converted to a scale of 0.0-1.0.

Fixes/Implements/Closes #[5474].

ns-bot commented Mar 9, 2018

Copy link
Copy Markdown

Can one of the admins verify this patch?

2 similar comments

ns-bot commented Mar 9, 2018

Copy link
Copy Markdown

Can one of the admins verify this patch?

ns-bot commented Mar 9, 2018

Copy link
Copy Markdown

Can one of the admins verify this patch?

ghost added the ♥ community PR label Mar 9, 2018

ns-bot commented Mar 9, 2018

Copy link
Copy Markdown

Please sign CLA at http://www.nativescript.org/cla

ns-bot added the cla: no label Mar 9, 2018

sudhanshu-15 commented Mar 9, 2018

Copy link
Copy Markdown
Contributor Author

Updated cla

ns-bot commented Mar 9, 2018

Copy link
Copy Markdown

CLA signature found, happy contributing!

ns-bot added cla: yes and removed cla: no labels Mar 9, 2018
sudhanshu-15 mentioned this pull request Mar 9, 2018
Closed
vakrilov suggested changes Mar 12, 2018
View reviewed changes

vakrilov left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can you add a comment in the definitions filethat the value of quality param should be between 1 and 100

Comment thread tests/app/image-source/image-source-tests.ts Outdated
// >> imagesource-to-base-string
const img = imageSource.fromFile(smallImagePath);
let base64String = img.toBase64String("png", 80);
// << imagesource-to-base-string

vakrilov Mar 12, 2018

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Remove the // << comment

Comment thread tests/app/image-source/image-source-tests.ts Outdated
}

export function testBase64Encode_PNG_WithQuality() {
// >> imagesource-to-base-string

vakrilov Mar 12, 2018

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Remove the // >> comment

Comment thread tests/app/image-source/image-source-tests.ts Outdated
const folder = fs.knownFolders.documents();
const path = fs.path.join(folder.path, "test.png");
const saved = img.saveToFile(path, "png", 70);
// << imagesource-save-to

vakrilov Mar 12, 2018

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Remove the // << comment

Comment thread tests/app/image-source/image-source-tests.ts Outdated
}

export function testSaveToFile_WithQuality() {
// >> imagesource-save-to

vakrilov Mar 12, 2018

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can you remove the // >> ... comments? We add those to extract code snippets that are later used in the docs. They are not related to testing so you don't have to add them.

sudhanshu-15 Mar 12, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Cool, not that makes a lot more sense.

Comment thread tns-core-modules/image-source/image-source.ios.ts Outdated
return false;
}

if (quality != null && quality > 1) {

vakrilov Mar 12, 2018

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can you change this conditional to just if(quality)?
It's better to always expect the value of quality(if such is provided) to be in the range of 1 to 100.
With this code you will get the same result when passing 0.70 and 70 which can be confusing.

sudhanshu-15 Mar 12, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yeah I am on it, wasn't sure what to assume so added a logic like this. But I will make the required changes.

Comment thread tns-core-modules/image-source/image-source.ios.ts Outdated
return res;
}

if (quality != null && quality > 1) {

vakrilov Mar 12, 2018

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can you change this conditional to just if(quality)?

vakrilov commented Mar 12, 2018

Copy link
Copy Markdown
Contributor

Hey @sudhanshu-15, thanks for the awesome PR.
I had some minor comments - can you look at them ;)

vakrilov commented Mar 12, 2018

Copy link
Copy Markdown
Contributor

test

feat(ios-image-source): standardize quality scale in image-source for…
6bb32de 
… both platforms

Normalize quality in saveToFile and toBase64String to follow 0-100 scale - standardize implementation on both platforms

closes NativeScript#5474
feat(ios-image-source): update definitions and fix logic of image-sou…
018ef93 
…rces

update definitions and fix logic of quality in image-sources

closes NativeScript#5474
sudhanshu-15 force-pushed the 5474-ImageQualityApi-Normalize-For-iOS branch from d83d33a to 018ef93 Compare March 13, 2018 05:41

sudhanshu-15 commented Mar 13, 2018

Copy link
Copy Markdown
Contributor Author

@vakrilov I have updated the changes, please let me know if I have missed something. Thanks a lot for all the help. :)

vakrilov commented Mar 13, 2018

Copy link
Copy Markdown
Contributor

test

sudhanshu-15 commented Mar 13, 2018

Copy link
Copy Markdown
Contributor Author

@vakrilov can you tell me where is the build failing?

vakrilov commented Mar 13, 2018

Copy link
Copy Markdown
Contributor

test

vakrilov merged commit 319c153 into NativeScript:master Mar 13, 2018
ghost removed the ♥ community PR label Mar 13, 2018

lock Bot commented Aug 26, 2019

Copy link
Copy Markdown

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

lock Bot locked and limited conversation to collaborators Aug 26, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
vakrilov vakrilov approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ImageQuality API differ on IOS and Android: Use one scale for both

3 participants

Back | FazBrowse Home | New Git URL