UI: Add Optional Flags for Tooltip Image Fields #119437

Harley Acheson · 2024-03-13T19:31:23+01:00

Harley Acheson commented

2024-03-13 19:31:23 +01:00

Add more flexibility to tooltip images by adding the ability to specify
if (one of two) checkerboards are added, border, premultiplied
blending, or recoloring.

The current tooltip images work okay in our current usages, but we need more flexibility to do things like this or that.

Why do we have two checkerboards? We have one that we use in most places which has colors that are set by the theme. But within the theme editor itself we use a checkerboard with fixed colors to always give consistent results. The former would be used for image thumbnails while the latter would be needed for previews of colors.

What is "recoloring"? When drawing images the shader has an option to shift the colors based on a passed color. An example of use is with font previews, which are always saved as white images on a transparent background. This would not be visible with light themes to we recolor with the text color so they appear dark there.

Add more flexibility to tooltip images by adding the ability to specify if (one of two) checkerboards are added, border, premultiplied blending, or recoloring. --- The current tooltip images work okay in our current usages, but we need more flexibility to do things like [this](https://projects.blender.org/blender/blender/pulls/117815) or [that](https://projects.blender.org/blender/blender/pulls/118945). Why do we have two checkerboards? We have one that we use in most places which has colors that are set by the theme. But within the theme editor itself we use a checkerboard with fixed colors to always give consistent results. The former would be used for image thumbnails while the latter would be needed for previews of colors. What is "recoloring"? When drawing images the shader has an option to shift the colors based on a passed color. An example of use is with font previews, which are always saved as white images on a transparent background. This would not be visible with light themes to we recolor with the text color so they appear dark there.

Harley Acheson added 1 commit 2024-03-13 19:31:34 +01:00

93d62db382 UI: Add Optional Flags for Tooltip Image Fields

Add more flexibility to tooltip images by adding a flag to the field to
specify if (one of two) checkerboards are added, border, premultiplied
blending, or recoloring.

Harley Acheson added this to the User Interface project 2024-03-13 19:32:02 +01:00

Harley Acheson requested review from Hans Goudey 2024-03-13 19:32:08 +01:00

Hans Goudey reviewed 2024-03-13 19:40:23 +01:00

Hans Goudey left a comment

I like the way this is headed, but I think it could be a little friendlier/more flexible with a struct for parameters instead. For example, UI_TIP_FLAG_IMAGE_CHECKER and UI_TIP_FLAG_IMAGE_CHECKER_FIXED are mutually exclusive, but that can't be represented by a flag.

What about a struct, something loosely like this?

struct uiTooltipImageParams {
  bool fixed = false;
  bool border = false;
  bool premultiplied = false;
  bool text_color = false;
};

For now it's just booleans I guess, but it could also contain other things, even the size and the image pointer itself I guess.

I like the way this is headed, but I think it could be a little friendlier/more flexible with a struct for parameters instead. For example, `UI_TIP_FLAG_IMAGE_CHECKER` and `UI_TIP_FLAG_IMAGE_CHECKER_FIXED` are mutually exclusive, but that can't be represented by a flag. What about a struct, something loosely like this? ``` struct uiTooltipImageParams { bool fixed = false; bool border = false; bool premultiplied = false; bool text_color = false; }; ``` For now it's just booleans I guess, but it could also contain other things, even the size and the image pointer itself I guess.

Harley Acheson added 1 commit 2024-03-13 21:29:22 +01:00

abf029de0c Let's pass a struct for all these options.

Harley Acheson commented

2024-03-13 21:31:04 +01:00

@HooglyBoogly

What do you think of this? It certainly reads a lot nicer.

@HooglyBoogly What do you think of this? It certainly reads a lot nicer.

Hans Goudey requested changes 2024-03-14 14:36:13 +01:00

Dismissed

Hans Goudey left a comment

Looks great!

source/blender/editors/include/UI_interface_c.hh Outdated

						
				@ -1722,6 +1722,22 @@ enum uiTooltipColorID {

				  UI_TIP_LC_MAX

				};

				enum uiTooltipImageBackground {

Hans Goudey commented

2024-03-14 14:35:00 +01:00

How about this :)

enum class uiTooltipImageBackground {
  None = 0,
  Themed = 1,
  Fixed = 2,
};

How about this :) ``` enum class uiTooltipImageBackground { None = 0, Themed = 1, Fixed = 2, }; ```

Harley marked this conversation as resolved

source/blender/editors/interface/interface_region_tooltip.cc Outdated

						
				@ -92,3 +92,2 @@

				  uiTooltipFormat format;

				  ImBuf *image;

				  short image_size[2];

				  uiTooltipImage *image = nullptr;

Hans Goudey commented

2024-03-14 14:32:45 +01:00

std::optional seems simpler here than allocating a struct. I don't see a matching delete in this PR, so I'd expect a memory leak currently.

`std::optional` seems simpler here than allocating a struct. I don't see a matching `delete` in this PR, so I'd expect a memory leak currently.

Harley marked this conversation as resolved

source/blender/editors/interface/interface_region_tooltip.cc Outdated

						
				@ -125,3 +124,3 @@

				}

				void UI_tooltip_image_field_add(uiTooltipData *data, const ImBuf *image, const short image_size[2])

				void UI_tooltip_image_field_add(uiTooltipData *data, uiTooltipImage image_data)