search

slint-ui / slint

Slint is a declarative GUI toolkit to build native user interfaces for Rust, C++, or JavaScript apps.
https://slint.dev
Other
16.94k stars 568 forks source link

Preview panics with "Recursion detected" when an init callbak queries the geometry indirectly #4390

Closed ogoffart closed 8 months ago

ogoffart commented 8 months ago

I was previewing something like this:

export component TestCase {
    ti := TextInput { text: "hello"; }
    init => { ti.set-selection-offsets(1, 2) }
}
thread 'main' panicked at /home/runner/work/slint/slint/internal/core/properties.rs:486:9:
Recursion detected
stack backtrace:
   0: rust_begin_unwind
             at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/std/src/panicking.rs:645:5
   1: core::panicking::panic_fmt
             at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/core/src/panicking.rs:72:14
   2: i_slint_core::properties::Property<T>::get
   3: core::ops::function::FnOnce::call_once
   4: i_slint_core::properties::alloc_binding_holder::evaluate
   5: i_slint_core::properties::Property<T>::get
   6: core::ops::function::FnOnce::call_once
   7: i_slint_core::properties::alloc_binding_holder::evaluate
   8: i_slint_core::properties::Property<T>::get
   9: core::ops::function::FnOnce::call_once
  10: i_slint_core::properties::alloc_binding_holder::evaluate
  11: i_slint_core::properties::Property<T>::get
  12: core::ops::function::FnOnce::call_once
  13: i_slint_core::properties::alloc_binding_holder::evaluate
  14: i_slint_core::properties::Property<T>::get
  15: core::ops::function::FnOnce::call_once
  16: i_slint_core::properties::alloc_binding_holder::evaluate
  17: i_slint_core::properties::Property<T>::get
  18: slint_lsp::preview::ui::slint_generatedPreviewUi::InnerComponent_empty_9::item_geometry
  19: <slint_lsp::preview::ui::slint_generatedPreviewUi::InnerComponent_empty_9 as const_field_offset::PinnedDrop>::drop::VT::item_geometry
  20: i_slint_core::items::text::TextInput::ime_properties
  21: i_slint_core::items::text::TextInput::set_cursor_position
  22: slint_interpreter::eval::call_builtin_function
  23: slint_interpreter::eval::eval_expression
  24: slint_interpreter::eval::eval_expression
  25: slint_interpreter::dynamic_item_tree::ErasedItemTreeBox::run_setup_code
  26: slint_lsp::preview::set_preview_factory::{{closure}}
  27: i_slint_core::component_factory::ComponentFactory::new::{{closure}}
  28: i_slint_core::items::component_container::ComponentContainer::ensure_updated
  29: core::ops::function::FnOnce::call_once
  30: i_slint_core::properties::alloc_binding_holder::evaluate
  31: i_slint_core::properties::Property<T>::get
  32: core::ops::function::FnOnce::call_once
  33: i_slint_core::properties::alloc_binding_holder::evaluate
  34: i_slint_core::properties::Property<T>::get
  35: core::ops::function::FnOnce::call_once
  36: i_slint_core::properties::alloc_binding_holder::evaluate
  37: i_slint_core::properties::Property<T>::get
  38: core::ops::function::FnOnce::call_once
  39: i_slint_core::properties::alloc_binding_holder::evaluate
  40: i_slint_core::properties::Property<T>::get
  41: core::ops::function::FnOnce::call_once
  42: i_slint_core::properties::alloc_binding_holder::evaluate
  43: i_slint_core::properties::Property<T>::get
  44: core::ops::function::FnOnce::call_once
  45: i_slint_core::properties::alloc_binding_holder::evaluate
  46: i_slint_core::properties::Property<T>::get
  47: <slint_lsp::preview::ui::slint_generatedPreviewUi::InnerPreviewUi as const_field_offset::PinnedDrop>::drop::VT::layout_info
  48: <i_slint_backend_winit::winitwindowadapter::WinitWindowAdapter as i_slint_core::window::WindowAdapter>::update_window_properties
  49: i_slint_core::window::WindowInner::update_window_properties
  50: <slint_lsp::preview::ui::slint_generatedPreviewUi::PreviewUi as i_slint_core::api::ComponentHandle>::show
  51: slint_lsp::preview::load_preview::{{closure}}::{{closure}}
  52: core::ops::function::FnOnce::call_once{{vtable.shim}}
  53: i_slint_backend_winit::event_loop::EventLoopState::process_event
  54: i_slint_backend_winit::event_loop::EventLoopState::run
  55: <i_slint_backend_winit::Backend as i_slint_core::platform::Platform>::run_event_loop
  56: slint_lsp::main

Commit for fix #3950 did a fix that is specific to focus() call because they are common. But other calls from the init callback could have bad effect

tronical commented 8 months ago

The fact that calling set-selection-offsets unconditionally issues an IME update feels wrong to me. The system input method should only get informed if the text input is focused.

I know this is orthogonal, there are still other ways to trigger that recursion - I don’t mean to downplay the more general issue.

I wonder if it’s perhaps wrong to lazily instantiate the component in the container and if instead we should do it immediately.

ogoffart commented 8 months ago

The fact that calling set-selection-offsets unconditionally issues an IME update feels wrong to me.

That is a very good point. We ought to check has-focus before doing so.

I wonder if it’s perhaps wrong to lazily instantiate the component in the container and if instead we should do it immediately.

I was thinking maybe doing it in a change handler or something, after one iteration

ogoffart commented 8 months ago

The test case works now with #4420 So i'm closing this issue, even if i'm sure there are still way to get to recursion and we probably should re-thinnk when to ComponentContainer instentiate its contents.