Skip to content

Implement push, remove and insert methods on arrays in slint#11614

Open
uAtomicBoolean wants to merge 10 commits into
slint-ui:masterfrom
uAtomicBoolean:model-modify-operations
Open

Implement push, remove and insert methods on arrays in slint#11614
uAtomicBoolean wants to merge 10 commits into
slint-ui:masterfrom
uAtomicBoolean:model-modify-operations

Conversation

@uAtomicBoolean
Copy link
Copy Markdown
Contributor

@uAtomicBoolean uAtomicBoolean commented May 4, 2026

I took a shot at implementing the push, remove and insert methods on the arrays in Slint but it is not complete as it is missing the C++ implementation. However, I'd like to have a first review if possible.

In order to do that, I added three new methods in the Model trait: push_row, insert_row and remove_row.
However, while implementing these methods in the different models, I came across code duplications with:

  • the Javascript ArrayModel which already implements a push methods that takes a list of values and a remove method.
  • the Python ListModel which implements an append method.

So I have the following questions:

  • What should I do regarding the code duplication in the JS ArrayModel and Python ListModel ?
  • What should be the values in inline_expressions.rs ? (I temporarily set all three methods to 50)

Fixes #9412

@uAtomicBoolean uAtomicBoolean force-pushed the model-modify-operations branch from 62f1cbe to ec5fd74 Compare May 4, 2026 15:13
Fully works on Rust.
Add basic implementation in interpreter (not final).
All three methods are not implemented yet in C++.
Add tests for the rust, c++ and js interpreter drivers.
Add new methods in the Model trait and implements where necessary.
This introduce duplicated methods in the Javascript API (pushRow -> push, etc), this should be fixed.
Improves Rust code generation to use directly the new Model methods.
Implements the methods in VecModel.
@uAtomicBoolean uAtomicBoolean force-pushed the model-modify-operations branch from a30465a to 3c93859 Compare May 4, 2026 15:28
@uAtomicBoolean uAtomicBoolean marked this pull request as ready for review May 4, 2026 15:29
@uAtomicBoolean uAtomicBoolean force-pushed the model-modify-operations branch from f97810f to 7216737 Compare May 4, 2026 18:02
Copy link
Copy Markdown
Member

@ogoffart ogoffart left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR.

(Coincidentally, someone else is working on something similar: #11564)

This is definitively the right direction IMHO.

I'd like to see more tests:
Doing these operations on read only model.
Doing these operations with invalid indexes. (negative or too high)
Doing these operations on empty model.
Or anything creative.

Also checking that repeater gets instantiated
(extend tests/cases/model.slint to do the same as what was done in rust, but through functions in slint)

Also syntax test that checks that pushing data of different type correctly errors out.

API wise, we have the choice between push and push_row for the Slint and Rust function. Should they be the same in Rust and Slint? Is the _row suffix useful? (it is consistant with the row_data/set_row_data functions)

the Javascript ArrayModel which already implements a push methods that takes a list of values and a remove method.

This is maybe an argument to just name it push
now the question is whether that would work to have thatfunction only take one argument in the interface, and more argument on the instance?

the Python ListModel which implements an append method.

I don't know the python convention much. We could have both doing the same thing, or keep only append.

let model = &#model;
let index = #index as usize;

if index < model.row_count() {
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.

I'd say we could leave remove_row to take care of this condition in order to simplify the generated code (same in ArrayInsert)

Comment thread internal/compiler/llr/optim_passes/inline_expressions.rs Outdated
Comment thread internal/core/model.rs Outdated
Comment thread internal/core/model.rs
///
/// If the model can update the data, it should also call [`ModelNotify::row_changed`] on its
/// internal [`ModelNotify`].
fn push_row(&self, _data: Self::Data) {
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.

API-wise, i wonder if this should be called push or push_row
I can see argument for both

And should we perhaps return bool for faillure?

(And same question for other methods)

@tronical : opinions?

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.

I think the _row suffix is a great idea. 👍

For the prefix, my preference is append, which is closer to being the opposite of
removal (the opposite of push is pull or perhaps pop) - but call it a preference :)

To summarize:

My preference is append_row, remove_row, and insert_row - those are in perfect symmetry to me.
But I'd also be okay with push_row, remove_row, and insert_row.

format!("slint::private_api::model_length({})", a.next().unwrap())
}
BuiltinFunction::ArrayPush => {
todo!()
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.

I guess the way to implement that is to add private function, similar to model_length before that.
One tricky part is that the default implementation of model generated by slint cannot be re-allocated. So we will have to get rid of this optimization. (use VectorModel instead of ArrayModel)

Comment on lines +1134 to +1136
.or_else(|| f("push", member_macro(BuiltinMacroFunction::ArrayPush)))
.or_else(|| f("remove", member_macro(BuiltinMacroFunction::ArrayRemove)))
.or_else(|| f("insert", member_macro(BuiltinMacroFunction::ArrayInsert)))
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.

This new API make sense.

But we could also argue for push_row/remove_row/insert_row to be consistent with the Rust API.

Comment on lines +270 to +272
ArrayPush: (Type::Model) -> Type::Void,
ArrayRemove: (Type::Model, Type::Int32) -> Type::Void,
ArrayInsert: (Type::Model, Type::Int32) -> Type::Void,
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.

Well, this is missing the data argument.
I am unsure if this type is used anywhere. (Maybe for function signature in the LSP I think.)
Maybe you can just use Type::Void or Type::InferredProperty or just some placeholder with a comment.

Worth making a completion or signature test in slint-lsp.

let element_type = match args[0].0.ty() {
Type::Array(t) => (*t).clone(),
_ => {
diag.push_error(format!("push() was called on a non-array: {:?}", args[0].0), node);
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.

I think we can never get this error, otherwise the lookup would fail, right?

diag: &mut BuildDiagnostics,
) -> Expression {
if args.len() != 2 {
diag.push_error(
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.

Could you add syntax tests for this errors? (no arguments, too many arguments, also arguments of wrong type)

(also for other functions)

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.

Allow modifying operation on Model (e.g. push, insert, remove)

3 participants