elements icon indicating copy to clipboard operation
elements copied to clipboard

better theming support

Open Xeverous opened this issue 5 years ago • 39 comments

I like the support of being able to set custom theme object, but not all variables which impact drawing are there

https://github.com/cycfi/elements/blob/2aea4fcd03992c642fc45bbfd38268abab482160/lib/include/elements/support/theme.hpp#L13-L20

For example, the button has its own radius variable:

https://github.com/cycfi/elements/blob/2aea4fcd03992c642fc45bbfd38268abab482160/lib/include/elements/element/gallery/button.hpp#L24-L26

and some color changes too:

https://github.com/cycfi/elements/blob/2aea4fcd03992c642fc45bbfd38268abab482160/lib/include/elements/element/gallery/button.hpp#L38-L44

Could we move all such variables to the theme so that one can create a theme which changes color and corners of all elements? I would like to be able to create eg a full square-like UI as on this screen.

Xeverous avatar Jan 21 '20 10:01 Xeverous

Yes, we can do that. Absolutely!

djowel avatar Jan 21 '20 10:01 djowel

What's the reason behind -1 here? If I try corner_radius = 0 will something break?

https://github.com/cycfi/elements/blob/2aea4fcd03992c642fc45bbfd38268abab482160/lib/src/support/draw_utils.cpp#L81-L104

Xeverous avatar Jan 21 '20 11:01 Xeverous

What's the reason behind -1 here?

Pseudo shadows with varying transparency; purely aesthetics. Same for -0.5.

If I try corner_radius = 0 will something break?

It should not; but try it out perhaps you'll uncover a bug. Also, I'm not sure if the code implementing round rects is optimized. Cairo does not have direct support for it; so it's all done in the canvas class.

djowel avatar Jan 22 '20 14:01 djowel

It should not; but try it out perhaps you'll uncover a bug. Also, I'm not sure if the code implementing round rects is optimized. Cairo does not have direct support for it; so it's all done in the canvas class.

Ah thinking about it again, there should be some checks here and constrain the corner_radius if it is allowed to be parametizable in the gallery.

djowel avatar Jan 22 '20 14:01 djowel

Got any objections for moving almost all color/geometry information into default theme? I would like to have much higher control over the look and experiment a bit (perhaps even make a theme-editor example to see changes on the go).

Currently majority of drawing functions have hardcoded values. I would like to move all or almost all of them into the theme type (it would grow significantly). The only things I would rather not touch are non-vanilla premade widgets like knobs which are intended to always look like real knobs.

See #62 for an example change which moves 32 opacity.

Xeverous avatar Feb 10 '20 21:02 Xeverous

No objections here. I think my only concern is how to organize the theme to make it easily understandable. But we can cross that bridge when we get there.

djowel avatar Feb 11 '20 00:02 djowel

Do you have any recommendations where theme object should/should not be used? I see 3 possible ways:

  1. refactor draw() to pass the theme object as a parameter
  2. widgets access theme upon draw() call and give all color/geometry info into draw utils
  3. widgets call draw utils and draw utils acess the theme

Currently it's a mix of 2 and 3. Thinking whether only 1 or only 2 or only 3 would be better.

Xeverous avatar Feb 12 '20 11:02 Xeverous

Do you have any recommendations where theme object should/should not be used? I see 3 possible ways:

  1. refactor draw() to pass the theme object as a parameter
  2. widgets access theme upon draw() call and give all color/geometry info into draw utils
  3. widgets call draw utils and draw utils acess the theme

Currently it's a mix of 2 and 3. Thinking whether only 1 or only 2 or only 3 would be better.

We used to have 1, but not anymore, because the theme is now globally accessible even at construction time via get_theme() For example:

https://github.com/cycfi/elements/blob/master/lib/include/elements/element/gallery/button.hpp#L41

djowel avatar Feb 12 '20 12:02 djowel

I don't think such at-construction use of theme is good. You need to recreate the widget to apply theme changes - I would prefer it to be only 1 or only 2 or only 3.

Xeverous avatar Feb 12 '20 13:02 Xeverous

By design, the theme should be valid at construction/elements-build time. I thought about this for quite some time until I arrived at that decision. There are problems with being able to dynamically change the theme on the fly. For one, there are things that are needed at construction time, for optimization reasons. Do you recall the issue with the labels' fonts in the ctor and at draw time? In such cases, the font should be known and set at ctor time, not draw time. The thing that you uncovered is a remnant of the "old" ways where the theme is only available at draw time in the context, and so I had to set the font every time it draws (and that I am fixing now).

