Skip to content

Use callbacks on heap and introduce reference counting #20

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.

Sign up for GitHub

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

Merged
kimkulling merged 2 commits into from
Mar 11, 2026
Merged

Use callbacks on heap and introduce reference counting #20

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
55daac0
Use callbacks on heap and introduce reference counting
kimkulling Mar 11, 2026
861ca9c
Use refcount for callbacks
kimkulling Mar 11, 2026
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
13 changes: 7 additions & 6 deletions samples/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,13 +95,14 @@
Widgets::button(5, RootPanelId, "Test 3", Rect(100, 150, 100, 40), nullptr);
Widgets::imageButton(6, RootPanelId, "button_test.png", Rect(100, 200, 100, 40), nullptr);


auto &ctx = TinyUi::getContext();
CallbackI quitCallback(quit, (void*) &ctx);
Widgets::button(7, RootPanelId, "Quit", Rect(100, 250, 100, 40), &quitCallback);

CallbackI updateProgressBarCallback(updateProgressbar, nullptr, Events::UpdateEvent);
Widgets::progressBar(8, RootPanelId, Rect(100, 300, 100, 40), 50, &updateProgressBarCallback);
// Allocate callbacks dynamically to ensure they persist during event handling
CallbackI *dynamicQuitCallback = new CallbackI(quit, (void*) &ctx);

Check failure on line 101 in samples/main.cpp

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Replace the use of "new" with an operation that automatically manages the memory. 


See more on https://sonarcloud.io/project/issues?id=kimkulling_tiny_ui&issues=AZzem9Sw5gyFWmdews0o&open=AZzem9Sw5gyFWmdews0o&pullRequest=20

Check warning on line 101 in samples/main.cpp

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Replace the redundant type with "auto". 


See more on https://sonarcloud.io/project/issues?id=kimkulling_tiny_ui&issues=AZzem9Sv5gyFWmdews0m&open=AZzem9Sv5gyFWmdews0m&pullRequest=20
CallbackI *dynamicUpdateProgressBarCallback = new CallbackI(updateProgressbar, nullptr, Events::UpdateEvent);

Check warning on line 102 in samples/main.cpp

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Replace the redundant type with "auto". 


See more on https://sonarcloud.io/project/issues?id=kimkulling_tiny_ui&issues=AZzem9Sw5gyFWmdews0n&open=AZzem9Sw5gyFWmdews0n&pullRequest=20

Check failure on line 102 in samples/main.cpp

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Replace the use of "new" with an operation that automatically manages the memory. 


See more on https://sonarcloud.io/project/issues?id=kimkulling_tiny_ui&issues=AZzem9Sw5gyFWmdews0p&open=AZzem9Sw5gyFWmdews0p&pullRequest=20

Widgets::button(7, RootPanelId, "Quit", Rect(100, 250, 100, 40), dynamicQuitCallback);
Widgets::progressBar(8, RootPanelId, Rect(100, 300, 100, 40), 50, dynamicUpdateProgressBarCallback);
Comment on lines +100 to +105
Copy link

@coderabbitai coderabbitai bot Mar 11, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Check how callbacks are assigned in widget creation functions

# Search for button implementation
echo "=== Searching for Widgets::button implementation ==="
ast-grep --pattern $'ret_code Widgets::button($$$) {
  $$$
}'

# Search for progressBar implementation  
echo "=== Searching for Widgets::progressBar implementation ==="
ast-grep --pattern $'ret_code Widgets::progressBar($$$) {
  $$$
}'

Repository: kimkulling/tiny_ui

Length of output: 172

🏁 Script executed:

#!/bin/bash
# Find the relevant files
echo "=== Finding main.cpp ==="
fd -t f "main.cpp"

echo "=== Finding tinyui.h ==="
fd -t f "tinyui.h"

echo "=== Finding CallbackI class definition ==="
rg "class CallbackI|struct CallbackI" -A 20

Repository: kimkulling/tiny_ui

Length of output: 1971

🏁 Script executed:

