Using font-loader

[krkthn]

Let's use font-loader to load the font data instead of using hard coded paths, and files.

See #4

[fschutt]

@krkthn A question: Could you make it accept an optional, default font? I don't know if the Font "Sans" is installed on all computer, so I want to embed a fallback font in my binary. I don't even expect any font to be installed on the target computer. The only "reliable" method is that I embed a fallback font in the binary - which is currently, even with your patch, impossible.

For example:

let app = limn::app::App::new(/**/)
                .with_fallback_font("myfont", Option<Vec<u8>>)
                .build();

[jrmuizel]

@fschutt wouldn't it be better to default to the default system font in that case instead of having to embed a font?

[fschutt]

@jrmuizel Several reasons:

• font-loader only supports truetype. What if the default font on the system is an OTF-font, such as "OpenSans.otf"? Boom, crashes.
• font-loader requires libfontconfig and libfontconfig1-dev (calls via FFI, lots of unwrap(), even). The space taken up by these two libraries is more than embedding a default font in the binary.
• What if I want to make a font-viewer, where the font data is loaded over the network? Both font-loader and hard-coding paths are bad solutions here.

Also listed:

• font-loader: Mac is not confirmed working, Android doesn't work, iOS doesn't work

Of course I would prefer the font to be on the system, but I don't want to risk my application deployment just because a font couldn't be loaded. Overall, limn should not concern itself with font loading. limn should have a required font_data: &[u8] bound for getting the font data. Where the data is comes from and where actually stored is something that the programmer of the final binary should be concerned with. This would also add the possibility to have multiple limn::Fonts that reference the same data (without owning the data). I think this PR is a bad idea.

The same goes for images / sprites.

[matprec]

@fschutt First of all, appreciate the feedback!

Also listed: font-loader: Mac is not confirmed working, Android doesn't work, iOS doesn't work

Mac support is provided by the rust community with direct bindings to core-text. As far as i can tell, the webrender people use it successfully (side note, they do most of the mac stuff in font-loader).

I have to admit, the Mac note regarding libfontconfig is somewhat misleading and is fixed on the repo. It's just that no code changes were introduced in the meantime, therefore the readme on crates.io isnt updated, as i don't want to publish a new version just for an readme update :)

font-loader requires libfontconfig and libfontconfig1-dev

Only on *nix systems other than mac. Tbh, those wouldnt even be needed currently, because we use servos bindings which compile and statically link automagically.

lots of unwrap()

True that, error handling could be better, issue is filed.

If you have other quality concerns, feel free to open issues! Its interface isn't cast in stone :)

I see that packing a font with the binary is a concern. Piping it through font-loader wouldn't make any sense other than to provide a coherent interface. Yet people would probably like to have an out of the box experience.

Therefore i'd suggest making it a feature, enabled by default. The font-loader code path would be a wrapper to a use_font(&[u8]) function.

[fschutt]

@MSleepyPanda Yeah, I think this is a good idea. People who don‘t want it can disable it and you can load fonts from wherever you want.

[krkthn]

Therefore i'd suggest making it a feature, enabled by default. The font-loader code path would be a wrapper to a use_font(&[u8]) function.

I implemented something like that in ccaf449 what do you think?

Could you make it accept an optional, default font? [...] The only "reliable" method is that I embed a fallback font in the binary - which is currently, even with your patch, impossible.

Having a default fallback font is not a bad idea, but I'm not really sure where to put that, I don't think that should be hard coded directly into the Resources object. You can however ship your own default now and test by hand wheather a font is available on the system or not, and if not you can fall back to your hard coded font if you want to (at least with the latest patch: ccaf449)

[matprec]

Personally, i don't like the deprecation, breaking the interface shouldn't be a problem here. Otherwise, LGTM!

[christolliday]

Hey @krkthn thanks for this, to be honest though this does feel like a small step backwards for compatibility, I generally agree with @fschutt that ease of deployment and compatibility is important. Basic functionality should be simple to use and not require work on the part of the library user.

This PR changes loading fonts in limn from being supported on any platform that rust supports, to any platform that both supports font-loader and that has all the specific fonts your app wants to use (or just the "Sans" font) already installed. I agree that the current situation with the assets directory is not ideal but at least it offers some guarantees.

At the very least I think it should detect if "Sans" or any requested font is missing and substitute it for something else that is available, but it might be better if that functionality was added to font-loader. Ideally I think the best solution would be if these features were added to font-loader.

@MSleepyPanda what do you think?

[fschutt]

1. Please don't use depreceated, because limn is 0.0.1 anyway, so it is expected to break. It is not even on crates.io.
2. I'd argue that font-loader isn't needed. If you want to use font-loader in your application, that's fine and you can. If you want to use hard-coded paths in your application, and File::open() that's fine and you can. limn however should not concern itself with this. You could add them as "convenience methods", though (see below).
3. The responsibility of how fonts should be loaded and if / how fallback fonts are structured is not up to limn. If there are font-loading functions in limn, they should return the appropriate error types. It is then up to the application programmer to handle these loading errors correctly.

