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: check for invalid configuration in limit settings #638

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import android.graphics.BitmapFactory;
import android.graphics.drawable.BitmapDrawable;
import android.os.Build;
import android.util.Log;
import android.util.AttributeSet;
import android.view.accessibility.AccessibilityEvent;
import android.view.accessibility.AccessibilityManager;
Expand Down Expand Up @@ -104,6 +105,15 @@ private void disableStateListAnimatorIfNeeded() {
updateAll();
}

/*package*/ int getValidProgressValue(int progress) {
if (progress < getLowerLimit()) {
progress = getLowerLimit();
} else if (progress > getUpperLimit()) {
progress = getUpperLimit();
}
return progress;
}

/* package */ void setValue(double value) {
mValue = value;
updateValue();
Expand Down Expand Up @@ -222,16 +232,28 @@ private void updateAll() {
updateValue();
}

/** Update limit based on props limit, max and min */
/** Update limit based on props limit, max and min
* Fallback to upper limit if invalid configuration provided
*/
private void updateLowerLimit() {
double limit = Math.max(mRealLowerLimit, mMinValue);
mLowerLimit = (int) Math.round((limit - mMinValue) / (mMaxValue - mMinValue) * getTotalSteps());
int lowerLimit = (int) Math.round((limit - mMinValue) / (mMaxValue - mMinValue) * getTotalSteps());
if(lowerLimit > mUpperLimit) {
Log.w("Invalid configuration", "reverting lower limit to upper limit");
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't display logs in very different format for the same type of error.
Let's unify all of them to the one proposed in the suggestions for iOS platform (of course considering whether it's for upperLimit or lowerLimit).

}
mLowerLimit = Math.min(lowerLimit, mUpperLimit);
}

/** Update limit based on props limit, max and min */
/** Update limit based on props limit, max and min
* Fallback to lower limit if invalid configuration provided
*/
private void updateUpperLimit() {
double limit = Math.min(mRealUpperLimit, mMaxValue);
mUpperLimit = (int) Math.round((limit - mMinValue) / (mMaxValue - mMinValue) * getTotalSteps());
int upperLimit = (int) Math.round((limit - mMinValue) / (mMaxValue - mMinValue) * getTotalSteps());
if(mLowerLimit > upperLimit) {
Log.w("Invalid configuration", "reverting upper limit to equal to lower limit");
}
mUpperLimit = Math.max(upperLimit, mLowerLimit);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if(mLowerLimit > upperLimit) {
Log.w("Invalid configuration", "reverting upper limit to equal to lower limit");
}
mUpperLimit = Math.max(upperLimit, mLowerLimit);
if (mLowerLimit > upperLimit) {
Log.w("Invalid configuration", "upperLimit < lowerLimit; upperLimit not set");
} else {
mUpperLimit = upperLimit;
}

}

/** Update value only (optimization in case only value is set). */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,8 @@ protected ViewManagerDelegate<ReactSlider> getDelegate() {
public void onProgressChanged(SeekBar seekbar, int progress, boolean fromUser) {
ReactSlider slider = (ReactSlider)seekbar;

if(progress < slider.getLowerLimit()) {
progress = slider.getLowerLimit();
seekbar.setProgress(progress);
} else if (progress > slider.getUpperLimit()) {
progress = slider.getUpperLimit();
seekbar.setProgress(progress);
}
progress = slider.getValidProgressValue(progress);
seekbar.setProgress(progress);

ReactContext reactContext = (ReactContext) seekbar.getContext();
int reactTag = seekbar.getId();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,8 @@ public class ReactSliderManager extends SimpleViewManager<ReactSlider> {
public void onProgressChanged(SeekBar seekbar, int progress, boolean fromUser) {
ReactSlider slider = (ReactSlider)seekbar;

if(progress < slider.getLowerLimit()) {
progress = slider.getLowerLimit();
seekbar.setProgress(progress);
} else if(progress > slider.getUpperLimit()) {
progress = slider.getUpperLimit();
seekbar.setProgress(progress);
}
progress = slider.getValidProgressValue(progress);
seekbar.setProgress(progress);

ReactContext reactContext = (ReactContext) seekbar.getContext();
if(fromUser) {
Expand Down
10 changes: 9 additions & 1 deletion package/ios/RNCSlider.m
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,14 @@ @implementation RNCSlider
bool _maximumTrackImageSet;
}

- (instancetype)init {
if (self = [super init]) {
_upperLimit = FLT_MAX;
_lowerLimit = FLT_MIN;
}
return self;
}

- (instancetype)initWithFrame:(CGRect)frame
{
return [super initWithFrame:frame];
Expand Down Expand Up @@ -46,7 +54,7 @@ - (void)setupAccessibility:(float)value
if (sliderValue && [sliderValue intValue] == 1) {
spokenUnits = [spokenUnits substringToIndex:stringLength-1];
}

self.accessibilityValue = [NSString stringWithFormat:@"%@ %@", sliderValue, spokenUnits];
}
}
Expand Down
44 changes: 27 additions & 17 deletions package/ios/RNCSliderComponentView.mm
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,12 @@ - (instancetype)initWithFrame:(CGRect)frame
forControlEvents:(UIControlEventTouchUpInside |
UIControlEventTouchUpOutside |
UIControlEventTouchCancel)];

UITapGestureRecognizer *tapGesturer;
tapGesturer = [[UITapGestureRecognizer alloc] initWithTarget: self action:@selector(tapHandler:)];
[tapGesturer setNumberOfTapsRequired: 1];
[slider addGestureRecognizer:tapGesturer];

slider.value = (float)defaultProps->value;
self.contentView = slider;
}
Expand All @@ -65,12 +65,12 @@ - (void)tapHandler:(UITapGestureRecognizer *)gesture {
}
RNCSlider *slider = (RNCSlider *)gesture.view;
slider.isSliding = _isSliding;

// Ignore this tap if in the middle of a slide.
if (_isSliding) {
return;
}

if (!slider.tapToSeek) {
return;
}
Expand All @@ -88,14 +88,14 @@ - (void)tapHandler:(UITapGestureRecognizer *)gesture {
}

[slider setValue:[slider discreteValue:value] animated: YES];

std::dynamic_pointer_cast<const RNCSliderEventEmitter>(_eventEmitter)
->onRNCSliderSlidingStart(RNCSliderEventEmitter::OnRNCSliderSlidingStart{.value = static_cast<Float>(slider.lastValue)});

// Trigger onValueChange to address https://github.com/react-native-community/react-native-slider/issues/212
std::dynamic_pointer_cast<const RNCSliderEventEmitter>(_eventEmitter)
->onRNCSliderValueChange(RNCSliderEventEmitter::OnRNCSliderValueChange{.value = static_cast<Float>(slider.value)});

std::dynamic_pointer_cast<const RNCSliderEventEmitter>(_eventEmitter)
->onRNCSliderSlidingComplete(RNCSliderEventEmitter::OnRNCSliderSlidingComplete{.value = static_cast<Float>(slider.value)});
}
Expand All @@ -122,7 +122,7 @@ - (void)sliderTouchEnd:(RNCSlider *)sender
- (void)RNCSendSliderEvent:(RNCSlider *)sender withContinuous:(BOOL)continuous isSlidingStart:(BOOL)isSlidingStart
{
float value = [sender discreteValue:sender.value];

if (value < sender.lowerLimit) {
value = sender.lowerLimit;
[sender setValue:value animated:NO];
Expand All @@ -134,7 +134,7 @@ - (void)RNCSendSliderEvent:(RNCSlider *)sender withContinuous:(BOOL)continuous i
if(!sender.isSliding) {
[sender setValue:value animated:NO];
}

if (continuous) {
if (sender.lastValue != value) {
std::dynamic_pointer_cast<const RNCSliderEventEmitter>(_eventEmitter)
Expand All @@ -150,15 +150,15 @@ - (void)RNCSendSliderEvent:(RNCSlider *)sender withContinuous:(BOOL)continuous i
->onRNCSliderSlidingStart(RNCSliderEventEmitter::OnRNCSliderSlidingStart{.value = static_cast<Float>(value)});
}
}

sender.lastValue = value;
}

- (void)updateProps:(const Props::Shared &)props oldProps:(const Props::Shared &)oldProps
{
const auto &oldScreenProps = *std::static_pointer_cast<const RNCSliderProps>(_props);
const auto &newScreenProps = *std::static_pointer_cast<const RNCSliderProps>(props);

if (oldScreenProps.value != newScreenProps.value) {
if (!slider.isSliding) {
slider.value = newScreenProps.value;
Expand All @@ -176,12 +176,7 @@ - (void)updateProps:(const Props::Shared &)props oldProps:(const Props::Shared &
if (oldScreenProps.maximumValue != newScreenProps.maximumValue) {
[slider setMaximumValue:newScreenProps.maximumValue];
}
if (oldScreenProps.lowerLimit != newScreenProps.lowerLimit) {
slider.lowerLimit = newScreenProps.lowerLimit;
}
if (oldScreenProps.upperLimit != newScreenProps.upperLimit) {
slider.upperLimit = newScreenProps.upperLimit;
}
updateLimits(slider, newScreenProps.lowerLimit, newScreenProps.upperLimit);
if (oldScreenProps.tapToSeek != newScreenProps.tapToSeek) {
slider.tapToSeek = newScreenProps.tapToSeek;
}
Expand Down Expand Up @@ -272,6 +267,21 @@ - (void)loadImageFromImageSource:(ImageSource)source completionBlock:(RNCLoadIma
}
}

void updateLimits(RNCSlider *slider, float newLowerLimit, float newUpperLimit) {
if (slider.lowerLimit != newLowerLimit) {
slider.lowerLimit = newLowerLimit;
}

if (slider.upperLimit != newUpperLimit) {
slider.upperLimit = newUpperLimit;
}

if (slider.lowerLimit > slider.upperLimit) {
NSLog(@"Invalid configuration: lowerLimit > upperLimit, reverting lowerLimit to upperLimit.");
slider.lowerLimit = slider.upperLimit;
}
}

- (void)setInverted:(BOOL)inverted
{
if (inverted) {
Expand Down
22 changes: 20 additions & 2 deletions package/ios/RNCSliderManager.m
Original file line number Diff line number Diff line change
Expand Up @@ -158,8 +158,26 @@ - (void)sliderTouchEnd:(RNCSlider *)sender
RCT_EXPORT_VIEW_PROPERTY(maximumTrackImage, UIImage);
RCT_EXPORT_VIEW_PROPERTY(minimumValue, float);
RCT_EXPORT_VIEW_PROPERTY(maximumValue, float);
RCT_EXPORT_VIEW_PROPERTY(lowerLimit, float);
RCT_EXPORT_VIEW_PROPERTY(upperLimit, float);
RCT_CUSTOM_VIEW_PROPERTY(lowerLimit, float, RNCSlider) {
float lowerLimit = [RCTConvert float:json];

if (lowerLimit > view.upperLimit) {
lowerLimit = view.upperLimit;
NSLog(@"Invalid configuration: reverting upper limit to equal to lower limit")
}

view.lowerLimit = lowerLimit;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (lowerLimit > view.upperLimit) {
lowerLimit = view.upperLimit;
NSLog(@"Invalid configuration: reverting upper limit to equal to lower limit")
}
view.lowerLimit = lowerLimit;
if (upperLimit < view.lowerLimit) {
NSLog(@"Invalid configuration: upperLimit < lowerLimit; lowerLimit not set")
} else {
view.lowerLimit = lowerLimit;
}

}
RCT_CUSTOM_VIEW_PROPERTY(upperLimit, float, RNCSlider) {
float upperLimit = [RCTConvert float:json];

if (upperLimit < view.lowerLimit) {
upperLimit = view.lowerLimit;
NSLog(@"Invalid configuration: reverting lower limit to equal to upper limit")
}

view.upperLimit = upperLimit;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (upperLimit < view.lowerLimit) {
upperLimit = view.lowerLimit;
NSLog(@"Invalid configuration: reverting lower limit to equal to upper limit")
}
view.upperLimit = upperLimit;
if (upperLimit < view.lowerLimit) {
NSLog(@"Invalid configuration: upperLimit < lowerLimit; upperLimit not set")
} else {
view.upperLimit = upperLimit;
}

}
RCT_EXPORT_VIEW_PROPERTY(minimumTrackTintColor, UIColor);
RCT_EXPORT_VIEW_PROPERTY(maximumTrackTintColor, UIColor);
RCT_EXPORT_VIEW_PROPERTY(onRNCSliderValueChange, RCTBubblingEventBlock);
Expand Down
14 changes: 13 additions & 1 deletion package/src/Slider.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, {useState} from 'react';
import React, {useCallback, useEffect, useState} from 'react';
import {
Image,
Platform,
Expand Down Expand Up @@ -281,6 +281,18 @@ const SliderComponent = (
default: constants.LIMIT_MAX_VALUE,
});

const limitCheck = useCallback(() => {
if (lowerLimit >= upperLimit) {
console.warn(
'Invalid configuration: lower limit is supposed to me smaller than upper limit',
);
}
}, [lowerLimit, upperLimit]);

useEffect(() => {
limitCheck();
}, [limitCheck, localProps]);
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to create separate callback for limitCheck and use it only in the effect.
Let's have that single if check and console.warn directly in this effect.
Also, if the callback uses lowerLimit, upperLimit for dependencies then I don't see a reason for using anything different in the dependencies of this effect.


return (
<View
onLayout={(event) => {
Expand Down
Loading