Moreover, if needed, a rebuild/reboot of the elements view should be effortless, as can be seen in the examples. Basically, you just re-construct the root element held by the view. The lightweight elements hierarchy is very efficient to construct!

BTW, in case you missed it, there's a way to "override" a specific theme. You can see that in action here:

https://github.com/cycfi/elements/blob/develop/lib/src/element/menu.cpp#L201

For reference:

https://github.com/cycfi/elements/blob/develop/lib/include/elements/support/theme.hpp#L133

djowel avatar Feb 12 '20 13:02 djowel

I'm fine with reconstructing the root element, but not sure if it will actually apply the change. If the root element holds a shared pointer to some widget hierarchy, I think you need to reconstruct the entire hierarchy to apply the theme changes. Am I right?

BTW, in case you missed it, there's a way to "override" a specific theme. You can see that in action here:

This should be documented on Wiki. But then it means we can use the theme in arbitrary places in code since its a globally accessible object.

  • Do you want to allow to use theme object in any place (at any time past static init before main)?
  • Do you want to allow run-time theme change, provided the library user reconstructs widget hierarchy?

Xeverous avatar Feb 12 '20 13:02 Xeverous

I'm fine with reconstructing the root element, but not sure if it will actually apply the change. If the root element holds a shared pointer to some widget hierarchy, I think you need to reconstruct the entire hierarchy to apply the theme changes. Am I right?

You rebuild the view content. For example, here's the point: https://github.com/cycfi/elements/blob/master/examples/buttons/main.cpp#L137

In that example, just call:

   view_.content(
      {
         share(make_controls(view_)),
         share(background)
      }
   );

and it will rebuild the entire thing, with all the shared stuff linking just fine. The root (content) is the root and cannot/should not link to anything. It can't because the view owns it privately.

BTW, in case you missed it, there's a way to "override" a specific theme. You can see that in action here:

This should be documented on Wiki. But then it means we can use the theme in arbitrary places in code since its a globally accessible object.

  • Do you want to allow to use theme object in any place (at any time past static init before main)?
  • Do you want to allow run-time theme change, provided the library user reconstructs widget hierarchy?
  • Yes, that should be in the docs when we get the documenting the theme. The theme is a very important part of elements.
  • The theme should be valid at view construct time when the root element (aka view content) is set by the client. That happens after window construction. Never before main. So the rule here is NEVER hold an element globally. It should always be constructed and passed on to the view for lifetime management.
  • Yes, we should allow run-time theme change, globally and locally via override_theme. override_theme uses the RAII idiom and restores the original theme value on scope exit --a very powerful concept (e.g. I use it to disable a menu item visually and many more uses to come).
  • Always defer theme access as much as possible at draw time to allow dynamic overriding. The ctors access to the theme may only be as hint, so it knows what to do and perhaps hold on potentially heavy things fonts. For example, the label's size is not an absolute size, but a scale that is multiplied by the theme's label font size:

https://github.com/cycfi/elements/blob/master/lib/src/element/misc.cpp#L119

Aside: you might notice that a one line above is: _font.c_str() which is actually a bad idea because it does not allow overriding. I am thinking of a ways to fix that. It might be that theme at ctor time held in a member variable conflicts with theme overriding after all. Hah! Let's ponder on it some more...

I hope I am making sense, almost thinking out loud! Let's talk about this some more in our next voice call.

djowel avatar Feb 12 '20 14:02 djowel

Aside: you might notice that a one line above is: _font.c_str() which is actually a bad idea because it does not allow overriding. I am thinking of a ways to fix that. It might be that theme at ctor time held in a member variable conflicts with theme overriding after all. Hah! Let's ponder on it some more...

Ah got it!... See the latest labels work-in-progress code here:

https://github.com/cycfi/elements/blob/text_revisit/lib/src/element/misc.cpp#L117)

  1. The theme should hold actual font objects, not strings.
  2. The _font member in the label should be optional (e.g. std::optional<font>). That way, we use the theme's label font if it is not set, and use the user supplied font at its ctor otherwise.

djowel avatar Feb 12 '20 14:02 djowel

You rebuild the view content. For example, here's the point:

I get the example but I mean such situation:

    auto ptr = share(make_controls(view_));
    view_.content(ptr, share(background));

    // change theme...

    view_.content(ptr, share(background));

Will this reapply the theme? I think not. I think I need to call make_controls again to change the theme.

Always defer theme access as much as possible at draw time to allow dynamic overriding. The ctors access to the theme may only be as hint, so it knows what to do and perhaps hold on potentially heavy things fonts.

That's exactly what I was asking for - a recommendation when a theme object should be used.

  1. The theme should hold actual font objects, not strings.
  2. The _font member in the label should be optional (e.g. std::optional). That way, we use the theme's label font if it is not set, and use the user supplied font at its ctor otherwise.

