Skip to content

Conversation

@mikeprosserni
Copy link
Collaborator

@mikeprosserni mikeprosserni commented Jun 5, 2025

What does this Pull Request accomplish?

If setting a value in a streamlit script sent the value update notification, the streamlit script would re-run, potentially setting the value again and causing an infinite loop. We need to prevent this by NOT notifying when StreamlitPanelValueAccessor.set_value() is called.

The SetValueRequest now includes a 'notify' flag. This flag is false when StreamlitPanelValueAccessor.set_value() is called, and is true when StreamlitPanel.set_value() is called.

Why should this Pull Request be merged?

Implements AB#3145075

What testing has been done?

Validated with a debugger that setting a value using StreamlitPanel in a measurement script triggers the _panelEventNotifier.NotifyPanelValueChanged(), but setting a value using StreamlitPanelValueAccessor in the streamlit script does not.

test___panel___set_value___notifies
test___accessor___set_value___does_not_notify

@github-actions
Copy link
Contributor

github-actions bot commented Jun 5, 2025

Test Results

 10 files  ± 0   10 suites  ±0   10s ⏱️ -1s
 67 tests + 2   67 ✅ + 2  0 💤 ±0  0 ❌ ±0 
640 runs  +20  640 ✅ +20  0 💤 ±0  0 ❌ ±0 

Results for commit e5f741c. ± Comparison against base commit f5af8a9.

@mikeprosserni mikeprosserni marked this pull request as ready for review June 5, 2025 20:20
@mikeprosserni mikeprosserni requested a review from csjall as a code owner June 5, 2025 20:20
@mikeprosserni mikeprosserni merged commit d9d7186 into main Jun 6, 2025
14 checks passed
@mikeprosserni mikeprosserni deleted the users/mprosser/techdebt-3145075-notify-less branch June 6, 2025 19:04
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.

4 participants