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

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

Open
olynch opened this issue Jun 20, 2023 · 1 comment
Open

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

olynch opened this issue Jun 20, 2023 · 1 comment

Comments

@olynch
Copy link
Member

olynch commented Jun 20, 2023

@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
Copy link
Collaborator

sjbreiner commented Jun 22, 2023

@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?

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

No branches or pull requests

2 participants