Skip to content

Commit 7a91c63

Browse files
committed
Fix Control lifetime tracking after native parent destruction
- Mark child wrappers as released when libui-ng container destructors destroy native children - Reject direct destroy on controls that still have a Crystal-side parent - Route Control subclass native pointer access through availability checks - Sync Window#on_closing with libui-ng native destroy behavior - Document lifetime rules for parent-owned controls and window close handling - Add specs for recursive release propagation and destroyed-wrapper access
1 parent c242c87 commit 7a91c63

28 files changed

Lines changed: 396 additions & 238 deletions

README.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -550,7 +550,7 @@ For example, when destroy is called on controls such as Window, Box, Grid, Group
550550
In the case of a Window, there are two common situations:
551551

552552
When the user clicks the close (×) button on the title bar
553-
In this case, on_closing is triggered, and Window#destroy is called automatically.
553+
In this case, `on_closing` is triggered. If the callback returns `true`, libui-ng destroys the native window. UIng mirrors that native destruction by marking the `Window` wrapper and all child wrappers as destroyed. Do not call `Window#destroy` from inside `on_closing`; return `true` to allow the close, or `false` to keep the window open.
554554

555555
When on_should_quit is triggered
556556
This means the entire program is about to quit. But here, Window#destroy is not called automatically, so the user needs to call it explicitly.
@@ -560,6 +560,7 @@ Crystal has garbage collection (GC) and provides a finalize hook when objects ar
560560
Therefore, UIng's primary policy is:
561561

562562
- Control objects (`Window`, `Box`, `Grid`, `Tab`, etc.) are managed with explicit `destroy`.
563+
- Child controls are owned by their parent. To remove a child without destroying it, use the container's delete/remove API where available. Destroying a child directly while it still has a parent is not allowed.
563564
- C resources that require strict ordering (for example `Table::Model`) are managed with explicit `free`.
564565
- `finalize` is never treated as a primary cleanup mechanism for order-sensitive resources.
565566

spec/control_lifetime_spec.cr

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
require "./spec_helper"
2+
3+
private class LifetimeControl < UIng::Control
4+
@ref_ptr : Pointer(UIng::LibUI::Button)
5+
6+
def initialize
7+
@ref_ptr = Pointer(UIng::LibUI::Button).null
8+
end
9+
10+
def adopt(parent : UIng::Control) : Nil
11+
take_ownership(parent)
12+
end
13+
14+
def mark_from_parent_destroy : Nil
15+
mark_released_from_parent_destroy
16+
end
17+
18+
def mark_from_native_destroy : Nil
19+
mark_released_from_native_destroy
20+
end
21+
22+
def to_unsafe
23+
ref_ptr
24+
end
25+
end
26+
27+
private class LifetimeContainer < LifetimeControl
28+
@child : UIng::Control?
29+
30+
def initialize(@child : UIng::Control? = nil)
31+
super()
32+
end
33+
34+
def adopt_child(child : LifetimeControl) : Nil
35+
@child = child
36+
child.adopt(self)
37+
end
38+
39+
protected def before_destroy : Nil
40+
@child.try &.mark_released_from_parent_destroy
41+
@child = nil
42+
end
43+
end
44+
45+
describe UIng::Control do
46+
it "marks descendants released when a parent is destroyed natively" do
47+
parent = LifetimeContainer.new
48+
child = LifetimeContainer.new
49+
grandchild = LifetimeControl.new
50+
51+
parent.adopt_child(child)
52+
child.adopt_child(grandchild)
53+
parent.mark_from_parent_destroy
54+
55+
parent.released?.should be_true
56+
child.released?.should be_true
57+
grandchild.released?.should be_true
58+
child.parent.should be_nil
59+
grandchild.parent.should be_nil
60+
end
61+
62+
it "rejects direct destroy while the control is owned by a parent" do
63+
parent = LifetimeContainer.new
64+
child = LifetimeControl.new
65+
parent.adopt_child(child)
66+
67+
expect_raises(Exception, /destroy a child control directly/) do
68+
child.destroy
69+
end
70+
71+
child.released?.should be_false
72+
child.parent.should eq(parent)
73+
end
74+
75+
it "rejects checked pointer access after parent destroy marking" do
76+
child = LifetimeControl.new
77+
child.mark_from_parent_destroy
78+
79+
expect_raises(Exception, /already been destroyed/) do
80+
child.to_unsafe
81+
end
82+
end
83+
84+
it "marks descendants released when native code destroys a root control" do
85+
root = LifetimeContainer.new
86+
child = LifetimeControl.new
87+
root.adopt_child(child)
88+
89+
root.mark_from_native_destroy
90+
91+
root.released?.should be_true
92+
child.released?.should be_true
93+
child.parent.should be_nil
94+
end
95+
end

