Skip to content

Use new xproperty#279

Merged
JohanMabille merged 22 commits into
jupyter-xeus:mainfrom
Hind-M:use_new_xproperty
Apr 21, 2026
Merged

Use new xproperty#279
JohanMabille merged 22 commits into
jupyter-xeus:mainfrom
Hind-M:use_new_xproperty

Conversation

@Hind-M

@Hind-M Hind-M commented Feb 2, 2026

Copy link
Copy Markdown
Member

@Hind-M
Hind-M force-pushed the use_new_xproperty branch from 8e80491 to 765b593 Compare April 8, 2026 09:00
@Hind-M
Hind-M force-pushed the use_new_xproperty branch from 765b593 to 7f94135 Compare April 14, 2026 13:12
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
1 pipeline(s) were filtered out due to trigger conditions.

@Hind-M

Hind-M commented Apr 16, 2026

Copy link
Copy Markdown
Member Author

NOTE:

In the xwidgets.ipynb notebook example:

#include <xwidgets/xbutton.hpp>

auto bt = xw::button::initialize()
    .description("button")
    .finalize();

bt.on_click([](){
    bt.description = "Clicked!";
    bt.button_style = "success";
});
    
xcpp::display(bt);

There was a bug where finalize() with a rvalue forced a copy of the widget. This resulted in the loss of registered validators for the new object.

In the case of xbutton, the following validator:

template <typename... T>
auto XEITHER(T... vals)
{
    const std::unordered_set<std::string> options({vals...});
    return [options](const auto& proposal)
    {
        auto position = options.find(proposal);
        if (position == options.end())
        {
            throw std::runtime_error("Invalid proposal for string enum");
        }
    };
}

used here:

XPROPERTY(
    std::string,
    xcommon,
    button_style,
    "",
    XEITHER("primary", "success", "info", "warning", "danger", "")
);

was not being triggered in the subsequent cell:

try {
    bt.button_style = "some invalid value";  // values are validated upon assignment
}
catch(std::exception const& btn_exception)
{
    std::cerr << btn_exception.what();
}

This was supposed to catch the thrown exception and print:
Invalid proposal for string enum, but that wasn't the case as nothing was printed and the following code cell:

std::cout << bt.button_style();

printed the invalid value which shouldn't be set in the first place: some invalid value.

This is fixed in this PR as the exception message is printed now and the value of bt.button_style() remains unchanged (the default empty string), meaning that the invalid assignment was correctly blocked.

@Hind-M
Hind-M marked this pull request as ready for review April 20, 2026 12:17
Comment thread include/xwidgets/xcommon.hpp Outdated
{
this->finalize();
return std::move(*this);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Now that the chaining syntax is not available anymore, I think we shoujld keep overloads for both lvalue and rvalue references. Maybe we can simply have a singel finalize method. Even if we find a nicer initialization syntax, it won't be based on chaining move constructor since the xproperty does not know its owner type anymore.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Actually having a single finalize method working for all kinds of reference would break the following:

auto w = std::move(my_widget).finalize();

Although this code is weird and unlikely to happen, we should opt for the less surprise principle. So let's keep bothe overloads for now, ignore my previous comment.

@JohanMabille
JohanMabille merged commit 8d50940 into jupyter-xeus:main Apr 21, 2026
9 checks passed
@Hind-M
Hind-M deleted the use_new_xproperty branch April 21, 2026 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants