Ladybird: Restore ability to load emoji from Ladybird #23643
Conversation
github-actions
bot
added
the
👀 pr-needs-review
Ladybird/FontPlugin.cpp
Outdated
| @@ -26,7 +26,7 @@ FontPlugin::FontPlugin(bool is_layout_test_mode) | |||
| Gfx::FontDatabase::set_default_font_query("Katica 10 400 0"); | |||
| Gfx::FontDatabase::set_fixed_width_font_query("Csilla 10 400 0"); | |||
|
|
|||
| Gfx::Emoji::set_emoji_lookup_path(String::formatted("{}/res/emoji", s_serenity_resource_root).release_value_but_fixme_should_propagate_errors()); | |||
| Gfx::Emoji::set_emoji_lookup_path(String::formatted("{}/emoji", s_serenity_resource_root).release_value_but_fixme_should_propagate_errors()); | |||
There was a problem hiding this comment.
Maybe good to be consistent with https://github.com/SerenityOS/serenity/pull/23016/files (?)
There was a problem hiding this comment.
I don't think that will work with installations, and that Ladybird has to load resources from s_serenity_resource_root (which changes depending on whether this is a dev build or a packaged installation)
There was a problem hiding this comment.
It's probably more correct for Ladybird to be using Core::Resource here actually, @ADKaster ?
There was a problem hiding this comment.
Iirc MacPDF always sets s_serenity_source_root to the bundle dir, and the command line Lagom tools set it to the build dir instead. That could work for cocoa ladybird too?
There was a problem hiding this comment.
Looking at it closer, I think AK has us spiritually doing the same thing in Ladybird. We default to the bundle path on mac, starting from the exe path:
serenity/Userland/Libraries/LibCore/System.cpp
Lines 1807 to 1810 in f58ca99
And then from that path, we get:
serenity/Ladybird/Utilities.cpp
Line 58 in f58ca99
Dunno what Andrew's long term plan here is for multiple lagom apps, but sounds like he has some consolidation ideas floating around:
#23304 (comment)
There was a problem hiding this comment.
I pushed Core::Resource usage for now - that would have at least told us we forgot to bundle the emoji in #23304
There was a problem hiding this comment.
Yeah in theory we should be copying the files to Build/lagom/share/emoji as part of the build, and then any app that needs them can get them from that path (app_path/../share). For macOS app bundles, we'd need to copy the files into the app bundle under resources like I did for Ladybird in that PR.
Still feels like a CFAppBundle Resource backend would be neat, then we could pretend that paths don't exist for these app bundles.
trflynn89
force-pushed
the
emoji
branch
2 times, most recently
from
8d595a2
to
7691cba
Compare
And add a verification step to the emoji data generator to ensure all emoji are listed in this file. This file will be used as a sources list in both the CMake and GN build systems. It is probably possible to generate this list. But in a first attempt, the CMake code to set the file as a dependency of a pseudo target, which would then parse the file and install the listed emoji was getting quite verbose and complicated. So for now, let's just maintain this list.
Otherwise, we are unable to render emoji on websites.
The path we were using is no longer correct, and we've been silently dropping this error. Use Core::Resource instead, which we use for most other Ladybird resources. This would have made it much more obvious that emoji were not installed with the application.
github-actions
bot
removed
the
👀 pr-needs-review
For example, this allows emoji to render on Strava.
Before:
After:
