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

Properly set the value of the select component in DebugBreakpointWidget #12567

Merged
merged 1 commit into from
Jul 26, 2023

Conversation

pisv
Copy link
Contributor

@pisv pisv commented May 26, 2023

What it does

Fixes #12547 by ensuring that the value of the select component of the DebugBreakpointWidget is set to this.context in the render method.

How to test

Use the steps described in #12547 to verify that the issue is fixed. Check that there are no regressions.

Review checklist

Reminder for reviewers

@pisv pisv changed the title Properly set the value of the selection component in DebugBreakpointWidget Properly set the value of the select component in DebugBreakpointWidget May 26, 2023
@pisv
Copy link
Contributor Author

pisv commented May 26, 2023

Although this fix seems to work fine and is based on the approach that is in line with other similar places like DebugConfigurationSelect and PreferenceSelectInputRenderer, I'd like to note that the issue might also be fixed in a more general way by ensuring that the SelectComponent itself resets its selection in case the defaultValue property changes. This could be done by overriding its componentDidUpdate method:

    override componentDidUpdate(prevProps: Readonly<SelectComponentProps>): void {
        if (this.props.defaultValue !== prevProps.defaultValue && this.value !== this.props.defaultValue) {
            this.value = this.props.defaultValue;
        }
    }

However, the alternative fix seems less safe to me.

@vince-fugnitto vince-fugnitto added the debug issues that related to debug functionality label Jul 18, 2023
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

LGTM 👍 I can confirm the bug on master, and the changes work as expected.

@pisv
Copy link
Contributor Author

pisv commented Jul 19, 2023

@vince-fugnitto Thank you for your review and approval.

@vince-fugnitto vince-fugnitto merged commit a75dd66 into eclipse-theia:master Jul 26, 2023
@pisv pisv deleted the GH-12547 branch July 26, 2023 13:31
@vince-fugnitto vince-fugnitto added this to the 1.40.0 milestone Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debug issues that related to debug functionality
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Setting Hit Count for a breakpoint sometimes sets Expression instead
2 participants