Bug fix/40328 Timeout default value #3759
Conversation
WalkthroughThis update enhances various components of the project. Key improvements include better timeout handling and validation in the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- Ginger/Ginger/Actions/ActionEditPage.xaml (1 hunks)
- Ginger/Ginger/Actions/ActionEditPage.xaml.cs (2 hunks)
- Ginger/GingerCoreCommon/GingerCoreCommon.csproj (1 hunks)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (1 hunks)
Files skipped from review due to trivial changes (2)
- Ginger/GingerCoreCommon/GingerCoreCommon.csproj
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs
Additional comments not posted (1)
Ginger/Ginger/Actions/ActionEditPage.xaml (1)
154-154: The changes toxTimeoutTextBoxincluding the updatedToolTipand the addition ofPreviewTextInputevent handler are well-implemented and enhance user experience by providing clearer instructions and preventing invalid input.
| private void xTimeoutTextBox_PreviewTextInput(object sender, TextCompositionEventArgs e) | ||
| { | ||
|
|
||
| bool isEmptyString = string.IsNullOrEmpty(e.Text); | ||
| if (isEmptyString) | ||
| { | ||
| //allow | ||
| e.Handled = false; | ||
| return; | ||
| } | ||
|
|
||
| string currentText = xTimeoutTextBox.Text != null ? xTimeoutTextBox.Text : string.Empty; | ||
| bool isValidInteger = int.TryParse(currentText + e.Text, out _); | ||
| if (isValidInteger) | ||
| { | ||
| //allow | ||
| e.Handled = false; | ||
| return; | ||
| } | ||
| else | ||
| { | ||
| //don't allow | ||
| e.Handled = true; | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Ensure input validation for timeout text box is robust.
The input validation logic for the timeout text box currently allows only integer values, which is good. However, the implementation can be simplified and made more robust by using regular expressions or leveraging existing WPF features for input restrictions. Here's a simplified version using a behavior:
// Add this class in a suitable place
public class NumericInputBehavior : Behavior<TextBox>
{
protected override void OnAttached()
{
base.OnAttached();
AssociatedObject.PreviewTextInput += OnPreviewTextInput;
DataObject.AddPastingHandler(AssociatedObject, OnPaste);
}
protected override void OnDetaching()
{
base.OnDetaching();
AssociatedObject.PreviewTextInput -= OnPreviewTextInput;
DataObject.RemovePastingHandler(AssociatedObject, OnPaste);
}
private void OnPreviewTextInput(object sender, TextCompositionEventArgs e)
{
e.Handled = !IsTextAllowed(e.Text);
}
private void OnPaste(object sender, DataObjectPastingEventArgs e)
{
if (e.DataObject.GetDataPresent(typeof(string)))
{
string text = (string)e.DataObject.GetData(typeof(string));
if (!IsTextAllowed(text))
{
e.CancelCommand();
}
}
else
{
e.CancelCommand();
}
}
private bool IsTextAllowed(string text)
{
return Regex.IsMatch(text, "[^0-9]+");
}
}
// Usage in XAML:
<TextBox Text="{Binding Timeout, UpdateSourceTrigger=PropertyChanged}">
<i:Interaction.Behaviors>
<local:NumericInputBehavior />
</i:Interaction.Behaviors>
</TextBox>This approach not only simplifies the code but also handles copy-paste scenarios where non-numeric values might be pasted into the text box.
| @@ -1668,7 +1668,7 @@ | |||
| { | |||
| if (xTimeoutTextBox.Text == String.Empty || xTimeoutTextBox.Text == null) | |||
| { | |||
| xTimeoutTextBox.Text = "0"; | |||
| xTimeoutTextBox.Text = null; | |||
There was a problem hiding this comment.
Consider handling empty string input more robustly.
The current implementation sets the text to null when it's empty. This might lead to unexpected behavior or errors if other parts of the code expect a string. Consider setting it to an empty string "" instead of null for better safety and consistency.
- xTimeoutTextBox.Text = null;
+ xTimeoutTextBox.Text = "";Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| xTimeoutTextBox.Text = null; | |
| xTimeoutTextBox.Text = ""; |
| @@ -1668,7 +1668,7 @@ private void txtTimeout_TextChanged(object sender, TextChangedEventArgs e) | |||
| { | |||
| if (xTimeoutTextBox.Text == String.Empty || xTimeoutTextBox.Text == null) | |||
| { | |||
| xTimeoutTextBox.Text = "0"; | |||
| xTimeoutTextBox.Text = null; | |||
There was a problem hiding this comment.
Set it to empty instead of null
| return; | ||
| } | ||
|
|
||
| string currentText = xTimeoutTextBox.Text != null ? xTimeoutTextBox.Text : string.Empty; |
There was a problem hiding this comment.
allow setting timeout to empty
There was a problem hiding this comment.
Yes it's allowing-
Basically this code is checking-
Allows input if the input text is empty.
Allows input if the resulting text (current text + new input) forms a valid integer.
Disallows any input that does not result in a valid integer.
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
Ginger/Ginger/Actions/ActionEditPage.xaml (1)
154-154: Enhance tooltip clarity and detail.The tooltip for
xTimeoutTextBoxhas been expanded to provide detailed information about the timeout setting. Ensure that this tooltip is concise yet informative, possibly breaking it down into simpler terms or using bullet points for better readability.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- Ginger/Ginger/Actions/ActionEditPage.xaml (1 hunks)
- Ginger/Ginger/Actions/ActionEditPage.xaml.cs (2 hunks)
Additional comments not posted (1)
Ginger/Ginger/Actions/ActionEditPage.xaml.cs (1)
1654-1654: Consider revising the handling of empty input intxtTimeout_TextChanged.The current implementation sets
xTimeoutTextBox.Texttonullwhen it is empty, which could lead to unexpected behavior or errors if other parts of the code expect a string. Consider setting it to an empty string""instead ofnullfor better safety and consistency. This aligns with previous feedback provided in the PR.- xTimeoutTextBox.Text = null; + xTimeoutTextBox.Text = "";Likely invalid or redundant comment.
|
|
||
| private void xTimeoutTextBox_PreviewTextInput(object sender, TextCompositionEventArgs e) | ||
| { | ||
|
|
||
| bool isEmptyString = string.IsNullOrEmpty(e.Text); | ||
| if (isEmptyString) | ||
| { | ||
| //allow | ||
| e.Handled = false; | ||
| return; | ||
| } | ||
|
|
||
| string currentText = xTimeoutTextBox.Text != null ? xTimeoutTextBox.Text : string.Empty; | ||
| bool isValidInteger = int.TryParse(currentText + e.Text, out _); | ||
| if (isValidInteger) | ||
| { | ||
| //allow | ||
| e.Handled = false; | ||
| return; | ||
| } | ||
| else | ||
| { | ||
| //don't allow | ||
| e.Handled = true; | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Simplify input validation logic in xTimeoutTextBox_PreviewTextInput method.
The method xTimeoutTextBox_PreviewTextInput has a complex flow for validating integer input which can be simplified. Consider using int.TryParse directly on the concatenated string and handle the event based on the result. This removes the need for separate checks and makes the code more concise and readable.
private void xTimeoutTextBox_PreviewTextInput(object sender, TextCompositionEventArgs e)
{
string combinedText = xTimeoutTextBox.Text + e.Text;
e.Handled = !int.TryParse(combinedText, out _); // Directly set handled based on the parse result
}| @@ -151,7 +151,7 @@ | |||
| <Label Content="Wait:" Style="{StaticResource $LabelStyle}" /> | |||
| <Actions:UCValueExpression x:Name="xWaitVeUC" ToolTip="Action will wait this seconds long before start to run" HorizontalAlignment="Stretch" Width="80" /> | |||
| <Label Content="Timeout:" Style="{StaticResource $LabelStyle}" Margin="10,0,0,0"/> | |||
| <TextBox x:Name="xTimeoutTextBox" TextChanged="txtTimeout_TextChanged" ToolTip="Timeout field is in seconds. Default is 30 minutes (1800 seconds). Use 0 for no timeout, allowing actions to run indefinitely. If an action exceeds the specified timeout duration, it will be stopped and marked as failed." Style="{StaticResource @TextBoxStyle}" Width="50" TextAlignment="Center" HorizontalAlignment="Left" /> | |||
| <TextBox x:Name="xTimeoutTextBox" TextChanged="txtTimeout_TextChanged" PreviewTextInput="xTimeoutTextBox_PreviewTextInput" ToolTip="Timeout field is in seconds. Default is 30 minutes (1800 seconds). Use 0 for no timeout, allowing actions to run indefinitely. If an action exceeds the specified timeout duration, it will be stopped and marked as failed." Style="{StaticResource @TextBoxStyle}" Width="50" TextAlignment="Center" HorizontalAlignment="Left" /> | |||
There was a problem hiding this comment.
Review the use of TextChanged event for setting default values.
The TextChanged event handler clears the xTimeoutTextBox text unconditionally. Consider implementing a more robust method for handling default values or empty inputs to avoid potential issues with user interactions.
- <TextBox x:Name="xTimeoutTextBox" TextChanged="txtTimeout_TextChanged" PreviewTextInput="xTimeoutTextBox_PreviewTextInput" ...
+ <TextBox x:Name="xTimeoutTextBox" LostFocus="xTimeoutTextBox_LostFocus" PreviewTextInput="xTimeoutTextBox_PreviewTextInput" ...Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <TextBox x:Name="xTimeoutTextBox" TextChanged="txtTimeout_TextChanged" PreviewTextInput="xTimeoutTextBox_PreviewTextInput" ToolTip="Timeout field is in seconds. Default is 30 minutes (1800 seconds). Use 0 for no timeout, allowing actions to run indefinitely. If an action exceeds the specified timeout duration, it will be stopped and marked as failed." Style="{StaticResource @TextBoxStyle}" Width="50" TextAlignment="Center" HorizontalAlignment="Left" /> | |
| <TextBox x:Name="xTimeoutTextBox" LostFocus="xTimeoutTextBox_LostFocus" PreviewTextInput="xTimeoutTextBox_PreviewTextInput" ToolTip="Timeout field is in seconds. Default is 30 minutes (1800 seconds). Use 0 for no timeout, allowing actions to run indefinitely. If an action exceeds the specified timeout duration, it will be stopped and marked as failed." Style="{StaticResource @TextBoxStyle}" Width="50" TextAlignment="Center" HorizontalAlignment="Left" /> |
Thank you for your contribution.
Before submitting this PR, please make sure:
Summary by CodeRabbit
New Features
Bug Fixes
nullinstead of"0"for clearer state management.Chores