Add LayerSet::get_or_create_layer

[RickyDaMa]

This one's a pain to do intuitively because of the borrow checker

You want to do something like:

match ufo.layers.get_mut("foobar") {
    Some(layer) => layer,
    None => ufo.layers.create_layer("foobar")?,
}

But the borrow checker won't let you because of multiple mutable borrows (which is arguably incorrect since None shouldn't be considered tied to ufo.layers, but improving the borrow checker is beyond my skill)

So what I've implemented is accepted by the borrow checker and doesn't iterate through layers multiple times. I've pulled out the happy path of new_layer to avoid the code duplication, hence the creation of the private function create_layer. Bikeshedding the naming is welcome because it's definitely not amazing, but I just wanted to get the PR out there and worry about naming later

[madig]

I think I removed this method in #255, also see the reasoning. If you come up with something robust, I'd want it back in :)

[RickyDaMa]

Haha didn't realise it was originally in the library but removed. I have a use for it and have basically implemented a worse version in my own code (since I don't have access to LayerSet's internals), so I'd like it back if possible :)

[madig]

If you can create something that deals with the pain points we found earlier, certainly! We can look at it together.

[RickyDaMa]

I can't really see any pain points from previous discussion other than "we don't use this" and "this method is getting ugly/complex". The latter I think I've solved by pulling out the common logic from new_layer, and taking advantage of invariants assured by searching for the layer before creating it.

Let me know if I've missed anything, because currently I don't understand what's wrong with the method or my implementation of it

[cmyr]

So one of the reasons we got rid of this was that we weren't using it, and I don't like holding onto API that isn't being used. If you are using it, though, then that is no longer a concern.

In any case, the new method here feels much less of a wart than the previous one, I think because we didn't have NamingError originally?

I have one note inline, but I'm happy to merge this, and then I'll get a release out when that's done. :)

[cmyr]

I think we still want to check that we aren't DEFAULT_LAYER_NAME, since it's possible that the default layer has another name.

See this test case that was deleted when we removed the previous version:

#[test]
#[should_panic(expected = "reserved")]
fn get_or_create_false_default_layer() {
    let mut layer_set = LayerSet::default();
    layer_set.rename_layer("public.default", "foreground", false).unwrap();
    layer_set.get_or_create("public.default");
}

and... I think honestly it's easiest/hardest to screw up if t his does just call the old new_layer method if we find nothing. In practice this isn't called much, and iterating even small-10s of layers is still going to be very fast, and then we can be confident we're always using the same logic.

[RickyDaMa]

Addressed in latest commit

[RickyDaMa]

Changed get_or_create_layer to call new_layer, got rid of create_layer. Added test for creating default layer name

[cmyr]

Thanks!

[cmyr]

(I've release 0.9.0)