I'll make a seperate PR of how I would expect the /resources module to look like, but (I think) it is not in limns responsibility to load fonts. You could add "convenience" methods for font-loader or for loading a font from a file, but please add feature-flags to reduce compile times and needed dependencies.

[christolliday]

@fschutt why would a UI library not concern itself with the loading of fonts? Your example of loading a font over the network should be possible sure, but the common cases are using fonts that are distributed with your application, or distributed with your OS and those are the cases that should be optimized for.

Also why should the functionality of having a graceful fallback to a usable application, instead of panics for missing fonts be something every app needs to re-implement? I suppose you could argue apps could depend on another library that handles it, but why shouldn't that library be font-loader and why shouldn't limn depend on it directly so that the user doesn't have to? You can add a feature-flag if you'd like if you don't want to use it, but I imagine most applications would.

[fschutt]

@christolliday I didn't mean "no loading of fonts", that was poorly expressed on my part, sorry. My vision was something like this: https://github.com/fschutt/limn/blob/reexport_api/src/resources/font.rs (working on it, doesn't compile yet). I'll do an alternative PR to show what I mean.

Sure, I'd do a feature-flag and then put it as a feature enabled by default. This way all applications can use it by default. I don't have objections against font-loader per se and I think it's a good idea, it's just that if there is no feature-flag, I now have a forced dependency on libfontconfig, which is not desirable.

[christolliday]

@fschutt ok that makes sense, if you make it so that you can still load from file or font-loader (if the feature flag is enabled) so the current functionality is maintained, would be a good first step.

Ultimately I think it would be nice if the only way loading a font panics, is if you get a NoAvailableFonts error, which can look through both system fonts, bundled fonts, manually loaded fonts or even network loaded fonts. If the specific font you request is missing it should just log an error instead of panicking. This functionality could be added later, and maybe resources is a better place for it than font-loader.

[matprec]

The API @fschutt propeses seems pretty concise and reasonable

Regarding the fallback aproach, i've commented on that here

In short: Customize font_loading behaviour by using unwrap_or

[fschutt]

Well, the thing I've thought about is an API that requires resources requiring a default font. Then it is up to the user of the library on how to do this:

// just as an example, function name may be different or wrapped differently
pub fn make_font(default_font: Font) -> FontInstanceKey { ... }

Then, the user of the library is forced to decide how to initialize the font, like this:

fn init_application() {
    let default_font = 
    limn::make_font(Font::from_font_loader("Sans"))
         .unwrap_or(Font::from_file("./fonts/Roboto.ttf"))
         .unwrap_or(Font::from_bytes(&font_bytes)?)));
    // making a font from bytes will always work, used as last resort

    let text = TextStyle::new("hello", default_font);
}

That way the user is forced into deciding what he wants to do, if he wants to panic or handle the error - but this is not up to limn.

[fschutt]

This could take a while.

In order to properly expose the font to the user, I need to basically turn all of the widgets inside out - there is way too much implicit state going on, which results in little control over the widgets themselves.

Also, the so-called "builders" are not really builders right now, like in a real builder pattern. The implicit conversion into a WidgetBuilder (via widget_wrapper!) is horrible, because it does not allow for any customization from the library user. If I as a library user want to pass in a font that is not somehow implicitly inferred by the library - there's just no way of doing it properly.

I've pushed what I have so far here: https://github.com/fschutt/limn/blob/reexport_api/src/widgets/button.rs#L106 which makes for a much cleaner usage afterwards:

// in order to use a font on a button, you have to acquire an Arc<Font>,
// which is only possible when registering it on the global resources:

fn my_application(root: &mut WidgetBuilder) {

    let default_font = limn::resources::get_global_resources().add_font(
               "Roboto", Font::try_from_file("Roboto.ttf").unwrap());

    // you can refer to the font later on as "Roboto" and reuse it

    // cloning the default font does not duplicate the font, only the reference to it
    let button1 = ToggleButtonBuilder::new(); // by default, a ToggleButtonBuilder has no text, 
                                              // so it doesn't need a font

    let mut button2 = ToggleButtonBuilder::new()
                      .with_text("hello", default_font.clone());

    // NOTE: Now you can customize the styling if you don't like it. 
    // This effectively fixes #22 - you don't need to copy the elements to customize them
    button2.button_builder.padding = Padding { top: 5.0, bottom: 500.0, left: 10.0, right: 2.0 };

    button2 = button2.with_callback(some_callback_fn);

    root.add_child(button2); // <- WidgetBuilder only gets created here, not before!

    // button1 is unused, will be optimized away (with warning)
}

The difference is that instead of making a WidgetBuilder directly at creation time, all the relevant styling functions are deferred to the Into<WidgetBuilder> call (via add_child or similar). This change in structure makes it possible for widgets to hold custom data suited to the purpose of the widget as well as for outside users to override the default style. I want to get rid of the widget_wrapper! macro, it seems like an ugly hack.

Especially the TextBuilder has a new API now, working on it: https://github.com/fschutt/limn/blob/reexport_api/src/widgets/text.rs

[christolliday]

thanks for this @krkthn! took me a while to look into this but it looks pretty good, I just rebased, fixed the conflicts and did some cleanup to make #39 so I'll close this