gtk-rs / gtk4-rs

Rust bindings of GTK 4
https://gtk-rs.org/gtk4-rs/
MIT License
1.89k stars 175 forks source link

Don't implement `Copy` on certain BoxedInline structs #1532

Open SeaDve opened 1 year ago

SeaDve commented 1 year ago

Currently, it implements Copy, which could cause unexpected behaviors. I think it is better to just implement Clone, so the user can explicitly create a copy of the iterator.

This is also the reason why Range from std lib doesn't implement Copy, even though the underlying type implements copy.

Example:

let mut text_start = text_end;
text_start.backward_line();

Does text_end also move backward a line? (Spoiler: it does not, since it is creating a copy behind the scenes)

let mut text_start = text_end.clone();
text_start.backward_line();

Here, it is explicit that we are making a deep copy of the text_end.

sdroege commented 1 year ago

That makes sense, yes. We should check other types that currently implement Copy too, here and in gtk-rs-core.

Can you create an MR and go over these?

SeaDve commented 1 year ago

A quick look on gtk4-rs which has possibly problematic Copy are the following:

In gtk-rs-core, there are no iterators, but there are these:

sdroege commented 1 year ago

GStringBuilder (BoxedInline, but somehow no Copy impl?)

Only ones that have no copy/clear function are Copy IIRC. If you need to free memory or do more things than a memcpy then it's not Copy and can't be.

The ones you listed seem all OK (gtk-rs-core). For gtk4-rs I'll let @bilelmoussaoui comment.

bilelmoussaoui commented 10 months ago

GskRoundedRect (has &mut methods)

Not sure how to handle this one

GtkBorder (has &mut methods)

But the type also provides a copy function? gtk_border_copy, where that is supposed to be used in this case?

GtkTopLevelSize (has &mut methods)

This was fixed already

bilelmoussaoui commented 10 months ago

For TextIter, see also #1280