Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fold LayerInfo into Layer #100

Closed
wants to merge 1 commit into from
Closed

Fold LayerInfo into Layer #100

wants to merge 1 commit into from

Conversation

madig
Copy link
Collaborator

@madig madig commented Apr 3, 2021

I don't think they need to be separate.

I'm not entirely sure about Layer::new_named taking a layer and dir name, but I suppose that's hard to avoid until we have a dedicated LayerSet stuct attached to Ufo that is essentially a HashMap and manages name and path name (collisions).

@github-actions
Copy link

github-actions bot commented Apr 3, 2021

🗜 Bloat check ⚖️

Comparing 96d24d5 against 4d9805d

target old size new size difference
target/release/examples/open_ufo 1.77 MB 1.77 MB -72 Bytes (-0.00%)
target/debug/examples/open_ufo 7.15 MB 7.15 MB 1.55 KB (0.02%)

@cmyr
Copy link
Member

cmyr commented Apr 5, 2021

I'm trying to recall the rationale for keeping these separate. I think the idea is that a Layer is really just a bag of glyphs in some directory, but the name and path of that directory live outside of the layer, in layercontents.plist?

I don't have strong opinions here, but looking through the code it isn't clear that this patch improves ergonomics in a meaningful way? Having one less type is nice.

Maybe one way to think about LayerInfo is that our Vec<LayerInfo> is our LayerSet?

@madig
Copy link
Collaborator Author

madig commented Apr 5, 2021

Maybe one way to think about LayerInfo is that our Vec is our LayerSet?

I think: yes. However, not quite, as e.g. ufoLib2's LayerSet prevents deletion of the default layer, making get_default_layer never fail.

So maybe, instead, I should turn LayerInfo into a

struct LayerSet {
    contents: HashMap<LayerName, OsPath>,
    layers: HashMap<LayerName, Layer>
}

?

@cmyr
Copy link
Member

cmyr commented Apr 5, 2021

I'm starting work today on the python bindings project, so maybe let's leave this for now and then when I go to implement the python LayerSet I'll take a look then and try to figure out a solution?

@cmyr cmyr closed this in #106 Apr 18, 2021
@cmyr cmyr deleted the fold-layerinfo-into-layer branch July 29, 2021 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants