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

findBestPreviewSizeValue: sorting optimized #795

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@a-ch

a-ch commented Apr 29, 2017

It's more effective to perform sorting on the supportedPreviewSizes list with unsupported values previously removed

findBestPreviewSizeValue: sorting optimized
It's more effective to perform sorting on the supportedPreviewSizes list with unsupported values removed
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Apr 29, 2017

Codecov Report

Merging #795 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #795   +/-   ##
=========================================
  Coverage     75.29%   75.29%           
  Complexity     4085     4085           
=========================================
  Files           250      250           
  Lines         13902    13902           
  Branches       2846     2846           
=========================================
  Hits          10468    10468           
  Misses         2630     2630           
  Partials        804      804

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff7137e...9ea09b6. Read the comment docs.

codecov-io commented Apr 29, 2017

Codecov Report

Merging #795 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #795   +/-   ##
=========================================
  Coverage     75.29%   75.29%           
  Complexity     4085     4085           
=========================================
  Files           250      250           
  Lines         13902    13902           
  Branches       2846     2846           
=========================================
  Hits          10468    10468           
  Misses         2630     2630           
  Partials        804      804

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff7137e...9ea09b6. Read the comment docs.

@srowen

This comment has been minimized.

Show comment
Hide comment
@srowen

srowen Apr 29, 2017

Contributor

Yeah you don't even need to sort then. At that point all that matters is the one with max resolution.
I don't think the sort is expensive and makes the log message that follows a little more orderly.
That said, maybe in some corner case it is better to take an acceptable option that was presented earlier by the device.
If you'll take it the extra logical step yes that's ok with me.

Contributor

srowen commented Apr 29, 2017

Yeah you don't even need to sort then. At that point all that matters is the one with max resolution.
I don't think the sort is expensive and makes the log message that follows a little more orderly.
That said, maybe in some corner case it is better to take an acceptable option that was presented earlier by the device.
If you'll take it the extra logical step yes that's ok with me.

@srowen

This comment has been minimized.

Show comment
Hide comment
@srowen

srowen May 2, 2017

Contributor

@a-ch if you'll update accordingly I'll merge it

Contributor

srowen commented May 2, 2017

@a-ch if you'll update accordingly I'll merge it

@a-ch

This comment has been minimized.

Show comment
Hide comment
@a-ch

a-ch May 2, 2017

Sorry, I don't really get what exactly you're asking me to update in the code. Do you mean that it would be better to remove sorting at all and simply find the max resolution?

a-ch commented May 2, 2017

Sorry, I don't really get what exactly you're asking me to update in the code. Do you mean that it would be better to remove sorting at all and simply find the max resolution?

@srowen

This comment has been minimized.

Show comment
Hide comment
@srowen

srowen May 2, 2017

Contributor

Yes, because at the point the sort happens now, afterwards, only the first element is used. Therefore you can remove the sort entirely and just find the max resolution.

Contributor

srowen commented May 2, 2017

Yes, because at the point the sort happens now, afterwards, only the first element is used. Therefore you can remove the sort entirely and just find the max resolution.

@srowen

This comment has been minimized.

Show comment
Hide comment
@srowen

srowen May 2, 2017

Contributor

Or, I think you don't even need to copy the collection of values and then prune it. You can just add the suitable sizes to the collection. And then, you don't even need to do that -- just track the suitable size with max resolution. I have a PR if you want to take a look

Contributor

srowen commented May 2, 2017

Or, I think you don't even need to copy the collection of values and then prune it. You can just add the suitable sizes to the collection. And then, you don't even need to do that -- just track the suitable size with max resolution. I have a PR if you want to take a look

srowen pushed a commit that referenced this pull request May 4, 2017

@srowen

This comment has been minimized.

Show comment
Hide comment
@srowen

srowen May 4, 2017

Contributor

Pardon, I addressed this directly in 3d0468e

Contributor

srowen commented May 4, 2017

Pardon, I addressed this directly in 3d0468e

@srowen srowen closed this May 4, 2017

@srowen srowen added the enhancement label Aug 16, 2017

@srowen srowen added this to the 3.3.1 milestone Aug 16, 2017

@srowen srowen added the android label Aug 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment