Permissive dropping

[sanbox-irl]

Hello!

Right now, we have punitive dropping for various token types. This ends up making the API a bit rough to use sometimes, and forces us to be very cognizant of these tokens. In my opinion, this can hurt readability and add to mental effort.

A typical token looks like this:

pub struct TooltipToken {
    ctx: *const Context,
}

impl TooltipToken {
    /// Ends a layout tooltip
    pub fn end(mut self, _: &Ui) {
        self.ctx = ptr::null();
        unsafe { sys::igEndTooltip() };
    }
}

impl Drop for TooltipToken {
    fn drop(&mut self) {
        if !self.ctx.is_null() && !thread::panicking() {
            panic!("A TooltipToken was leaked. Did you call .end()?");
        }
    }
}

what if instead, it looked like this?

pub struct TooltipToken(bool);

impl TooltipToken {
    /// Ends a layout tooltip
    pub fn end(mut self) {
        self.0 = true;
        unsafe { sys::igEndTooltip() };
    }
}

impl Drop for TooltipToken {
    fn drop(&mut self) {
        if !self.0 {
             unsafe { sys::igEndTooltip() };
        }
    }
}

Notice a few things going on with the change -- first, we're just using a bool instead of a raw pointer, since we don't really use the raw pointer for anything anyway, so we might as well make it into a bool.

Secondly, we never actually need the Ui to drop, and that makes our users do more typing for....minimal purposes? If we made this change, however, it would be a serious breaking change to remove those Ui calls, so we can leave them in instead.

Finally, notice how Drop conditionally executes the end logic. If we choose to drop early, we can without issue, or we can drop in the end result.

The only downside to this approach is that if you ignore the must_use warnings when you create one of these tokens, like this:

ui.begin_tooltip();

We will create a tooltip and then immediately end it by calling drop immediately. This seems...not such a bad thing to me? The user has done no work, sure, but they'll have a fat warning on that line of code letting them know they're doing something odd.

What do you think?

Just to be clear, I'd be proposing this kind of change for all tokens in the library like this.

[sanbox-irl]

Extremely minor point here, but converting the tokens to use bool instead of a raw pointer technically will save users some space on the cache. It doesn't matter that much, but could have a negligible to undetectable performance improvement. I'm proposing this for ergonomics instead

[thomcc]

I'll have to play around with it a bit, and think on it more after the holidays, but my initial feeling is positive.

I think if we kept it at all, perhaps the panicing should be replaced with either a warning, or only be present if a feature is enabled or something. I'll have to think. I'm not sure full RAII is the way to go.

[thomcc]

I also think with some use of mem::forget we probably could avoid the bool, and just have the structs be wrappers around PhantomData.

Actually, as listed, even that's not needed.

[sanbox-irl]

I actually completely agree about the notion of non RAII. Here's the imgui-show-demo help text in C++ and in (current) imgui-rs:

    ImGui::TextDisabled("(?)");
    if (ImGui::IsItemHovered())
    {
        ImGui::BeginTooltip();
        ImGui::PushTextWrapPos(ImGui::GetFontSize() * 35.0f);
        ImGui::TextUnformatted(desc);
        ImGui::PopTextWrapPos();
        ImGui::EndTooltip();
    }

    ui.text_disabled("(?)");

    if ui.is_item_hovered() {
        let tooltip = ui.begin_tooltip();
        let output = ui.push_text_wrap_pos(ui.current_font_size() * 35.0);
        ui.text(description);
        output.pop(ui);
        tooltip.end(ui);
    }

I think, personally, the C++ is remarkably simpler and easier to scan over, though absolutely allows for logical errors. I think the better option would be exposing a non-RAII version entirely in the Rust side. This can also allow for an easier time referencing existing C++ imgui code

[sanbox-irl]

a tab bar impl with PhantomData and mem::forget --

/// Tracks a window that must be ended by calling `.end()`
pub struct TabBarToken<'a>(std::marker::PhantomData<Ui<'a>>);

impl<'a> TabBarToken<'a> {
    fn new(_: &Ui<'a>) -> Self {
        Self(std::marker::PhantomData)
    }

    /// Ends a tab bar.
    pub fn end(self) {
        unsafe { sys::igEndTabBar() };
        std::mem::forget(self);
    }
}

impl Drop for TabBarToken<'_> {
    fn drop(&mut self) {
        // maybe we warn to not drop values?
        // how do we warn? a println? pull in log?
        unsafe { sys::igEndTabBar() };
    }
}

I believe the idea of dropping and panicking for these tokens initially was because some token stacks must be balanced, and ImGui will panic if they aren't. ImGui panics don't get a stacktrace since they're over FFI, and the idea was that by panicking ourselves first, we'll get a better stacktrace.

Fundamentally, the support, and ergonomics, is the idea behind these style tokens. I'm not sure either is a great argument though. But getting rid of all tokens is probably a bigger conversation.

For now, having the tokens be nicer and cover our user's mistakes seems nice, since they're only really for making developer's lives easier, rather than being punitive

[sanbox-irl]

After messing with the above and a few other methods, I think this is the best option for an RAII approach:

/// Tracks a window that must be ended by calling `.end()`
pub struct TabBarToken;

impl TabBarToken {
    fn new(_: &Ui) -> Self {
        Self
    }

    /// Ends a tab bar.
    pub fn end(self) {
      // empty -- just drop
    }
}

impl Drop for TabBarToken {
    fn drop(&mut self) {
        unsafe { sys::igEndTabBar() };
    }
}

The major difference with the current type is that it's ZST and it's permissive, relying on the drop to do everything. Its difference from the above is that it has no lifetime.

This works well for a lot of things that are stack based.

I've also been experimenting with adding methods which are actual Global methods, which don't return tokens and which rely on the programmer to maintain the program's state entirely, like the C++. For some things, this works great. I think that's a separate issue, but I will add another issue with a check in as I make more

[sanbox-irl]

Want to ping this again -- I have a decent number of these written out on my own branch and would like to PR these in, but to do so, I'd want to adjust every RAII token which already exists (it would be terrible if some panic on drop, and others don't). That'll be a bit of regexing and scratching my head, so want to check in.

After going through the above, I've ended up mixing the two -- adding back in the lifetime for the extremely weird case where someone kept one of these around after droping Ui (which is kind of annoying, but works just fine), but relying on just running drop.

/// Tracks a window that can either be ended with `end` or by dropping. Forgetting
/// this struct with std::mem::forget will create panics (so don't do that please).
pub struct TabBarToken<'a>(std::marker::PhantomData<Ui<'a>>);

impl<'a> TabBarToken<'a> {
    fn new(_: &'a Ui) -> Self {
        Self(std::marker::PhantomData)
    }

    /// Ends a tab bar.
    pub fn end(self) {
      // empty -- just drop
    }
}

impl Drop for TabBarToken<'_> {
    fn drop(&mut self) {
        unsafe { sys::igEndTabBar() };
    }
}

Additionally, having looked through all of these, different words are used for the force drop method, such as end and pop. Although there are times when each word is more appropriate, I think the entire API should just use one single word for all them. I suggest pop personally, since that evokes the stack we're handling, though I would also like us to add push instead of new as well if we went that route. That being said, we can use deprecate and what not to soft change those functions over time. I am also totally willing to do all of that work.

Additionally, I've made another issue #426 on a related subject, but have separated them out so we don't overwhelm

[sanbox-irl]

for anyone curious, a first draft of this PR is here: https://github.com/imgui-rs/imgui-rs/pull/440

[dbr]

..and now merged :partying_face: I think this issue can be closed?

[sanbox-irl]

noice noice