#!/bin/bash
# Get the full CallbackI struct definition
echo "=== Full CallbackI definition from tinyui.h ==="
rg "struct CallbackI" -A 80 src/tinyui.h | head -120

echo ""
echo "=== Search for incRef and decRef methods ==="
rg "incRef|decRef" src/tinyui.h -A 3

echo ""
echo "=== Search for Widgets namespace and button/progressBar functions ==="
fd -t f "*.cpp" "*.h" -x rg "namespace Widgets|ret_code.*button|ret_code.*progressBar" {} + | head -30

Repository: kimkulling/tiny_ui

Length of output: 3404

🏁 Script executed:

#!/bin/bash
# Search for Widgets functions
echo "=== Search for button function ==="
rg "button\(" src/ -B 2 -A 15 | head -50

echo ""
echo "=== Search for progressBar function ==="
rg "progressBar\(" src/ -B 2 -A 15 | head -50

echo ""
echo "=== Search for any calls to incRef ==="
rg "incRef\(\)" src/ samples/ -B 2 -A 2

Repository: kimkulling/tiny_ui

Length of output: 4818

🏁 Script executed:

#!/bin/bash
# Get the full progressBar implementation
echo "=== Full progressBar implementation ==="
rg "ret_code Widgets::progressBar" -A 30 src/widgets.cpp

echo ""
echo "=== Check samples/main.cpp lines 95-110 ==="
sed -n '95,110p' samples/main.cpp

Repository: kimkulling/tiny_ui

Length of output: 1941

Only the progressBar callback will leak; the button callback is properly managed.

The review comment incorrectly states that incRef() is never called in Widgets::button(). The button() function does call callback->incRef() after assignment, so dynamicQuitCallback will be properly reference-counted and freed when the widget is destroyed.

However, Widgets::progressBar() does not call incRef(). It only assigns the callback and adds it to ctx.mUpdateCallbackList. This means dynamicUpdateProgressBarCallback starts with mNumRefs = 0, and when the widget is destroyed, decRef() will not delete it, causing a memory leak.

🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis

[failure] 101-101: Replace the use of "new" with an operation that automatically manages the memory.

See more on https://sonarcloud.io/project/issues?id=kimkulling_tiny_ui&issues=AZzem9Sw5gyFWmdews0o&open=AZzem9Sw5gyFWmdews0o&pullRequest=20

[warning] 102-102: Replace the redundant type with "auto".

See more on https://sonarcloud.io/project/issues?id=kimkulling_tiny_ui&issues=AZzem9Sw5gyFWmdews0n&open=AZzem9Sw5gyFWmdews0n&pullRequest=20

[failure] 102-102: Replace the use of "new" with an operation that automatically manages the memory.

See more on https://sonarcloud.io/project/issues?id=kimkulling_tiny_ui&issues=AZzem9Sw5gyFWmdews0p&open=AZzem9Sw5gyFWmdews0p&pullRequest=20

[warning] 101-101: Replace the redundant type with "auto".

