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 ability to specify sensor sensitivity (aka ISO) #35

Closed
wants to merge 7 commits into from

Conversation

hagabaka
Copy link
Contributor

@hagabaka hagabaka commented Jun 7, 2017

Part of #21

Yaohan Chen and others added 7 commits May 25, 2017 16:19
It has a Input and Output type, so that it's possible to have
SelectorFunctions based on input other than Collection.

Following advice in RedApparat#21 RedApparat#21 (comment)
Add querying and selection functions
WIP for RedApparat#21
Add API for setting sensor sensitivity (FotoapparatBuilder#sensorSensitivity)
Handle conversion to CaptureRequest and Camera.Parameter
WIP RedApparat#21
@dmitry-zaitsev
Copy link
Member

Thanks! I finish the review tomorrow.

There are things which are different from our writing style (which is, to be fair, not documented anywhere yet), they will need to be changed before we can merge it. I don't expect you to fix any of them, we can do it ourselves as well :)

Copy link
Member

@dmitry-zaitsev dmitry-zaitsev left a comment

Choose a reason for hiding this comment

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

As I said - we are quite strict with our code quality (which often sparks long discussions with @Diolor ;) ). If you feel that it is too much to fix, let me know and we'll handle it ourselves.

lensPositionSelector = selector;
return this;
}

