-
Notifications
You must be signed in to change notification settings - Fork 660
interpreter: Support debug-hooks #8097
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
// Copyright © SixtyFPS GmbH <[email protected]> | ||
// SPDX-License-Identifier: GPL-3.0-only OR LicenseRef-Slint-Royalty-free-2.0 OR LicenseRef-Slint-Software-3.0 | ||
|
||
use crate::{dynamic_item_tree, Value}; | ||
|
||
use smol_str::SmolStr; | ||
|
||
use std::pin::Pin; | ||
|
||
pub type DebugHookCallback = Box<dyn Fn(&str, crate::Value) -> Value>; | ||
|
||
pub(crate) fn set_debug_hook_callback( | ||
component: Pin<&dynamic_item_tree::ItemTreeBox>, | ||
func: Option<DebugHookCallback>, | ||
) { | ||
let Some(global_storage) = component.description().compiled_globals.clone() else { | ||
return; | ||
}; | ||
*(global_storage.debug_hook_callback.borrow_mut()) = func; | ||
} | ||
|
||
pub(crate) fn debug_hook_triggered( | ||
component_instance: &dynamic_item_tree::InstanceRef, | ||
id: SmolStr, | ||
value: Value, | ||
) -> Value { | ||
let Some(global_storage) = component_instance.description.compiled_globals.clone() else { | ||
return value; | ||
}; | ||
let callback = global_storage.debug_hook_callback.borrow(); | ||
|
||
if let Some(callback) = callback.as_ref() { | ||
callback(id.as_str(), value) | ||
} else { | ||
value | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -396,7 +396,13 @@ pub fn eval_expression(expression: &Expression, local_context: &mut EvalLocalCon | |
} | ||
} | ||
Expression::EmptyComponentFactory => Value::ComponentFactory(Default::default()), | ||
Expression::DebugHook { expression, .. } => eval_expression(expression, local_context), | ||
Expression::DebugHook { expression, id: _id } => { | ||
let value = eval_expression(expression, local_context); | ||
#[cfg(feature = "internal-highlight")] | ||
let value = crate::debug_hook::debug_hook_triggered(&local_context.component_instance, _id.clone(), value); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm thinking we should not pass the value in the debug_hook. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I absolutely do want to see the unchanged value. That information is super valuable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you elaborate how do you make use of it? Remember that this function can be called several times for all instance of this property. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But the debug-hooks assigned to those instances should have different ids. Not perfect, true, as repeaters will duplicate those, but still pretty valuable. I should really try to get the enclosing components repeater-index (as an Option maybe?) so that it is trivial to see what came trough at least one level of repeaters and what did not. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They will have the same ids. component Foo {
in property <int> hello: 42;
in property <int> something : hello;
Text {
text: something;
}
}
component Bar {
foo1 := Foo {
hello: 85;
}
foo2 := Foo {}
foo3 := Foo { something: -8; }
} If you edit any property of Foo (hello, something or text), you never know if it is the one from foo1, foo2, or foo3. Repeater only makes it worse, having the index won't really help. What is the use of it anyway? What's written in the property editor should be based on what is on the source code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But why do you need it anyway? We shouldn't be computing the value if that's not required. Normally the value are
If you edit that, it will still call And it will create more dependency than needed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This PR will have a side-effect on side-effects one way or the other. We have "real input changes" when the input to the debug-hooked expression changes and we have "override changes" when the override itself changes. If I calculate the "real" value and then check for an override, I trigger too many side effects (on "real input changes" and on "override changes"). If I ask for the override value and do not ask for the real value if there is an override, then I miss side effects (when we have "real input changes" while an override is n effect). Not missing "real input changes" seems more important to me, so I went to calculate the "real value" at all times. When I do that I might as well pass the "real value" to the override call... More information is always better than less information when making a decision:-) I do not expect either to be a problem in the real world. People already try to avoid side-effects. There are not too many in practice, especially not side effects that are not debugging-related. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm still not convinced on how you'd use the value.
What side effects do you miss? I don't understand what you mean with real input changes. Do you have an example?
Yes that's true. That said I still don't understand what you need the value for in the LSP. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
With a debug hook I have two reasons to mark a property as dirty:
My approachI want to make sure see any side-effect triggered by the non-override code, independent of whether an override is in effect or not. For that I need to always evaluate the non-override path. I do get a few spurious side effects triggered when an override is set/changed/removed. Your suggestionYou safe a few cycles by not evaluating the non-override code while an override is in effect. Downside: By not evaluating the non-override path, you do not register the dependencies in that path. So you will not re-evaluate the property if any of these dependencies become dirty. That way you miss side-effects you would see in a run without where no override is ever present. This is probably a theoretical difference only:
In the rare cases where I need to rely on side-effects, I really need those side-effects to be visible and get into trouble if one of them goes missing. So I wanted to err on the side of seeing more side-effects than normal over seeing less than normal. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have already asked many time and you haven't yet provided an answer of what you'd do with this value. (Or why you'd want the re-evaluation to happen) |
||
value | ||
|
||
} | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should go in the
CompilationResult
, because this would apply to all the components. Or even in theCompiler
then you can enable the write compilation config.But sionce it's internal anyway, it should probably not be in the API at all, and just call
crate::debug_hook::set_debug_hook_callback
from the LSP.