Add option to set label fonts by thecodewarrior · Pull Request #785 · linebender/druid

thecodewarrior · 2020-03-30T22:33:57Z

Issue #774

This adds an option to Label to configure which font it uses. It's a simple implementation using a KeyOrValue<&'static str>. While it's likely possible to adjust the system to not need the cumbersome &'static str (which also doesn't allow dynamic strings), I have neither the Rust experience nor the depth of understanding of the current system necessary to know how to do that elegantly.

druid/src/widget/label.rs

futurepaul · 2020-03-30T22:49:44Z

druid/examples/styled_text.rs

+    .env_scope(|env: &mut druid::Env, data: &AppData| {
+        env.set(MY_CUSTOM_TEXT_SIZE, data.size);
+        if data.mono {
+            env.set(theme::FONT_NAME, "monospace");


The current thinking on "idiomatic" druid (a WIP) is to use a custom key and override that (like with MY_CUSTOM_TEXT_SIZE) instead of overriding a default key

Trying to set the custom font name to the current theme name causes errors due to borrowing the env when I borrow the font name. I think adjustments may need to be made to the current env system to make this work.

you might need to clone the name before passing it back in or something?

Curious to see what your problem was.

futurepaul · 2020-03-30T22:51:22Z

druid/src/env.rs

            }
        }
+
+        impl Into<Value> for &$ty {


Just flagging this as interesting, I don't understand this enough to comment usefully.

This is where we just about reach the limit of my experience and understanding of the system. Without that it's impossible to use into to turn a Key<&str> into a KeyOrValue<&str>, since you can't into a &str into a Value, and it's not possible to use KeyOrValue<String> since the Key<T> and KeyOrValue<T> have to match. I'm certain there are more elegant ways to do this, but I don't understand the system well enough to know how to implement them.

I think you want a Key<String>, which can be borrowed as a &str with a lifetime derived from the value. This is similar to how you can use a &str to check if an item exists in a HashSet<String>?

Or to clarify more: a Key<String> will return a &str when you get it from the env.

I started a topic in zulip to discuss implementation details.

cmyr

This seems generally reasonable; it's slightly annoying that we need the 'static lifetime but I can't really think of a viable workaround, unless we store all strings as Arc<str> and just clone them instead of trying to get fancy with references.

cmyr · 2020-03-31T14:33:10Z

druid/examples/styled_text.rs

+        format!(
+            "Size {:.1}{}: {}",
+            data.size,
+            if data.mono { " mono" } else { "" },


huh, I'm surprised that rustfmt doesn't break this up.

In any case, as this starts to get longer I think I would consider just implementing Display (or some to_label_text() method) for our AppData type, and then this can be clearer.

cmyr · 2020-04-01T13:43:01Z

druid/examples/styled_text.rs

+    .env_scope(|env: &mut druid::Env, data: &AppData| {
+        env.set(MY_CUSTOM_TEXT_SIZE, data.size);
+        if data.mono {
+            env.set(theme::FONT_NAME, "monospace");


you might need to clone the name before passing it back in or something?

Curious to see what your problem was.

cmyr · 2020-04-03T14:27:36Z

druid/examples/styled_text.rs

+        )
+    })
+    .with_text_color(theme::PRIMARY_LIGHT)
+    .with_text_size(MY_CUSTOM_TEXT_SIZE)


I think this should be trying to use the with_font method we've added?

cmyr · 2020-04-03T14:28:52Z

@thecodewarrior I think this is on reasonable shape; would you like to rebase and address feedback and I can take another look?

thecodewarrior · 2020-04-03T18:24:57Z

I think I've addressed all your comments. The example now uses a custom key (doing get(...).to_string() fixed the borrow issue I mentioned earlier), I implemented Display on AppData as opposed to formatting it inline in the label, and I removed the now unnecessary edit to env.rs

cmyr

cool, this looks good; one comment inline.

cmyr · 2020-04-03T18:48:50Z

druid/src/widget/label.rs


+    /// Builder-style method for setting the font.
+    ///
+    /// The argument can be a `&'static str`, `String`, or [`Key<&'static str>`].


Can you verify that this actually doesn't work with a non-static str? I would expect it to.

Closes linebender#774

thecodewarrior · 2020-04-03T19:15:01Z

Yup, it works with a plain &str as well as a plain Key<&str>. Updated the docs accordingly.

cmyr · 2020-04-17T15:22:16Z

@thecodewarrior ping on this? seems like it was very close to ready to go.

…l-font # Conflicts: # druid/examples/styled_text.rs

thecodewarrior · 2020-04-27T20:37:42Z

Everything should be ready to go. Were there any other changes that you wanted before you merged?

cmyr

Great, thanks!

thecodewarrior force-pushed the configurable-label-font branch from 600c168 to 3da113d Compare March 30, 2020 22:36

xStrom reviewed Mar 30, 2020

View reviewed changes

druid/src/widget/label.rs Outdated Show resolved Hide resolved

druid/src/widget/label.rs Outdated Show resolved Hide resolved

futurepaul reviewed Mar 30, 2020

View reviewed changes

cmyr mentioned this pull request Mar 31, 2020

Fix ownership issue with KeyOrValue #786

Merged

cmyr reviewed Apr 1, 2020

View reviewed changes

cmyr reviewed Apr 3, 2020

View reviewed changes

cmyr added the S-waiting-on-author waits for changes from the submitter label Apr 3, 2020

thecodewarrior force-pushed the configurable-label-font branch 2 times, most recently from 178cd07 to 0f8a8aa Compare April 3, 2020 18:05

cmyr reviewed Apr 3, 2020

View reviewed changes

Add option to set label fonts

b36625c

Closes linebender#774

thecodewarrior force-pushed the configurable-label-font branch from 0f8a8aa to b36625c Compare April 3, 2020 19:14

Merge remote-tracking branch 'upstream/master' into configurable-labe…

980daf6

…l-font # Conflicts: # druid/examples/styled_text.rs

cmyr approved these changes Apr 28, 2020

View reviewed changes

cmyr merged commit df88830 into linebender:master Apr 28, 2020

thecodewarrior deleted the configurable-label-font branch April 28, 2020 16:20

xStrom removed the S-waiting-on-author waits for changes from the submitter label May 15, 2020

Conversation

thecodewarrior commented Mar 30, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmyr Mar 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmyr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmyr commented Apr 3, 2020

Uh oh!

thecodewarrior commented Apr 3, 2020

Uh oh!

cmyr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thecodewarrior commented Apr 3, 2020

Uh oh!

cmyr commented Apr 17, 2020

Uh oh!

thecodewarrior commented Apr 27, 2020

Uh oh!

cmyr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cmyr Mar 30, 2020 •

edited

Loading