Shouldn't this be a sort of font pointer/reference? If I have multiple labels, which all use the same but non-standard font I don't want to construct multiple expensive font objects.

Xeverous avatar Feb 12 '20 15:02 Xeverous

You rebuild the view content. For example, here's the point:

I get the example but I mean such situation:

    auto ptr = share(make_controls(view_));
    view_.content(ptr, share(background));

    // change theme...

    view_.content(ptr, share(background));

Will this reapply the theme? I think not. I think I need to call make_controls again to change the theme.

Just don't do that :-) The elements inside a view should be self-contained. Yes, just call make_controls again. Elements construction should be fast.

Always defer theme access as much as possible at draw time to allow dynamic overriding. The ctors access to the theme may only be as hint, so it knows what to do and perhaps hold on potentially heavy things fonts.

That's exactly what I was asking for - a recommendation when a theme object should be used.

Ideally, anywhere where there's a context.

  1. The theme should hold actual font objects, not strings.
  2. The _font member in the label should be optional (e.g. std::optional). That way, we use the theme's label font if it is not set, and use the user supplied font at its ctor otherwise.

Shouldn't this be a sort of font pointer/reference? If I have multiple labels, which all use the same but non-standard font I don't want to construct multiple expensive font objects.

The font is a ref-counted handle. Same fonts are shared: https://github.com/cycfi/elements/blob/text_revisit/lib/src/support/font.cpp

djowel avatar Feb 12 '20 15:02 djowel

The font is a ref-counted handle.

Great, that solves both lifetime and construction cost problems.

I guess I should probably wait a bit with theme-related PRs as I will probably get conflicts with text_revisit.

Xeverous avatar Feb 12 '20 17:02 Xeverous

Seems I need to add another dependency: pango. It is the recommended library for text handling in cairo anyway. The font feature set has all we need and more: https://developer.gnome.org/pango/stable/pango-Fonts.html

@Xeverous, see if you can deal with the windows install. I pushed a test example (named typography) to the text_revisit branch. It build and runs on OSX. See if you can get that to build and run in Windows.

djowel avatar Feb 14 '20 12:02 djowel

@Xeverous, see if you can deal with the windows install.

This is why I sometimes truly hate building specific C and C++ libraries.

  • linked pango website has no building manual, actually only API reference
  • links to prebuild Windows binaries on https://pango.gnome.org/Download are 404
  • Pango repo readme says Most of the code of Pango is licensed under the terms of the GNU Lesser Public License (LGPL) - see the file COPYING for details. but the COPYING file is just a copy of LGPL text and never precises the meaning of "most of the code" and what are the other licenses
  • Pango, like cairo has a multitude of build options. We absolutely must document which options are required - in case of cairo they even affect public API.
  • I found https://gitlab.gnome.org/GNOME/pango/blob/master/README.win32 in the repo but it looks like a pain to get through. I already know it won't be in conan since they don't have cairo either.
  • Pango has multiple subdependencies (some shared with cairo), where each of them might be another pile of problems. Pango says it requires Glib for Windows but Glib doesn't even document how to build it on non-unix systems.

I will try my best and see what can be done. I hope meson will turn out to be better than autotools - cairo uses them and the Windows build is broken by design because autotools explicitly document they don't support special characters in paths and Windows by default stores all dependencies in Program Files.

I might aswell publish my multi-page notes how to build cairo with MinGW.

Please document all required Glib/Pango/Cairo options.

Xeverous avatar Feb 14 '20 15:02 Xeverous

This is why I sometimes truly hate building specific C and C++ libraries.

Yeah, I know... That is why I tried as much as I can to avoid pango, although I know from the start that the cairo docs says we should avoid their "toy" text/font APIs and use non-toy APIs like that of pango. As for the license, we will only be using a strict subset of pango for font selection and basic text layout.

djowel avatar Feb 15 '20 00:02 djowel

  • links to prebuild Windows binaries on https://pango.gnome.org/Download are 404

Perhaps this can help: https://github.com/ImageMagick/pango

djowel avatar Feb 15 '20 00:02 djowel

BTW, here's the full text layout stack for reference: State of Text Rendering

It is not trivial at all!

The section on "Consumers" is revealing and somehow points to using HarfBuzz directly. (HarfBuzz uses cmake, btw)

djowel avatar Feb 15 '20 01:02 djowel

Forget Pango for now :-/... It is broken on MacOS unless you add the environment variable PANGOCAIRO_BACKEND=fc which forces it to use fontconfig. I think we're better off just using fontconfig+ HarfBuzz or maybe just fontconfig+cairo.