public FotoapparatBuilder sensorSensitivity(SelectorFunction<Range<Integer>, Integer> selector) {
Copy link
Member

Choose a reason for hiding this comment

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

JavaDoc is missing

public FotoapparatBuilder sensorSensitivity(SelectorFunction<Range<Integer>, Integer> selector) {
sensorSensitivitySelector = selector;
return this;
}
Copy link
Member

Choose a reason for hiding this comment

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

Empty line after method is missing

@@ -73,6 +79,13 @@ public static Capabilities empty() {
return flashModes;
}

/**
* @return the sensor sensitivity capability
Copy link
Member

Choose a reason for hiding this comment

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

JavaDoc does not properly explain what the method returns. I would expect something like @return range of supported sensor sensitivities

/**
* @return the sensor sensitivity capability
*/
public Range sensorSensitivityCapability() {
Copy link
Member

Choose a reason for hiding this comment

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

The method name is not descriptive. Would be nice if it would follow the pattern set by other methods. Something like supportedSensorSensitivities

@@ -86,4 +88,52 @@ public Capabilities fromParameters(Camera.Parameters parameters) {
return result;
}

private DiscreetRange extractSupportsSensorSensitivityValue(Camera.Parameters parameters) {
Copy link
Member

Choose a reason for hiding this comment

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

This method does too much. Normally method should not be longer than 20 lines. I suggest to extract a class and make it a dependency of CapabilitiesFactory

Copy link
Member

Choose a reason for hiding this comment

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

The method needs some optimisation. Either class oriented or functional.

abstract public T highest();
abstract public T lowest();

public String toString() {
Copy link
Member

Choose a reason for hiding this comment

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

I would not put it here, because it could be that lowest() and highest() implementations might perform costly operations. So, I would instead generate different toString implementation for DiscreteRange and for ContinuosRange.

return String.format("%1$s to %2$s", lowest(), highest());
}

public static <T> Range<T> emptyRange() {
Copy link
Member

Choose a reason for hiding this comment

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

JavaDoc is missing. I would also name it empty() (because it is already clear from invocation that it is a range: Range.emptyRange()). But +1 for creating this.

return new EmptyRange<>();
}

public static class EmptyRange<T> extends Range<T> {
Copy link
Member

Choose a reason for hiding this comment

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

Let's make it private for now. We can always make it public if we'll need to, but we won't be able to make it private again without breaking clients.

Copy link
Member

Choose a reason for hiding this comment

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

or move to upper level and be package local

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer private

* Created by ychen on 5/24/2017.
*/

public class SensorSensitivitySelectors {
Copy link
Member

Choose a reason for hiding this comment

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

Please add JavaDocs to the class and public methods

@@ -64,4 +68,7 @@ public T get(long timeout,
return new HashSet<>(asList(items));
}

public static <T extends Comparable<T>> Range<T> asRange(T... items) {
Copy link
Member

Choose a reason for hiding this comment

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

JavaDoc. +1 for creating method.

private DiscreetRange extractSupportsSensorSensitivityValue(Camera.Parameters parameters) {
final Set<Integer> isoValuesSet = new HashSet<>();

// Based on https://stackoverflow.com/a/23567103/791323
Copy link
Member

Choose a reason for hiding this comment

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

We now have the knowledge in our code, here. :) 💪 Can be deleted

// Based on https://stackoverflow.com/a/23567103/791323
String flat = parameters.flatten();
String[] isoValues = null;
String values_keyword=null;
Copy link
Member

Choose a reason for hiding this comment

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

also misses some basic code formatting + line below. So whole file

@@ -86,4 +88,52 @@ public Capabilities fromParameters(Camera.Parameters parameters) {
return result;
}

private DiscreetRange extractSupportsSensorSensitivityValue(Camera.Parameters parameters) {
Copy link
Member

Choose a reason for hiding this comment

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

The method needs some optimisation. Either class oriented or functional.

@@ -86,4 +88,52 @@ public Capabilities fromParameters(Camera.Parameters parameters) {
return result;
}

private DiscreetRange extractSupportsSensorSensitivityValue(Camera.Parameters parameters) {
final Set<Integer> isoValuesSet = new HashSet<>();
Copy link
Member

Choose a reason for hiding this comment

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

@dmitry-zaitsev sometimes likes finals. :) For me if they are not there it's also making the code cleaner

return new EmptyRange<>();
}

public static class EmptyRange<T> extends Range<T> {
Copy link
Member

Choose a reason for hiding this comment

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

or move to upper level and be package local

*/
public interface SelectorFunction<T> {
public interface SelectorFunction<Input, Output> {
Copy link
Member

Choose a reason for hiding this comment

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

👍

*/
T select(Collection<T> items);
Output select(Input input);
Copy link
Member

Choose a reason for hiding this comment

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

I am not a big fan of having to add Collection to all our existing selectors. One idea is to add 2 new interfaces RangeSelectorFunction and CollectionsSelectorFunction based on this one. Would help only to reduce the addition of the generics the whole time. cc @dmitry-zaitsev

Copy link
Contributor Author

@hagabaka hagabaka Jun 12, 2017

Choose a reason for hiding this comment

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

I tried doing something like this at first, but had some obstacles. Selectors.firstAvailable would need to work for both types of SelectorFunction, so they would need to be abstract classes extending the same base abstract class. But since select is dependent on the "input" type, it's hard to make this work:

abstract class SelectorFunction<T> {
  public T select(Object input);
}
abstract class RangeSelectorFunction extends SelectorFunction<T> {
  // does not actually override
  public T select(Range<T> input);
}
abstract class CollectionSelectorFunction extends SelectorFunction<T> {
  // does not actually override
  public T select(Collection<T> input);
}

I also tried something like:

abstract class SelectorFunction<Input, T> {
  public T select(Input input);
}
abstract class RangeSelectorFunction<T> extends SelectorFunction<Range<T>, T> {
  public T select(Range<T> input);
}
abstract class CollectionSelectorFunction<T> extends SelectorFunction<Collection<T>, T> {
  public T select(Collection<T> input);
}

But this also did not work. I think the problem was that Java doesn't consider RangeSelectorFunction<Integer> to be a subclass of SelectorFunction<Input, Integer>, but I don't remember the exact error message now.

Copy link
Member

Choose a reason for hiding this comment

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

@hagabaka you are right here, we'll have class hierarchy problems. Let's keep it as you did it since it allows us to reuse the code.

@Diolor
Copy link
Member

Diolor commented Jun 11, 2017

Thanks for the PR! I think most of the comments are quite straight forward and no architectural changes are needed. We can easily work on resolving them 👍 Maybe more test can be added, will check as soon as I check the code in my AS.

@Diolor
Copy link
Member

Diolor commented Jun 11, 2017

Fixes #38

@hagabaka
Copy link
Contributor Author

I've been busy with other projects. It might take two more weeks before I can work on this again.

@Diolor
Copy link
Member

Diolor commented Sep 5, 2017

Thanks @hagabaka ! Will close this since this work was merged with #87

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.

3 participants