Move DeleteLayer to come before SelectionChanged on DeletedSelectedLayers

[eolculnamo2]

Fixes #1393

Let me know if there is some other underlying issue that needs to be addressed. I'm just skipping most of the body of render_subpath_overlays if the document.generate_transform_relative_to_viewport errors (in this case, from its inability to find the deleted folder)

[Keavon]

I went to reproduce the issue as described in #1393 and found that it doesn't actually need to be inside a folder. It happens also if you drag a box selection with the Path tool after deleting the layer. So, based on the title of this PR (but having not looked closely at your code), I ask if the solution you've taken requires it be specific to a folder? If that's the case, you may want to test if it's still an issue without involving the folder step.

Thanks!

[eolculnamo2]

Thanks for that! This change fixes the issue both when a folder is used and when its omitted, but there's still some weirdness I'd like to figure out.

The Err which is unwrapped up the stack comes from this function which seems to only be able to Err if the let Ok(folder) condition is met.

	pub fn multiply_transforms(&self, path: &[LayerId]) -> Result<DAffine2, DocumentError> {
		let mut root = &self.root;
		let mut trans = self.root.transform;
		for id in path {
			if let Ok(folder) = root.as_folder() {
				root = folder.layer(*id).ok_or_else(|| DocumentError::LayerNotFound(path.into()))?;
			}
			trans = trans * root.transform;
		}
		Ok(trans)
	}

Will keep you posted

[0HyperCube]

This sort of works around the issue without really solving it. The core issue is that we appear to handle the DeleteLayer message after the SelectionChanged message when handling DeleteSelectedLayers. This means that the selection is updated before the layer is actually deleted.

[0HyperCube]

The solution for this would be to modify the implementation of the DeleteSelectedLayers message to ensure that DeleteLayer is called before SelectionChanged.

[eolculnamo2]

Thanks, I had a feeling that what I had might not be addressing the core problem.
Is the current solution any better?

[RBird111]

I've tried the proposed changes and the crash still occurs. It looks like the source of the crash is the bezier-rs library.

Specifically in the Subpath module:

1. Inside of the iterator implementation a call to next on an empty Subpath results in a subtract with overflow error. We can kick the can down road by having it end iteration in that case

	fn next(&mut self) -> Option<Self::Item> {
		if self.subpath.is_empty() {
		    return None;
		}
		let len = self.subpath.len() - 1
		    + match self.subpath.closed {
			true => 1,
			false => 0,
		    };
		if self.index >= len {
		    return None;
		}
		let start_index = self.index;
		let end_index = (self.index + 1) % self.subpath.len();
		self.index += 1;

		Some(self.subpath[start_index].to_bezier(&self.subpath[end_index]))
	}

2. Inside the function subpath_to_svg. With the above fix in place a crash occurs in this function after attempting to index into an empty subpath so we can silently exit the function before that happens

	/// Write the curve argument to the string (the d="..." part)
	pub fn subpath_to_svg(&self, svg: &mut String, transform: glam::DAffine2) -> std::fmt::Result {
		if self.is_empty() {
		    return Ok(());
		}
		let start = transform.transform_point2(self[0].anchor);
		write!(svg, "{SVG_ARG_MOVE}{},{}", start.x, start.y)?;
		for bezier in self.iter() {
		    bezier.apply_transformation(|pos| transform.transform_point2(pos)).write_curve_argument(svg)?;
		    svg.push(' ');
		}
		if self.closed {
		    svg.push_str(SVG_ARG_CLOSED);
		}
		Ok(())
	}

These two bandaids will stop the reported crashes but an error does start appearing in the console that I can't track down:
Error: <path> attribute d: Expected moveto path command ('M' or 'm'), "Z".

Also I don't know anything about image editing so I'm not sure what a bezier or subpath are but from the way the iterator is implemented and how Subpath is being blindly indexed into there's an assumption that there should always be at least one element even in the case of an 'empty' iterator. Something is invalidating that assumption and it's probably the core problem.

[eolculnamo2]

demo.mp4

Thanks for trying it out... The fix seems to work when I try it locally -- am I missing something that you're trying? Here's a video that demos this. It shows it breaking with the current code then I make the change in the PR , wait for it to build, then show it without breaking.

[0HyperCube]

Looks good.

[0HyperCube]

@RBird111

1. We haven't really considered the case of empty subpaths. The user probably shouldn't be able to create them (as it would be confusing), but we should also try not to panic where possible.
2. The svg path element defines an arbitrary set of curves, with a syntax like M10,0 L10,10 (to move to (10,0) and draw a line from there to (10,10)). When dealing with an empty subpath, we likely produce an invalid path, which we should fix.

[Keavon]

Hi @eolculnamo2, thanks again for this code contribution to the project! We're still hoping you will respond to the request to relicense this code. Please see #4208 ASAP, thank you!