src/uing.cr

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,11 @@ module UIng
2828
@@should_quit_callback_box : Pointer(Void)?
2929

3030
# Convert control to Pointer(LibUI::Control)
31+
def self.to_control(control : Control)
32+
control.check_available
33+
control.to_unsafe.as(Pointer(LibUI::Control))
34+
end
35+
3136
def self.to_control(control)
3237
if control.is_a?(Pointer)
3338
control.as(Pointer(LibUI::Control))

src/uing/area/area.cr

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -29,33 +29,32 @@ module UIng
2929
@ref_ptr = LibUI.new_scrolling_area(area_handler.to_unsafe, width, height)
3030
end
3131

32-
def destroy
32+
protected def before_destroy : Nil
3333
@area_handler = nil
34-
super
3534
end
3635

3736
def set_size(width : Int32, height : Int32) : Nil
38-
LibUI.area_set_size(@ref_ptr, width, height)
37+
LibUI.area_set_size(ref_ptr, width, height)
3938
end
4039

4140
def queue_redraw_all : Nil
42-
LibUI.area_queue_redraw_all(@ref_ptr)
41+
LibUI.area_queue_redraw_all(ref_ptr)
4342
end
4443

4544
def scroll_to(x : Float64, y : Float64, width : Float64, height : Float64) : Nil
46-
LibUI.area_scroll_to(@ref_ptr, x, y, width, height)
45+
LibUI.area_scroll_to(ref_ptr, x, y, width, height)
4746
end
4847

4948
def begin_user_window_move : Nil
50-
LibUI.area_begin_user_window_move(@ref_ptr)
49+
LibUI.area_begin_user_window_move(ref_ptr)
5150
end
5251

5352
def begin_user_window_resize(edge : WindowResizeEdge) : Nil
54-
LibUI.area_begin_user_window_resize(@ref_ptr, edge)
53+
LibUI.area_begin_user_window_resize(ref_ptr, edge)
5554
end
5655

5756
def to_unsafe
58-
@ref_ptr
57+
ref_ptr
5958
end
6059
end
6160
end

src/uing/box.cr

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -26,11 +26,11 @@ module UIng
2626
self.padded = true if padded
2727
end
2828

29-
def destroy
29+
protected def before_destroy : Nil
3030
@children_refs.each do |child|
31-
child.release_ownership
31+
child.mark_released_from_parent_destroy
3232
end
33-
super
33+
@children_refs.clear
3434
end
3535

3636
def delete(child : Control)
@@ -43,7 +43,7 @@ module UIng
4343

4444
def append(control, stretchy : Bool = false) : Nil
4545
control.check_can_move
46-
LibUI.box_append(@ref_ptr, UIng.to_control(control), stretchy)
46+
LibUI.box_append(ref_ptr, UIng.to_control(control), stretchy)
4747
@children_refs << control
4848
control.take_ownership(self)
4949
end
@@ -55,26 +55,26 @@ module UIng
5555
end
5656

5757
def num_children : Int32
58-
LibUI.box_num_children(@ref_ptr)
58+
LibUI.box_num_children(ref_ptr)
5959
end
6060

6161
def delete(index : Int32) : Nil
6262
child = @children_refs[index]
63-
LibUI.box_delete(@ref_ptr, index)
63+
LibUI.box_delete(ref_ptr, index)
6464
@children_refs.delete_at(index)
6565
child.release_ownership
6666
end
6767

6868
def padded? : Bool
69-
LibUI.box_padded(@ref_ptr)
69+
LibUI.box_padded(ref_ptr)
7070
end
7171

7272
def padded=(padded : Bool) : Nil
73-
LibUI.box_set_padded(@ref_ptr, padded)
73+
LibUI.box_set_padded(ref_ptr, padded)
7474
end
7575

7676
def to_unsafe
77-
@ref_ptr
77+
ref_ptr
7878
end
7979
end
8080
end

src/uing/button.cr

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,25 +11,24 @@ module UIng
1111
@ref_ptr = LibUI.new_button(text)
1212
end
1313

14-
def destroy
14+
protected def before_destroy : Nil
1515
@on_clicked_box = nil
16-
super
1716
end
1817

