warn for unclosed widgets

[iacore]

Let's say I have the following code.

            try dvui.label(@src(), "domain", .{}, .{});
            var txt_domain = try dvui.textEntry(@src(), .{ .text = &this.b_domain }, .{});
            txt_domain.deinit();

this is what it looks like

However, I forgot to add txt_domain.deinit(); at first, and it looked like this

the other widgets are nowhere to be found, and no error in terminal either

I think there should be a message (like the duplicate id one) to warn the user something is wrong.

[iacore]

About the detection logic

First, widgets that are opened and never closed (deinit) are all errors (I think). so (conservatively) we can detect "leak" at the end of each frame.

Second, about nested widgets. A more aggressive way to detect widget leak is to detect when the parent widget is closed. (I don't know if hierarchy is tracked in DVUI. If not, this is over-engineering)

here's the generalized case of "wrong usage".

var a = A.init(...);
var b = B.init(...);
a.deinit();
b.deinit();

Most widgets do nest properly instead of intermingle like this. However, a similar code pattern appears in https://github.com/david-vanderson/dvui/discussions/37).

the two valid cases

- window
+- text entry
+- button

- window
+- text entry
 +- button

[david-vanderson]

This is an excellent idea! I will try the second option where each widget that installs itself will check in deinit that it is still the parent before resetting the parent. I think if the parent id doesn't match, the id will (always?) be of the problem widget. Let me play around with that a bit and see if it works.

If it doesn't we'll do the first idea (leak detection at end of frame), but that requires more stored state.

Thanks!

[iacore]

Excellent find! The second, recursive option is indeed simpler.

[david-vanderson]

I've committed the second strategy and it seems to work well for me! It's especially nice since you added source locations to WidgetData because we can show the whole current parent chain.

Give it a test! How does it look?

[iacore]

Give it a test! How does it look?

You mean the debug window? It looks cool! Although I would have appreciated the red border on parents to be 1px wide, and 2px wide on current widget, so I can tell the exact location of the "current widget".

[david-vanderson]

Sorry I got ahead of myself. I mean, if you purposefully omit calling deinit() on a widget, does the resulting error message look okay?

(but good point about the red border, I've slimmed it down to 1px)

[iacore]

I updated the error message! take a look / i hope i did not leave any important info out of the message

while looking through the code I don't like this

    src: if (builtin.mode == .Debug) std.builtin.SourceLocation else void,

this does save some bytes, but makes programming harder. options: Options is waaaay bigger anyway

keeping it as ?std.builtin.SourceLocation is good, since we may decide to decouple "does it have source info" with "build mode" later, and then we only have to change in a few places, instead of if (builtin.mode == .Debug) being littered everywhere in the source code as it is now

[david-vanderson]

Looks pretty good but there are a few things (I've gone ahead and made these changes so you can see what I'm thinking, but can change again if needed)

• I don't think we need the source column - no way you can get multiple errors per line
• Why use stderr.print over log.err? Was it newlines, because I haven't figured out whether log lines should include them or not (it seems to be different on my linux machine vs. my mac)
• We need the whole parent chain. For example, if you comment out a TextEntryWidget deinit(), the message you get says to look at src/dvui.zig:8611 (our line number might not match up), but it's the line self.textLayout = TextLayoutWidget.init(... in TextEntryWidget.install(). By adding the parent chain you get stuff like this:

error(dvui): widget is not closed within its parent. did you forget to call `.deinit()`?
  TextLayout id 1b9e09ce was initialized at [src/dvui.zig:8604]
  ScrollContainer id d7be7cff was initialized at [src/dvui.zig:6021]
  ScrollAreaWidget vbox id d57146e2 was initialized at [src/dvui.zig:6009]
  ScrollAreaWidget id add5175 was initialized at [src/dvui.zig:8592]
  TextEntry id dfd0b9b0 was initialized at [src/Examples.zig:502]  *** Here's the actual problem
  Box id 7b1f0f49 was initialized at [src/Examples.zig:497]
  Box id 6f1fbb96 was initialized at [src/Examples.zig:255]
  Box id f803ecd7 was initialized at [src/Examples.zig:241]
  Box id 36f7bc53 was initialized at [src/dvui.zig:6807]
  Scale id f3031006 was initialized at [src/Examples.zig:238]
  ScrollContainer id eda46d91 was initialized at [src/dvui.zig:6021]
  ScrollAreaWidget vbox id 5657f38c was initialized at [src/dvui.zig:6009]
  ScrollAreaWidget id 9e02a683 was initialized at [src/Examples.zig:235]
  Box id fdc0ea4a was initialized at [src/dvui.zig:3603]
  FloatingWindow id 917bae4a was initialized at [src/Examples.zig:208]
  Window id 194e7b7b was initialized at [sdl-test.zig:18]

I agree the .Debug stuff is not great. Somehow I had gotten the idea that you wanted that! Oops. I've ripped it out. We can change to an Optional SourceLocation in the future if it becomes needed. I also realized that there was only one callsite for minSizeSet, so I inlined it into WidgetData.minSizeSetAndRefresh().

[iacore]

no way you can get multiple errors per line

some people may write code like that and never use zig fmt

Why use stderr.print over log.err?

the error(dvui): prefix is annoying if its on every line

We need the whole parent chain.

Then I don't think the algorithm is simple or correct.

Your example was like this

a = A.init()
b = B.init()
c = C.init()
d = D.init()
...
a.deinit()

IMO, the algorithm should only warn that b is not closed. When b is closed, it should warn that c is not closed.

This gives the user much less information to work with.

[david-vanderson]

For the column info, I'm fine either way - add it back!

For the log prefix, yes it's annoying, but if we are living inside the logging framework I think we should stay inside it. If someone is using the logging to send all dvui logs somewhere, we need to keep them together.

The algorithm might not be optimal, but I'm not sure how to make it better. In many cases there will be multiple unclosed widgets, and I don't know how we could tell which is the right one to report.

So I see it more like a zig error trace. When you get a crash, usually the first couple of functions in the stack are inside the zig standard library, so you scan down until you see your code and go from there.

If you have an idea for how to make it work better, that would be wonderful! My test case is commenting out te.deinit() in src/Examples.zig around line 494.

[iacore]

The algorithm might not be optimal, but I'm not sure how to make it better.

I'll take a look.

In many cases there will be multiple unclosed widgets, and I don't know how we could tell which is the right one to report.

Report the one opened earliest

[iacore]

I think this is done!

[david-vanderson]

Thank you!