djowel avatar Feb 16 '20 01:02 djowel

OK, that worked. Simply: fontconfig+cairo. Now I need to fugure out how to get fontconfig into the windows build. Perhaps https://github.com/tgoyne/fontconfig will help.

Anyway, text_revisit branch works fine on OSX.

djowel avatar Feb 17 '20 14:02 djowel

typography

djowel avatar Feb 17 '20 14:02 djowel

Oh, we might not support synthesizing styles.

Note: On Mac with Cocoa, Bold, Italic and BoldItalic must have the proper font, or the style is simply not available. It was not always the case. Carbon used to synthesize these styles by playing on weight and slant. Windows still synthesizes Bold, Italic and BoldItalic when the specific stylistic variation font is not present.

But that practice is going out of style, IMO. Synthesized styles will never be as good as true weighted fonts (e.g. bold) and italics. Modern fonts support many style variations.

https://docs.huihoo.com/apple/wwdc/2012/session_226__core_text_and_fonts.pdf "CoreText does not synthesize font styles (bold, italic) • Clients may choose to do something for compatibility reasons ■ Italic: Skewed font matrix ■ Bold: No best approach"

djowel avatar Feb 17 '20 14:02 djowel

Wow, the image looks really good.

But that practice is going out of style, IMO. Synthesized styles will never be as good as true weighted fonts (e.g. bold) and italics. Modern fonts support many style variations.

Does it mean elements will not support rendering fonts with such styles if they are not in the font itself? I'm fine with it, jus no bad limitations like "not being able to use the font at all when a subset of styles is not in it".

OK, that worked. Simply: fontconfig+cairo. Now I need to fugure out how to get fontconfig into the windows build. Perhaps https://github.com/tgoyne/fontconfig will help.

fontconfig is already a dependency of cairo. I have already built it. Now just need to add it to the CMake recipe.

Xeverous avatar Feb 17 '20 14:02 Xeverous

Wow, the image looks really good.

But that practice is going out of style, IMO. Synthesized styles will never be as good as true weighted fonts (e.g. bold) and italics. Modern fonts support many style variations.

Does it mean elements will not support rendering fonts with such styles if they are not in the font itself? I'm fine with it, jus no bad limitations like "not being able to use the font at all when a subset of styles is not in it".

Did you know that Adobe graphics programs such as Photoshop and Illustrator do not fake bold and italic fonts? And indeed, the MacOS, known for its superiority in typography (and graphics) stopped doing so in the OS level. It makes sense, if you think about it. Faking bold and italics were a necessity in the old days when we have limited fonts available and computer typography was in its infancy.

I am an artist and I want Elements to have superiority in aesthetics and design. I have a keen eye on such things and I dislike faking things, especially now that you do not have to because there are a LOT of superb font families available.

Perhaps these articles will be more convincing: Say No to Faux Bold The Right Bold & Italic for the Right Job

OK, that worked. Simply: fontconfig+cairo. Now I need to fugure out how to get fontconfig into the windows build. Perhaps https://github.com/tgoyne/fontconfig will help.

fontconfig is already a dependency of cairo. I have already built it. Now just need to add it to the CMake recipe.

Wonderful!

djowel avatar Feb 17 '20 19:02 djowel

I am an artist and I want Elements to have superiority in aesthetics and design. I have a keen eye on such things and I dislike faking things

I like this approach. I know that fake bold does not look really good (already seen it a couple of times when experimenting with bold in CSS, which has 8 levels IIRC - lots of them look the same or have very irregular line thickness depending on the font).

I like your attention to details. Recently found an article that was about how many UIs commit the mistake of non-assymetric ranges (where asymetric ranges are standard, basically [first, last) where the dist = last - first and last points 1 past actual last object). Because of this mistake, it was impossible to either move a widget to the top or bottom without 2+ drags and drops (always appeared 1-off the maximum due to lack of "1-past last element" logic for cursor moves).

Xeverous avatar Feb 17 '20 20:02 Xeverous

I like this approach. I know that fake bold does not look really good (already seen it a couple of times when experimenting with bold in CSS, which has 8 levels IIRC - lots of them look the same or have very irregular line thickness depending on the font).

Indeed, a well-designed application should not have any fake fonts in its UI.

djowel avatar Feb 18 '20 02:02 djowel

How about a theme editor example? I noticed some colors don't play nicely (eg black background because a lot of code uses color.level() and anything multiplied by 0 is 0) and I think it would be good to have a playing ground for experimenting with different themes. This could also help catching some bugs in case some elements have hardcoded color values or use the theme object incorrectly.

Xeverous avatar May 26 '20 15:05 Xeverous