1918
def text : String?
20-
str_ptr = LibUI.button_text(@ref_ptr)
19+
str_ptr = LibUI.button_text(ref_ptr)
2120
UIng.string_from_pointer(str_ptr)
2221
end
2322

2423
def text=(text : String) : Nil
25-
LibUI.button_set_text(@ref_ptr, text)
24+
LibUI.button_set_text(ref_ptr, text)
2625
end
2726

2827
def on_clicked(&block : -> Nil) : Nil
2928
@on_clicked_box = ::Box.box(block)
3029
if boxed_data = @on_clicked_box
3130
LibUI.button_on_clicked(
32-
@ref_ptr,
31+
ref_ptr,
3332
->(_sender, data) : Nil {
3433
begin
3534
data_as_callback = ::Box(typeof(block)).unbox(data)
@@ -44,7 +43,7 @@ module UIng
4443
end
4544

4645
def to_unsafe
47-
@ref_ptr
46+
ref_ptr
4847
end
4948
end
5049
end

src/uing/checkbox.cr

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -11,26 +11,25 @@ module UIng
1111
@ref_ptr = LibUI.new_checkbox(text)
1212
end
1313

14-
def destroy
14+
protected def before_destroy : Nil
1515
@on_toggled_box = nil
16-
super
1716
end
1817

1918
def text : String?
20-
str_ptr = LibUI.checkbox_text(@ref_ptr)
19+
str_ptr = LibUI.checkbox_text(ref_ptr)
2120
UIng.string_from_pointer(str_ptr)
2221
end
2322

2423
def text=(text : String) : Nil
25-
LibUI.checkbox_set_text(@ref_ptr, text)
24+
LibUI.checkbox_set_text(ref_ptr, text)
2625
end
2726

2827
def checked? : Bool
29-
LibUI.checkbox_checked(@ref_ptr)
28+
LibUI.checkbox_checked(ref_ptr)
3029
end
3130

3231
def checked=(checked : Bool) : Nil
33-
LibUI.checkbox_set_checked(@ref_ptr, checked)
32+
LibUI.checkbox_set_checked(ref_ptr, checked)
3433
end
3534

3635
def on_toggled(&block : Bool -> Nil) : Nil
@@ -41,7 +40,7 @@ module UIng
4140
@on_toggled_box = ::Box.box(wrapper)
4241
if boxed_data = @on_toggled_box
4342
LibUI.checkbox_on_toggled(
44-
@ref_ptr,
43+
ref_ptr,
4544
->(_sender, data) : Nil {
4645
begin
4746
data_as_callback = ::Box(typeof(wrapper)).unbox(data)
@@ -56,7 +55,7 @@ module UIng
5655
end
5756

5857
def to_unsafe
59-
@ref_ptr
58+
ref_ptr
6059
end
6160
end
6261
end

src/uing/color_button.cr

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,18 +11,17 @@ module UIng
1111
@ref_ptr = LibUI.new_color_button
1212
end
1313

14-
def destroy
14+
protected def before_destroy : Nil
1515
@on_changed_box = nil
16-
super
1716
end
1817

1918
def color : {Float64, Float64, Float64, Float64}
20-
LibUI.color_button_color(@ref_ptr, out r, out g, out b, out a)
19+
LibUI.color_button_color(ref_ptr, out r, out g, out b, out a)
2120
{r, g, b, a}
2221
end
2322

2423
def set_color(r : Float64, g : Float64, b : Float64, a : Float64) : Nil
25-
LibUI.color_button_set_color(@ref_ptr, r, g, b, a)
24+
LibUI.color_button_set_color(ref_ptr, r, g, b, a)
2625
end
2726

2827
def on_changed(&block : Float64, Float64, Float64, Float64 -> Nil) : Nil
@@ -33,7 +32,7 @@ module UIng
3332
@on_changed_box = ::Box.box(wrapper)
3433
if boxed_data = @on_changed_box
3534
LibUI.color_button_on_changed(
36-
@ref_ptr,
35+
ref_ptr,
3736
->(_sender, data) : Nil {
3837
begin
3938
data_as_callback = ::Box(typeof(wrapper)).unbox(data)
@@ -48,7 +47,7 @@ module UIng
4847
end
4948

5049
def to_unsafe
51-
@ref_ptr
50+
ref_ptr
5251
end
5352
end
5453
end

0 commit comments

Comments
 (0)