From 495dcf9d054e3e1c51faf148137602b035fd4807 Mon Sep 17 00:00:00 2001 From: David Wendt Date: Mon, 9 Nov 2020 18:43:46 -0500 Subject: [PATCH] core: Remove direct execution list head manipulation and the `replace_at_depth` flag that indicates that it has occured. Depending on how I'm reading the old code I replaced, it appeared to be constructing execution lists backwards. I have no idea if this was intended behavior or not. If so, then I'll need to add reverse-add capability to `replace_at_depth`. --- core/src/avm1/globals/movie_clip.rs | 7 ++- core/src/display_object/button.rs | 21 ++------ core/src/display_object/container.rs | 73 +++++++++------------------ core/src/display_object/movie_clip.rs | 2 +- 4 files changed, 30 insertions(+), 73 deletions(-) diff --git a/core/src/avm1/globals/movie_clip.rs b/core/src/avm1/globals/movie_clip.rs index f29a2a269..f3d287e3b 100644 --- a/core/src/avm1/globals/movie_clip.rs +++ b/core/src/avm1/globals/movie_clip.rs @@ -521,7 +521,7 @@ fn attach_movie<'gc>( { // Set name and attach to parent. new_clip.set_name(activation.context.gc_context, &new_instance_name); - movie_clip.replace_at_depth(&mut activation.context, new_clip, depth, false); + movie_clip.replace_at_depth(&mut activation.context, new_clip, depth); let init_object = if let Some(Value::Object(init_object)) = init_object { Some(init_object.to_owned()) } else { @@ -572,7 +572,7 @@ fn create_empty_movie_clip<'gc>( // Set name and attach to parent. new_clip.set_name(activation.context.gc_context, &new_instance_name); - movie_clip.replace_at_depth(&mut activation.context, new_clip.into(), depth, false); + movie_clip.replace_at_depth(&mut activation.context, new_clip.into(), depth); new_clip.post_instantiation( &mut activation.context, new_clip.into(), @@ -627,7 +627,6 @@ fn create_text_field<'gc>( &mut activation.context, text_field, (depth as Depth).wrapping_add(AVM_DEPTH_BIAS), - false, ); text_field.post_instantiation( &mut activation.context, @@ -697,7 +696,7 @@ pub fn duplicate_movie_clip_with_bias<'gc>( { // Set name and attach to parent. new_clip.set_name(activation.context.gc_context, &new_instance_name); - parent.replace_at_depth(&mut activation.context, new_clip, depth, false); + parent.replace_at_depth(&mut activation.context, new_clip, depth); // Copy display properties from previous clip to new clip. new_clip.set_matrix(activation.context.gc_context, &*movie_clip.matrix()); diff --git a/core/src/display_object/button.rs b/core/src/display_object/button.rs index 2155d1a4c..31512a496 100644 --- a/core/src/display_object/button.rs +++ b/core/src/display_object/button.rs @@ -120,13 +120,6 @@ impl<'gc> Button<'gc> { context: &mut crate::context::UpdateContext<'_, 'gc, '_>, state: ButtonState, ) { - // Clear previous child execution list. - for child in self.iter_execution_list() { - child.set_next_sibling(context.gc_context, None); - child.set_prev_sibling(context.gc_context, None); - } - self.set_first_executed_child(context.gc_context, None); - let movie = self.movie().unwrap(); let mut write = self.0.write(context.gc_context); write.state = state; @@ -135,7 +128,7 @@ impl<'gc> Button<'gc> { ButtonState::Over => swf::ButtonState::Over, ButtonState::Down => swf::ButtonState::Down, }; - write.container.clear(); + write.container.clear(context.gc_context); let mut new_children = Vec::new(); for record in &write.static_data.read().records { @@ -160,15 +153,9 @@ impl<'gc> Button<'gc> { drop(write); - let mut prev_child = None; + //TODO: This code originally reinserted children onto the execution list + //"backwards" (compared to standard behavior) - do we still want that? for (child, depth) in new_children { - // Wire up new execution list. - if let Some(prev_child) = prev_child { - child.set_prev_sibling(context.gc_context, Some(prev_child)); - prev_child.set_next_sibling(context.gc_context, Some(child)); - } else { - self.set_first_executed_child(context.gc_context, Some(child)); - } // Initialize child. child.post_instantiation(context, child, None, Instantiator::Movie, false); child.run_frame(context); @@ -177,9 +164,7 @@ impl<'gc> Button<'gc> { (*self).into(), child, depth.into(), - true, ); - prev_child = Some(child); } } } diff --git a/core/src/display_object/container.rs b/core/src/display_object/container.rs index 0d50f8304..7f15ff38c 100644 --- a/core/src/display_object/container.rs +++ b/core/src/display_object/container.rs @@ -63,16 +63,11 @@ pub trait TDisplayObjectContainer<'gc>: /// render and execution lists if and only if the child was not flagged as /// being placed by script. If such a child was removed from said lists, it /// will be returned here. Otherwise, this method returns `None`. - /// - /// The `already_on_exec_list` flag signals to the container that the child - /// being replaced is already on it's execution list and should not be - /// reinserted into the list. fn replace_at_depth( &mut self, context: &mut UpdateContext<'_, 'gc, '_>, child: DisplayObject<'gc>, depth: Depth, - already_on_exec_list: bool, ) -> Option>; /// Move a child display object around in the container's depth list. @@ -115,21 +110,8 @@ pub trait TDisplayObjectContainer<'gc>: child: DisplayObject<'gc>, ) -> bool; - /// Set the head of the execution list. - /// - /// TODO: Direct execution list manipulation shouldn't be allowed. Code - /// that uses this method is free to violate execution list invariants - /// (such as turning it into a circular list, not treating prev-and-next as - /// opposites, etc). - /// - /// It's only here for compatibility with some `Button` code. If you're - /// reviewing this PR please mark this as a problem so I can finish the - /// refactor - fn set_first_executed_child( - &self, - gc_context: MutationContext<'gc, '_>, - node: Option>, - ); + /// Clear all three lists in the container. + fn clear(&mut self, context: MutationContext<'gc, '_>); /// Iterates over the children of this display object in execution order. /// This is different than render or depth order. @@ -186,14 +168,12 @@ macro_rules! impl_display_object_container { context: &mut UpdateContext<'_, 'gc, '_>, child: DisplayObject<'gc>, depth: Depth, - already_on_exec_list: bool, ) -> Option> { self.0.write(context.gc_context).$field.replace_at_depth( context, (*self).into(), child, depth, - already_on_exec_list, ) } @@ -249,15 +229,8 @@ macro_rules! impl_display_object_container { .remove_child(context, child) } - fn set_first_executed_child( - &self, - gc_context: MutationContext<'gc, '_>, - node: Option>, - ) { - self.0 - .write(gc_context) - .$field - .set_first_executed_child(node) + fn clear(&mut self, gc_context: MutationContext<'gc, '_>) { + self.0.write(gc_context).$field.clear(gc_context) } }; } @@ -321,11 +294,6 @@ impl<'gc> ChildContainer<'gc> { self.exec_list } - /// Change the head of the execution list. - pub fn set_first_executed_child(&mut self, node: Option>) { - self.exec_list = node; - } - /// Adds a child to the front of the execution list. /// /// This does not affect the render or depth lists. @@ -475,7 +443,6 @@ impl<'gc> ChildContainer<'gc> { } } else { if let Some(old_parent) = child.parent() { - //TODO: container access for non-clips if let Some(mut old_parent) = old_parent.as_container() { old_parent.remove_child(context, child); } @@ -498,17 +465,12 @@ impl<'gc> ChildContainer<'gc> { /// such children. /// /// `parent` should be the display object that owns this container. - /// - /// The `already_on_exec_list` flag indicates that the caller has already - /// added the child to the execution list, and thus this function should - /// not modify the execution list. pub fn replace_at_depth( &mut self, context: &mut UpdateContext<'_, 'gc, '_>, parent: DisplayObject<'gc>, child: DisplayObject<'gc>, depth: Depth, - already_on_exec_list: bool, ) -> Option> { child.set_place_frame(context.gc_context, 0); child.set_depth(context.gc_context, depth); @@ -546,14 +508,12 @@ impl<'gc> ChildContainer<'gc> { None }; - if !already_on_exec_list { - if let Some(removed_child) = removed_child { - self.remove_child_from_exec_list(context, removed_child); - } - - self.add_child_to_exec_list(context.gc_context, child); + if let Some(removed_child) = removed_child { + self.remove_child_from_exec_list(context, removed_child); } + self.add_child_to_exec_list(context.gc_context, child); + removed_child } @@ -643,8 +603,21 @@ impl<'gc> ChildContainer<'gc> { removed_from_render_list || removed_from_depth_list } - /// Remove all children from the container. - pub fn clear(&mut self) { + /// Remove all children from the container's execution, render, and depth + /// lists. + pub fn clear(&mut self, gc_context: MutationContext<'gc, '_>) { + let mut head = self.exec_list; + + while let Some(child) = head { + let next_head = child.next_sibling(); + + child.set_next_sibling(gc_context, None); + child.set_prev_sibling(gc_context, None); + + head = next_head; + } + + self.exec_list = None; self.render_list.clear(); self.depth_list.clear(); } diff --git a/core/src/display_object/movie_clip.rs b/core/src/display_object/movie_clip.rs index 1b28cfaa8..9e3cfcc1d 100644 --- a/core/src/display_object/movie_clip.rs +++ b/core/src/display_object/movie_clip.rs @@ -1133,7 +1133,7 @@ impl<'gc> MovieClip<'gc> { let prev_child = { let mut mc = self.0.write(context.gc_context); mc.container - .replace_at_depth(context, self.into(), child, depth, false) + .replace_at_depth(context, self.into(), child, depth) }; { // Set initial properties for child.