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

Fix font #3108

Merged
merged 18 commits into from
Aug 3, 2023
Merged

Fix font #3108

merged 18 commits into from
Aug 3, 2023

Conversation

jmercouris
Copy link
Member

I can't seem to load fonts from a local resource. It is not allowed.

Not sure how to circumvent this without running our own HTTP server to load the assets. This seems way overboard though. Any ideas @atlas-engineer/atlas ?

@hgluka
Copy link
Contributor

hgluka commented Jul 31, 2023

Maybe data URLs could work? We even have a convenient make-data-url function in web-extensions.lisp, although it's not exported.

I don't know if there are any drawbacks performance-wise, though.

@aartaka
Copy link
Contributor

aartaka commented Jul 31, 2023

I can't seem to load fonts from a local resource. It is not allowed.

Not sure how to circumvent this without running our own HTTP server to load the assets. This seems way overboard though. Any ideas @atlas-engineer/atlas ?

What Epiphany does is defining their own ephy-resource:// custom theme, which can gain access to the local files. And then they use ephy-resource links to load bundled resources.

Here's a definition:

webkit_web_context_register_uri_scheme (priv->web_context, "ephy-resource",
                                        (WebKitURISchemeRequestCallback)ephy_resource_request_cb,
                                        NULL, NULL);
webkit_security_manager_register_uri_scheme_as_secure (webkit_web_context_get_security_manager (priv->web_context),
                                                       "ephy-resource");

What we can do is:

  • Include all the static assets as :static-files with ASDF.
  • Define a scheme that's resolve files with ASDF.
  • And pack these assets together with Nyxt (should be automatic on Guix, but could be non-trivial elsewhere.)

We can mimic Epiphany with:

(define-internal-scheme "nyxt-resource"
  (lambda (url buffer)
    #|...|#
    (asdf:system-relative-pathname :nyxt (quri:uri-path (url url)))
    #|...|#)
  :secure-p t)

@aartaka
Copy link
Contributor

aartaka commented Jul 31, 2023

Maybe data URLs could work? We even have a convenient make-data-url function in web-extensions.lisp, although it's not exported.

I don't know if there are any drawbacks performance-wise, though.

That's a solution we rely on in several places, yes. But it's better to avoid it.

There are several drawbacks, starting with the length restrictions for data: URLs, and up to security risks. So if we have any option to not use data: URLs, then we probably should pick that non-data: option.

@jmercouris
Copy link
Member Author

Thank you for the idea Artyom, I have implemented it! Still not sure about its portability or what asdf:static-file does. Does that mean that the system relative files will be available on a compiled system even if the binary is moved?

@jmercouris jmercouris requested a review from aartaka August 2, 2023 16:06
@jmercouris jmercouris marked this pull request as ready for review August 2, 2023 16:06
@aartaka
Copy link
Contributor

aartaka commented Aug 2, 2023 via email

@jmercouris
Copy link
Member Author

there's an NASDF file thing, perhaps that can be used. Beyond this, do you think it is ready? (I'll test the flatpak)

@jmercouris jmercouris requested a review from Ambrevar August 2, 2023 16:49
@jmercouris
Copy link
Member Author

flatpak failed, have to load the fonts into the image somehow and cache them.

@jmercouris
Copy link
Member Author

I've figured out a strategy for caching the fonts in the image. I will continue to think about it a little bit more before committing.

@aartaka
Copy link
Contributor

aartaka commented Aug 2, 2023 via email

@jmercouris
Copy link
Member Author

The problem is not on your side, static file will not do what we wish. We must load things into the image.

@jmercouris
Copy link
Member Author

@aartaka take a look at my latest commits. For some reason, the *static-data* is populated interactively, but not when compiled... I don't get it :-(

@jmercouris
Copy link
Member Author

@Ambrevar any ideas?

@jmercouris
Copy link
Member Author

note that 5bbc97d does work, but that is no way to do things...

@jmercouris
Copy link
Member Author

I have made it work even portably. Static-files is not the way to go here.

@jmercouris jmercouris merged commit e5e3f9b into master Aug 3, 2023
@jmercouris jmercouris deleted the fix-font branch August 3, 2023 15:34
@jmercouris
Copy link
Member Author

I used a merge commit because I want to show the process and show why static files doesn't work, and how it could work. Perhaps someone could pick it up in the future.

(glyph-logo
#.(alexandria:read-file-into-string
(asdf:system-relative-pathname :nyxt "assets/nyxt.svg"))
:documentation "The logo of Nyxt as an SVG.")
Copy link
Member

Choose a reason for hiding this comment

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

Why call it "glyph-logo" and not "nyxt-logo" or "application-logo"?
Does it have to be an SVG?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, has to be an SVG. Name was set to be consistent with other glyphs

@@ -178,4 +178,11 @@ Return nil on error."
(when (and commits (not (zerop commits)))
(push-feature "UNSTABLE"))))

(defvar +logo-svg+ (alexandria:read-file-into-string (asdf:system-relative-pathname :nyxt "assets/nyxt.svg")))
(export-always '*static-data*)
Copy link
Member

Choose a reason for hiding this comment

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

I suggest we unexport this and instead add a function to get the static data. This would be more future-proof.

Copy link
Member Author

Choose a reason for hiding this comment

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

A good idea!

(multiple-value-bind (data exists)
(gethash (quri:uri-path (url url)) *static-data*)
(declare (ignore exists))
data))
Copy link
Member

Choose a reason for hiding this comment

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

Why the multiple-value-bind if you are ignoring exists?
This is effectively equivalent to (gethash (quri:uri-path (url url)) *static-data*).

Copy link
Member Author

Choose a reason for hiding this comment

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

I know what you are thinking, but if you remove it, Webkit loses its mind. I didn't add it for no reason :-D

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see, this is because the callback multiple values have specific expectations for an Internal scheme, see define-internal-scheme documentation.

Anyways, to effectively discard multiple values beside the primary value, use nth-value 0 or prog1.

Done in b3ac06d.

Copy link
Member Author

Choose a reason for hiding this comment

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

I almost used nth value, since I was on the fence, we go with it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants