AlgebraicJulia / Semagrams.jl

A graphical editor for graph-like structures
https://algebraicjulia.github.io/Semagrams.jl/
MIT License
91 stars 10 forks source link

Reversion: API Change for Viewports and out of date comments #140

Open olynch opened 1 year ago

olynch commented 1 year ago

@sjbreiner I should have caught this when reviewing your PR, but you changed the API for viewports from viewports having their own element to viewports attaching children to the main window. Except, only the MainViewport attaches children to the main window. And deregistration does nothing to change this. So essentially the entire viewport API is now pointless; the whole point was that different things could have different scaling and with your current design this no longer is possible.

Now, I think that it makes sense to refactor this anyways, because I think that EditorState is going to be split up into things that are window-level, and things that are Semagram-level, and perhaps this removes the necessity of having separate viewports because what we termed "UI" could now be handled outside of the Semagram itself, though perhaps still overlaid.

That being said... it was not best development practices, and further more the comments are out of date now. Again, I should have caught this while reviewing, but in the future when breaking an API please remember to change the comments.

sjbreiner commented 1 year ago

@olynch You're looking at this line?

  // Attach all of the root elements of the viewports to the main element
  elt.amend(
    children <-- viewports.signal.map(_.map(_._2.elt).toSeq),
    events --> Observer[Event](evt =>
      dispatcher.unsafeRunAndForget(eventQueue.offer(evt))
    )
  )

I think I changed it from this:

  elt.amend(
    children <-- viewports.signal.map(_.map(_.elt).toSeq),
  )

I thought that was doing essentially the same thing, just using a dictionary rather than a set. What would be a better formulation?