christolliday / limn

Experimental cross platform GUI library
Other
404 stars 19 forks source link

[WIP] Reexport API, add customization options + custom text + image loading #29

Closed fschutt closed 6 years ago

fschutt commented 6 years ago

This PR fixes #22, #23 and provides an alternative for font- and image loading (from #21).

This doesn't compile yet and I probably have to rebase the changes for a "proper" documentation, but since it's a bigger task, I wanted to be transparent with what I'm doing.

Also to avoid spamming #21 too much. Comments on the PR are welcome, even though it isn't done yet. However, I've seen that #21 and #22 are highly interleaved - you can't fix the one without fixing the other.

The goal is to defer the creation of a WidgetBuilder until an item gets actually used. This enables users to override styling options, including fonts and images. This is why this is one PR - you can't fix the one without fixing the other. What this means is that I also have to fix every widget. Should't take too long since the code is already written, I basically put every option that was implicit in the SomeWidget::new() method and put it in the Into<WidgetBuilder> method.

Merging has to probably be done with --merge-strategy=own on reexport-api. However, I first want to clean up my commits (supposing this branch will be merged).

christolliday commented 6 years ago

So, I understand the temptation to deal with #21 and #22 at the same time but I really would prefer if any attempt to work on #21 was put off for the time being, I have some ideas about #21 that I think will require a far more substantial refactoring, I'll have more to say about this next week. Also fixing of re-exports should also be done in a separate PR.

For font handling specifically, I think specifying TextState and by extension WidgetBuilder APIs that specify fonts, should only be concerned with font names, sizes and other font parameters rather than passing around Arc<Font>.

So for instance, in this example:

fn my_application(root: &mut WidgetBuilder) {
    let default_font = limn::resources::get_global_resources().add_font(
               "Roboto", Font::try_from_file("Roboto.ttf").unwrap());

    let button1 = ToggleButtonBuilder::new();
    let mut button2 = ToggleButtonBuilder::new().with_text("hello", "Roboto");
    // instead of
    // let mut button2 = ToggleButtonBuilder::new().with_text("hello", default_font.clone());
    ...
}

Also I'm not sure why you removed the distinction between a font and a font instance, a font just holds the data for a loaded font, whereas a font instance caches data about a font at a specific size, there should be a difference.

fschutt commented 6 years ago

My reasoning for that was: If you have an Arc<Font> (or maybe I should use an Arc<FontInstance>), it is guaranteed that the font is still there. If you pass "Roboto" around, it isn't guaranteed that you actually have a font. Then you'd have to check every time that there is a font.

Later on I want to be able to invoke garbage-collection on the fonts and / or safely delete them - i.e. the user should be able to say "remove all fonts that aren't needed". This is important for me because I'm trying to build a file explorer where you could go into a directory with lots and lots of fonts. If I'd just add fonts, I'd be out of memory very soon. If you have an Arc<Font>, you can simply check the strong count to see if anyone is still using it, if not, delete it. This wouldn't be possible with just passing around the name.

Maybe an Arc<Font> isn't the best idea, rather an Arc<FontInstance>.

Regarding the FontInstance, yes, I will probably need to reintroduce it (currently the FontInstanceId is the replacement, I should rename this back). I'll need to reintroduce texture descriptors - this isn't finished yet.

The problem with remaking the Fonts is simply that it "bubbles up" - if you redo the font to be explicitly required, now the TextBuilder wants to know where to get the font from. If you redo the TextBuilder to introduce the concept of passing in a Font, now everything that depends on TextBuilder complains about: "How should I pass a font to the text builder?". And so you end up redoing every widget.

I'll do #23 in a seperate PR, though, you're right about that. Again, this is why I'm making the PR early, to get feedback.

For #21, I'm curious to see what your idea is. My idea (with this PR) was to make with this is to delay the creation of a WidgetBuilder until it is added to the UI. This way users can still override styling to their liking. If you'd want to introduce gradients, for example, you could just have a default gradient. If a user doesn't like that, he can override it. When the element is added to the UI, it will then use the new gradient. I hope I'm not working against you in this regard.

christolliday commented 6 years ago

My reasoning for that was: If you have an Arc (or maybe I should use an Arc), it is guaranteed that the font is still there. If you pass "Roboto" around, it isn't guaranteed that you actually have a font. Then you'd have to check every time that there is a font.

There will always be cases where you will want to or have to specify the font by name, either to make widget definitions that are reusable without bundling fonts, or when eventually you can serialize style/ui descriptions, in json or whatever. I'd rather not have separate or "weaker" APIs for those use cases since they are actually more important. Aside from the simplicity of having one way of specifying a font, it also gives motivation to improve the usability of this use case, by making the rules for it simple and well documented.

To clarify, I'm saying there should be separate multiple ways of loading or registering a font, but only one of selecting a font.

Later on I want to be able to invoke garbage-collection on the fonts and / or safely delete them - i.e. the user should be able to say "remove all fonts that aren't needed". This is important for me because I'm trying to build a file explorer where you could go into a directory with lots and lots of fonts. If I'd just add fonts, I'd be out of memory very soon. If you have an Arc, you can simply check the strong count to see if anyone is still using it, if not, delete it. This wouldn't be possible with just passing around the name.

What TextState could do is get and cache an Rc<FontInstance> from resources after the first time it draws, and reset it when the style changes, this will allow fonts to be loaded as lazily as possible, and allow resources to have access to the reference count.

Then you can either just have it delete fonts as soon as nothing is using it to draw, or at least know when nothing is using it to draw if you want to manually trigger a font cleanup.

christolliday commented 6 years ago

For #21, I'm curious to see what your idea is. My idea (with this PR) was to make with this is to delay the creation of a WidgetBuilder until it is added to the UI. This way users can still override styling to their liking. If you'd want to introduce gradients, for example, you could just have a default gradient. If a user doesn't like that, he can override it. When the element is added to the UI, it will then use the new gradient. I hope I'm not working against you in this regard.

Oh sorry I think I said #21 when I meant #22 before, but I think that's what you're talking about too.

There isn't really a concrete plan here for styling, just some loose ideas, I'll write them up in that issue. actually the changes here don't seem like a big deal, mostly I'd just prefer it be in a separate PR. It does make sense to delay setting up the WidgetBuilder until the end, this is what SliderBuilder does, mostly. Rather than create the WidgetBuilder at the end though, it should create the WidgetBuilder at the beginning, but only create it, and delay configuring it up until the end. That way you can still create a WidgetRef from it at any time, use it to layout other widgets without attaching it etc.

For the re-exports actually I implemented that myself as part of some other clean up, so I'll merge that soon.

fschutt commented 6 years ago

While I haven't forgotten about this PR, I have a lot of work to do. @christolliday it might be better to take the API that I proposed for the font and image loading and the improvements for the WidgetBuilder and work on your own PR for this. I can't say that this PR will get any attention until next year.

I have built my own ad-hoc layout framework which doesn't work very well, but for the stuff I need to do right now, it's OK. I'm sorry that I can't work on this a lot. I wish you good luck and I'll definitely stay in touch with the project - to get a pure-Rust drawing / UI solution. Thank you for doing this.

christolliday commented 6 years ago

@fschutt no problem, I can take care of it, have a few things that I'll probably do first but I'll get around to it eventually ;) thanks for the thought anyway and good luck with your stuff!