Drop general extensions in favor of widget-specific extensions #127
Conversation
Return `RelmRemovableExt` to its previous state, remove `RelmWidgetExt::iter_children` and add widget-specific extensions instead. Add tests for the widget-specific extensions.
There was a problem hiding this comment.
Excellent work! This is shaping up nicely and I really like the clean implementations for RelmIterChildrenExt and the DoubleEndedIterator implementation. I couldn't have done this better :)
There are just a few small things, I'd like to sort out before merging it.
…f `ChildrenIterator`
|
Done. Also I've noticed incorrect behavior of |
src/extensions/mod.rs
Outdated
| if !self.init_start { | ||
| self.init_start = true; | ||
| return self.start.clone(); | ||
| } | ||
|
|
||
| std::iter::from_fn(move || { | ||
| if let Some(child) = widget.take() { | ||
| widget = child.next_sibling(); | ||
| return Some(child); | ||
| if self.start == self.end { | ||
| if self.init_end { | ||
| return None; | ||
| } else { | ||
| self.init_end = true; | ||
| return self.start.clone(); | ||
| } | ||
| } | ||
|
|
||
| if let Some(start) = self.start.take() { | ||
| self.start = start.next_sibling().map(|child| { | ||
| child | ||
| .downcast::<T::Child>() | ||
| .expect("The type of children does not match.") | ||
| }); | ||
| if self.start == self.end { | ||
| if self.init_end { | ||
| return None; | ||
| } else { | ||
| self.init_end = true; | ||
| return self.start.clone(); | ||
| } | ||
| } | ||
| return self.start.clone(); |
There was a problem hiding this comment.
What do you think about this?
if self.done {
None
} else {
// Handle cases where only one child exists and
// when all but one widget were consumed
if self.start == self.end {
self.done = true;
self.start.clone()
} else if let Some(start) = self.start.take() {
// "Increment" the start child
self.start = start.next_sibling().map(|child| {
child
.downcast::<T::Child>()
.expect("The type of children does not match.")
});
// Just to make sure the iterator ends next time
// because all widgets were consumed
self.done = self.start.is_none();
Some(start)
} else {
None
}
}There was a problem hiding this comment.
Wait a minute, I think I've overlooked a case where you use next_back().
There was a problem hiding this comment.
I've tested your proposal, seems like it's working - all tests pass.
There was a problem hiding this comment.
Never mind, I actually think it's correct to use what I suggested, if DoubleEndedIterator does the same thing in reverse. Just had to go through some cases in my mind, but couldn't find one that fails.
RelmRemovableExt::remove_all. It relies on widget internal structure and may break between releases, which would be hard to maintain.RelmWidgetExt::iter_children,RelmWidgetExt::iter_children_reverseandRelmWidgetExt::for_each_children. Those aren't really reliable and can yeild surprising results on some widgets, likegtk::HeaderBar(see link).RelmBoxExt,RelmListBoxExt,RelmFlowBoxExtandRelmGridExt. Add tests for those extensions.GTK4 encourages widget-specific implementations, instead of generalizations. See link.