Skip to content

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 8 additions & 0 deletions internal/interpreter/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1527,6 +1527,14 @@ impl ComponentInstance {
) -> Vec<(i_slint_compiler::object_tree::ElementRc, usize)> {
crate::highlight::element_node_at_source_code_position(&self.inner, path, offset)
}

/// Set a callback triggered by `Expression::DebugHook``.
#[cfg(feature = "internal-highlight")]
pub fn set_debug_hook_callback(&self, callback: Option<crate::debug_hook::DebugHookCallback>) {
Copy link
Member

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 the Compiler 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.

generativity::make_guard!(guard);
let comp = self.inner.unerase(guard);
crate::debug_hook::set_debug_hook_callback(comp, callback);
}
}

impl ComponentHandle for ComponentInstance {
Expand Down
37 changes: 37 additions & 0 deletions internal/interpreter/debug_hook.rs
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
}
}
2 changes: 1 addition & 1 deletion internal/interpreter/dynamic_item_tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -402,7 +402,7 @@ pub struct ItemTreeDescription<'id> {
pub(crate) popup_menu_description: PopupMenuDescription,

/// The collection of compiled globals
compiled_globals: Option<Rc<CompiledGlobalCollection>>,
pub(crate) compiled_globals: Option<Rc<CompiledGlobalCollection>>,

/// The type loader, which will be available only on the top-most `ItemTreeDescription`.
/// All other `ItemTreeDescription`s have `None` here.
Expand Down
8 changes: 7 additions & 1 deletion internal/interpreter/eval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The 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.
The debug hook can return a Option and only if that is None (default), then we would evaluate the inner expression.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

@ogoffart ogoffart Apr 9, 2025

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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 pure but if they are not this might have side effect (eg)

foo:  {  debug("hello"); return 42; }

If you edit that, it will still call debug("hello") even if it is not supposed to be called.

And it will create more dependency than needed.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

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

I'm still not convinced on how you'd use the value.
I'd like to see it in use in your LSP patch.

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).

What side effects do you miss? I don't understand what you mean with real input changes. Do you have an example?

I do not expect either to be a problem in the real world. People already try to avoid side-effects.

Yes that's true. That said I still don't understand what you need the value for in the LSP.
I'd say it's more efficient to not compute the value and avoid useless dirty tracking.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd say it's more efficient to not compute the value and avoid useless dirty tracking.

With a debug hook I have two reasons to mark a property as dirty:

  • The actual binding that is used in the UI without the debug hook triggers an update. This code may have side-effects.
  • A override is set/changed/removed. No side-effects here:-)

My approach

I 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 suggestion

You 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:

  • Overrides are not going to be that widely applicable. They get removed pretty regularly by setting values.
  • I do not expect overrides to be in effect for very long
  • I seriously hope users do not rely on side-effects too much in the first place

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.

Copy link
Member

Choose a reason for hiding this comment

The 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

}
}
}

Expand Down
10 changes: 9 additions & 1 deletion internal/interpreter/global_component.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ pub struct CompiledGlobalCollection {
/// Map of all exported global singletons and their index in the compiled_globals vector. The key
/// is the normalized name of the global.
pub exported_globals_by_name: BTreeMap<SmolStr, usize>,

#[cfg(feature = "internal-highlight")]
pub(crate) debug_hook_callback: RefCell<Option<crate::debug_hook::DebugHookCallback>>,
}

impl CompiledGlobalCollection {
Expand Down Expand Up @@ -54,7 +57,12 @@ impl CompiledGlobalCollection {
global
})
.collect();
Self { compiled_globals, exported_globals_by_name }
Self {
compiled_globals,
exported_globals_by_name,
#[cfg(feature = "internal-highlight")]
debug_hook_callback: RefCell::new(None),
}
}
}

Expand Down
2 changes: 2 additions & 0 deletions internal/interpreter/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,8 @@ compile_error!(
);

mod api;
#[cfg(feature = "internal-highlight")]
mod debug_hook;
mod dynamic_item_tree;
mod dynamic_type;
mod eval;
Expand Down
Loading