See more on https://sonarcloud.io/project/issues?id=kimkulling_tiny_ui&issues=AZzem9Sv5gyFWmdews0m&open=AZzem9Sv5gyFWmdews0m&pullRequest=20

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@samples/main.cpp` around lines 100 - 105, The progressBar path leaks because
Widgets::progressBar assigns dynamicUpdateProgressBarCallback and pushes it into
ctx.mUpdateCallbackList without calling CallbackI::incRef(), leaving mNumRefs at
0 so decRef() won't delete it; update Widgets::progressBar (the assignment and
list-push logic that handles the callback) to call callback->incRef() when the
widget takes ownership (same place Widgets::button calls incRef()), or ensure
the code that adds to ctx.mUpdateCallbackList increments the refcount so
decRef() can later free the CallbackI instance.


Widgets::inputText(9, RootPanelId, Rect(100, 350, 100, 40), Alignment::Left);

Expand All @@ -110,7 +111,7 @@
//Widgets::treeItem(12, 11, "Item 1.1");
Widgets::treeItem(13, 10, "Item 2");
Widgets::treeItem(14, 13, "Item 2.1");

while (TinyUi::run()) {
TinyUi::render();
}
Expand Down
17 changes: 17 additions & 0 deletions src/tinyui.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,8 @@
funcCallback mfuncCallback[Events::NumEvents];
/// The data instance.
void *mInstance{nullptr};
/// The reference count for the callback interface, used for memory management.
uint32_t mNumRefs{ 0 };
Comment on lines +317 to +318
Copy link

@coderabbitai coderabbitai bot Mar 11, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Initialize mNumRefs to 1 for correct reference counting semantics.

With mNumRefs initialized to 0, a newly created callback requires an explicit incRef() before the first decRef() will function correctly. The standard intrusive reference counting pattern initializes the count to 1 at construction time, representing the creator's ownership.

🐛 Proposed fix 
     /// The reference count for the callback interface, used for memory management.
-    uint32_t mNumRefs{ 0 };
+    uint32_t mNumRefs{ 1 };
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/tinyui.h` around lines 317 - 318, The member mNumRefs is initialized to 0
which breaks intrusive reference-count semantics; change its initialization to 1
so that a newly constructed callback object owns one reference by default
(update the declaration of uint32_t mNumRefs to initialize to 1); locate the
member in the callback class in src/tinyui.h and adjust the initializer for
mNumRefs accordingly so decRef() can be called without a prior explicit
incRef().


/// @brief The default class constructor.
CallbackI() : mfuncCallback{ nullptr } {
Expand All @@ -339,6 +341,21 @@
mfuncCallback[i] = nullptr;
}
}

/// @brief Increment the reference count.
void incRef() {
++mNumRefs;
}

/// @brief Decrement the reference count.
void decRef() {
if (mNumRefs > 0) {
--mNumRefs;
if (mNumRefs == 0) {
delete this;

Check failure on line 355 in src/tinyui.h

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Rewrite the code so that you no longer need this "delete". 


See more on https://sonarcloud.io/project/issues?id=kimkulling_tiny_ui&issues=AZzem9QZ5gyFWmdews0l&open=AZzem9QZ5gyFWmdews0l&pullRequest=20
}
}
}
Comment on lines +351 to +358
Copy link

@coderabbitai coderabbitai bot Mar 11, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

decRef() logic flaw and fragile delete this pattern.

Two concerns:

  1. Logic issue: If mNumRefs is 0 (initial value), decRef() silently does nothing. This means callbacks that were never incRef()'d will leak. Either fix the initial value (see above) or adjust the guard.

  2. delete this is error-prone: The caller has no indication that the object was destroyed, which can lead to use-after-free. Consider returning a boolean indicating whether the object was deleted, or using a weak reference pattern.

🛡️ Proposed safer implementation 
     /// `@brief` Decrement the reference count.
