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.

Sign up for GitHub

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

Jump to bottom

Bug fix/40328 Timeout default value #3759

Merged
merged 3 commits into from
Jun 24, 2024
Merged

Bug fix/40328 Timeout default value #3759

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
20f1a95
ActionTimeoutChanges
GokulBothe Jun 12, 2024
21b5dda
Merge branch 'master' into BugFix/40328_Timeout_DefaultValue
Maheshkale447 Jun 24, 2024
d6c01ad
Merge branch 'master' into BugFix/40328_Timeout_DefaultValue
Maheshkale447 Jun 24, 2024
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
2 changes: 1 addition & 1 deletion Ginger/Ginger/Actions/ActionEditPage.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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" />
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Suggested change
<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" />

</StackPanel>

<Expander x:Name="xRetryExpander" DockPanel.Dock="Top" Margin="0,10,0,0" ExpandDirection="Down" IsExpanded="False" Style="{StaticResource $ExpanderStyle}">
Expand Down
31 changes: 30 additions & 1 deletion Ginger/Ginger/Actions/ActionEditPage.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1651,7 +1651,7 @@ private void txtTimeout_TextChanged(object sender, TextChangedEventArgs e)
{
if (xTimeoutTextBox.Text == String.Empty || xTimeoutTextBox.Text == null)
{
xTimeoutTextBox.Text = "0";
xTimeoutTextBox.Text = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Suggested change
xTimeoutTextBox.Text = null;
xTimeoutTextBox.Text = "";

Copy link
Contributor

Choose a reason for hiding this comment

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

Set it to empty instead of null

xTimeoutTextBox.CaretIndex = 1;
}
}
Expand Down Expand Up @@ -2242,5 +2242,34 @@ protected override void IsVisibleChangedHandler(object sender, DependencyPropert
base.IsVisibleChangedHandler(sender, e);
}
}

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

allow setting timeout to empty

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

bool isValidInteger = int.TryParse(currentText + e.Text, out _);
if (isValidInteger)
{
//allow
e.Handled = false;
return;
}
else
{
//don't allow
e.Handled = true;
}

}
Comment on lines +2246 to +2271
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Comment on lines +2245 to +2271
Copy link
Contributor

Choose a reason for hiding this comment

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

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
}



}
}
Loading