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

Clarify server API #1863

Merged
merged 14 commits into from
Dec 19, 2024
Merged

Clarify server API #1863

merged 14 commits into from
Dec 19, 2024

Conversation

simoncozens
Copy link
Contributor

This PR defines more clearly the interface between the backend and the frontend. It does this by:

  • Creating and documenting an abstract base class in the frontend through which all communications with the backend take place.
  • Abstracting common code from fontinfo.js and editor.js into view.js
  • Adding an .on() method to the remote font object so that events from the backend are channeled (and verified) through the interface instead of being called directly on the proxy.
  • Adding typings for the font proxy object to document the available methods supplied by the backend.

@davelab6
Copy link
Member

@schriftgestalt I think you might want to take a look at this, since it will pave the way to a possible "glyphsapp as fontra client" future :)

Copy link
Collaborator

@justvanrossum justvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks quite good. Esp. the view refactoring is very welcome.

I will play with this and test a bit more, but overall I think this is good to go.

I'd have a slight preference to call the "view.js" module "view-controller.js".

@ollimeier
Copy link
Collaborator

@simoncozens Thank you very much for the great work you have done. @justvanrossum asked me to test it a bit. What I will do now. It's not code review. It's testing the app, therefore you will find for every issue I find a new post here.

@ollimeier
Copy link
Collaborator

Issues with all path operations:

Clicked 'Remove overlaps':

panel-transformation.a39d2027.js:505 Uncaught ReferenceError: unionPath is not defined
    at IconButton.onclick [as _buttonOnClick] (panel-transformation.a39d2027.js:505:55)
    at HTMLButtonElement.onclick (icon-button.a39d2027.js:76:32)

Same for the other path operations: subtractPath, intersectPath and excludePath

Bug_path-operations.mp4

NOTE: http://localhost:8000/ is your branch and http://localhost:8002/ is our main branch (for comparison if the bug already existed before.)

@ollimeier
Copy link
Collaborator

Ok, everything else seem to work just fine. This is what I tested (please let me know if there is something else you would like to be tested):

Other Transformations via transformation sidebar do work for outlines as well as for composites.

Font Info works:
Changing axis values, axes order: works.
Adding new source: works.
Changing vertical metrics values: works.
Changing font family name in font-info: works.
Changing cross-axis mapping: works.

Clipboard works:

  • copied outline from Fontra to RoboFont (glif)
  • copied outline from Fontra to Glyphs (SVG)
  • Copied outline from Fontra to. Fontra (json)

Changes in one editor window are reflected in another: works.

Adding background image: works.
Changing position of image is also reflected in another editor window.

'Find glyphs that use 'XXX'': works.

@simoncozens
Copy link
Contributor Author

Thanks for the thorough testing! I'll get the view controller renamed and fix up those boolean operations. I think there may also be some remote font calls in the fontinfo view that I haven't pulled out yet as well.

@justvanrossum
Copy link
Collaborator

justvanrossum commented Dec 18, 2024

What role does remotefont.d.ts serve right now? How do we prevent it going stale?

edit: and the other *.d.ts files.

@simoncozens
Copy link
Contributor Author

Right now, it's documentation - documentation of the contract between the client and the server - albeit documentation which can be interpreted by your code editor. We prevent it going stale the same way we prevent any documentation from going stale. :-)

In the future it may grow more uses.

@justvanrossum
Copy link
Collaborator

Would be nice to have prettier configured for it, so we at least avoid committing syntax errors.

@simoncozens
Copy link
Contributor Author

I'm not sure you need to do anything special to get prettier to format Typescript - at least VSCode here is autoformatting it without any particular config.

@justvanrossum
Copy link
Collaborator

It’s for pre-commit:

types_or: [css, javascript, json, xml, yaml, markdown, html]

return await returnFuture

return methodWrapper
async def messageFromServer(self, text):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Thinking out loud here)

While I understand and appreciate the desire make this less dynamic, the idea here was that remote.py has no domain knowledge. Adding these methods explicitly here breaks that abstraction.

Perhaps this means that this proxy class doesn't belong here and should move to fonthandler.py. Perhaps the construction of this proxy should then no longer be the responsibility of the RemoteObjectConnection class.

I'd like to think about this a bit more. No need for changes right now, in the worst case I'll refactor slightly as a follow-up PR.

@justvanrossum
Copy link
Collaborator

The unionPath etc. changes don't actually work: originally these we simple functions, and could be (and are) passed around as such. Now that they are methods, passing around Backend.unionPath doesn't work, because it loses the this association.

@justvanrossum
Copy link
Collaborator

I'm now wondering what the extra "Backend" class brings us, compared to the original good old functions.

@simoncozens
Copy link
Contributor Author

Currently, heavily baked into the design of Fontra is the idea that the frontend is going to talk to a Python server and can call whatever methods it likes so long as they are defined in Python. There's no official contract (or documentation) as to what methods are available; someone working on the client code needs to know what the server code does and vice versa. They're tightly coupled to the point of unity. And maybe that is by design.

If that is what you want, then I agree that Backend seems useless when there is only one backend, as there is currently.

But there may in the future be multiple backends. Or we might decide to replace some of the backend - the boolean path operations could happen more quickly if implemented client-side in WASM or paper.js, for example, while the font storage still requires talking to the server. Or the whole thing could go client-side using the web as storage. (To be honest, I am not just speculating here; I've already started...)

But this can only happen if there is a clean abstraction layer which sets out what the backend does and the expectations that the client has for it. With a Backend object, I can add a single line Backend = WASMBackend and the Python server gets swapped out and the WASM interface gets swapped in; you don't need to care what I'm messing around with because once I do that it's my problem, and I don't need to dig through all the code to locate all the server calls.

And actually in terms of interface design, I would argue that even if you will only ever use and support a single Python server backend, knowing where the boundary is and what the contract is is still actually helpful. The Backend object is the documentation for your client-server boundary. All calls to the server go through it, and they all happen in one place.

@justvanrossum
Copy link
Collaborator

Understood, and that makes sense. (Currently, all these calls to the server literally only exist because it was easier that way and/or there isn't a ready solution that can run in the browser. For example: I'm still hoping hoping that Kurbo path operations will happen at some point.)

That leaves us with: how to fix the current usage of unionPath and friends, which is not compatible with the static method approach.

@simoncozens
Copy link
Contributor Author

Sure, I'll take a look and fix that. Javascript is just like any other occult practice, I probably forgot to bind something before calling it.

@justvanrossum
Copy link
Collaborator

Don't forget about this beauty, which currently gives a ReferenceError:

const doUnion = pathOperationFunc === unionPath;

Copy link
Collaborator

@justvanrossum justvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'll merge.

@justvanrossum justvanrossum merged commit e230eeb into googlefonts:main Dec 19, 2024
5 checks passed
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