-    void decRef() {
-        if (mNumRefs > 0) {
-            --mNumRefs;
-            if (mNumRefs == 0) {
-                delete this;
-            }
-        }
+    /// `@return` true if the object was deleted, false otherwise.
+    bool decRef() {
+        assert(mNumRefs > 0 && "decRef called on object with zero refcount");
+        --mNumRefs;
+        if (mNumRefs == 0) {
+            delete this;
+            return true;
+        }
+        return false;
     }

Alternatively, for modern C++17, consider using std::shared_ptr with a custom destructor or std::enable_shared_from_this to avoid manual reference counting entirely.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
void decRef() {
if (mNumRefs > 0) {
--mNumRefs;
if (mNumRefs == 0) {
delete this;
}
}
}
/// `@brief` Decrement the reference count.
/// `@return` true if the object was deleted, false otherwise.
bool decRef() {
assert(mNumRefs > 0 && "decRef called on object with zero refcount");
--mNumRefs;
if (mNumRefs == 0) {
delete this;
return true;
}
return false;
}
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis

[failure] 355-355: Rewrite the code so that you no longer need this "delete".

See more on https://sonarcloud.io/project/issues?id=kimkulling_tiny_ui&issues=AZzem9QZ5gyFWmdews0l&open=AZzem9QZ5gyFWmdews0l&pullRequest=20

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/tinyui.h` around lines 351 - 358, decRef() currently silently ignores
calls when mNumRefs is 0 and uses delete this, which is unsafe; change decRef to
return a bool and enforce/validate the counter: ensure mNumRefs is initialized
correctly (or assert mNumRefs>0 on entry), decrement mNumRefs and if it reaches
0 perform deletion and return true, otherwise return false; update all callers
of decRef to handle the boolean result so they know the object may have been
destroyed (or, as an alternative, replace manual ref counting with
std::shared_ptr/std::enable_shared_from_this to eliminate delete this and manual
mNumRefs management).

};

/// @brief The event callback array.
Expand Down
10 changes: 10 additions & 0 deletions src/widgets.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,9 @@ ret_code Widgets::button(Id id, Id parentId, const char *text, const Rect &rect,
}

child->mCallback = callback;
if (callback != nullptr) {
callback->incRef();
}
Comment on lines 273 to +276
Copy link

@coderabbitai coderabbitai bot Mar 11, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Reference count leak when reassigning callback on existing widget.

createWidget can return an existing widget (lines 119-121). If that widget already has a callback, assigning the new callback overwrites mCallback without calling decRef() on the old one—leaking its reference count.

🔧 Proposed fix: decrement old callback before assignment 
+    if (child->mCallback != nullptr) {
+        child->mCallback->decRef();
+    }
     child->mCallback = callback;
     if (callback != nullptr) {
         callback->incRef();
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
child->mCallback = callback;
if (callback != nullptr) {
callback->incRef();
}
if (child->mCallback != nullptr) {
child->mCallback->decRef();
}
child->mCallback = callback;
if (callback != nullptr) {
callback->incRef();
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/widgets.cpp` around lines 273 - 276, When assigning a new callback to an
existing widget you must release the old reference first to avoid leaking refs:
in the code path where you set child->mCallback (e.g., in createWidget/where
mCallback is reassigned), if child->mCallback is non-null call
child->mCallback->decRef() before overwriting it, then assign child->mCallback =
callback and if (callback != nullptr) call callback->incRef(); ensure you update
every place that reassigns mCallback so incRef/decRef are balanced.

if (text != nullptr) {
child->mText.assign(text);
}
Expand All @@ -294,6 +297,10 @@ ret_code Widgets::imageButton(Id id, Id parentId, const char *image, const Rect
}

child->mCallback = callback;
if (callback != nullptr) {
callback->incRef();
}

Comment on lines 299 to +303
Copy link

@coderabbitai coderabbitai bot Mar 11, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Same reference count leak issue as button().

Apply the same fix here—call decRef() on the existing callback before reassignment.

🔧 Proposed fix 
+    if (child->mCallback != nullptr) {
+        child->mCallback->decRef();
+    }
     child->mCallback = callback;
     if (callback != nullptr) {
         callback->incRef();
     }
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/widgets.cpp` around lines 299 - 303, Before assigning child->mCallback =
callback, if child->mCallback is non-null call child->mCallback->decRef() to
release the old reference, then assign the new callback and call
callback->incRef() only if callback is non-null; i.e., ensure the existing
callback is decRef'ed prior to reassignment (referencing child->mCallback,
callback, decRef(), and incRef()) to avoid the same reference-count leak as in
button().

if (image != nullptr) {
child->mImage = loadIntoImageCache(ctx, image);
}
Expand Down Expand Up @@ -395,6 +402,9 @@ ret_code Widgets::treeView(Id id, Id parentId, const char *title, const Rect &re

CallbackI *callback = new CallbackI(onTreeViewItemClicked, nullptr, Events::MouseButtonDownEvent);
widget->mCallback = callback;
if (callback != nullptr) {
callback->incRef();
}

return ResultOk;
}
Expand Down
4 changes: 4 additions & 0 deletions src/widgets.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,10 @@ struct Widget {
if (mContent != nullptr) {
delete [] mContent;
}

if (mCallback != nullptr) {
mCallback->decRef();
}
}

/// @brief Check if the widget has a specific style.
Expand Down
Loading