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

Refactor module structure #237

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Refactor module structure #237

wants to merge 14 commits into from

Conversation

PoignardAzur
Copy link
Contributor

@PoignardAzur PoignardAzur commented Dec 22, 2024

This echoes a discussion I had with Chad a few weeks ago. Parley's current structure de-facto splits into three types of items:

  • input data structures.
  • output data structures.
  • algorithms mapping the former to the latter.

(EDIT: There's also data structures for text editing which are mostly separate from the rest.)

This PR makes these four blocs into explicit modules, which should make both the code and the documentation much easier to read.

This should also help people navigate the upcoming refactor; algorithms will be changed radically, input types may be changed, output types are likely to stay the same. Explicitly marking them should make parallel work easier.

Another thing the PR does in de-tangle layout types. Currently, when you're looking for Cluster, do you find it in cluster.rs? Nope, layout/mod.rs. What about ClusterData? layout/data.rs. Splitting these types into their eponymous files makes them easier to find without IDE assistance.

@PoignardAzur PoignardAzur changed the title [WIP] Refactor module structure Refactor module structure Dec 22, 2024
@PoignardAzur
Copy link
Contributor Author

I think this is ready to merge. I'd recommend reviewing the commits individually otherwise this is a lot to take in at once.

@PoignardAzur
Copy link
Contributor Author

To be clear, this is a first stab at sorting parley's files differently.

Follow-up PRs should:

  • Move a lot of code from outputs/ to algos/.
  • Flatten the directory structure a bit.
  • Document all of this, including an ARCHITECTURE.md file.

@waywardmonkeys
Copy link
Contributor

I think this is blocked (at least) pending a comment from @dfrg

@PoignardAzur
Copy link
Contributor Author

I think this is blocked (at least) pending a comment from @dfrg

I'm not sure it is. This goes into the discussions we've had about crate ownership: if we accept that this is a Linebender community crate (and that multiple people have hands-on experience by now), then we should also accept that Chad's approval isn't necessary for a major change.

Obviously it'd be better to get Chad's input, to coordinate things, etc, but I don't think we should block on it indefinitely.

I would suggest late January as the deadline.

@DJMcNab
Copy link
Member

DJMcNab commented Jan 13, 2025

From my perspective, the changes to create the new editing module is fantastic, and I'd happily approve and merge that in as a separate PR.

I'm not going to approve the rest of it, as this is not code that I maintain.

@nicoburns
Copy link
Contributor

I am opposed to this change in it's current form. Aside from the fact that some things are miscategorised (our line-breaking algorithm now seems to live in inputs), I don't think the inputs/outputs/algos organisation of the code makes sense. Many of these input/ouput types are only inputs/outputs internally within parley and are not things that users would recognise as inputs outputs (module structure is important for user-facing documentation).

In Taffy inputs are "styles" in the style module and the output is Layout type. And IMO a broadly similar structure would make sense for parley, albeit Parley's Layout type is much "fatter". I think internal code should probably organised in modules that group an algorithm and it's inputs/outputs together. Which is loosely how things already are (although it's by no means perfect).

I agree that splitting editing out into it's own top-level module would make sense.

I reckon it would also make sense to:

  • Merge swash_convert into the util module (optionally making it a directory)
  • Merge the top-level font module into context.
  • Consider moving context and builder into a directory
  • Refactor the layout module so that impls are colocated with their struct.

There are a lot more improvements to code organisation to be made, but some of them involve actually changing what the code does too, and I think these are the low-hanging fruit.

From a meta perspective I'd suggest:

  • That none of this happens prior to a 0.3 release
  • That this happens as small, incremental PRs that can be reviewed individually.

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.

4 participants