Refactor: U.dpi_fac -> U.scale_factor #105750

Harley Acheson · 2023-03-14T03:39:22+01:00

Harley Acheson commented

2023-03-14 03:39:22 +01:00

A renaming of UI scale factors from names that imply a relationship to
monitor DPI to names that imply that they simply change "scale"

This is a disruptive change request that should be considered carefully. When posted on Phab Brecht approved it, but I still worry that this is a big change just because "Harley doesn't like a word".

With the last release we changed U.dpi so that it contains actual monitor DPI and is no longer scaled by line width. This might be a time to consider changing other UI variable names to be less confusing as well. The term "dpi_fac" has always seemed a bit odd to me as its not really related to DPI and "fac" is a clumsy shortening of factor. My hope is the following makes it all a bit less confusing for new developers.

This PR first renames the following:

U.dpi_fac 🡒 U.scale_factor
U.inv_dpi_fac 🡒 U.inv_scale_factor
UI_DPI_FAC 🡒 UI_SCALE_FAC
UI_DPI_ICON_SIZE 🡒 UI_ICON_SIZE

Then ALL current uses of U.scale_factor are changed to instead use the macro UI_SCALE_FAC, making it easier to move or change this later. And all uses of U.inv_scale_factor use macro UI_INV_SCALE_FAC. Our current code contains a mixture of U.dpi_fac and UI_DPI_FAC, often in the same file.

A review item that was previously "Not Done" is completed here. Campbell wanted a doc-string on UI_SCALE_FAC and it is now in DNA_userdef_types.h as "Scaling factor for all UI elements, based on the "Resolution Scale" user preference and the DPI/OS Scale of each monitor. This is a read-only, run-time value calculated by WM_window_set_dpi at various times, including between the drawing of each window and so can vary between monitors."

A renaming of UI scale factors from names that imply a relationship to monitor DPI to names that imply that they simply change "scale" --- This is a disruptive change request that should be considered carefully. When posted [on Phab](https://archive.blender.org/developer/D16807) Brecht approved it, but I still worry that this is a big change just because "Harley doesn't like a word". With the last release we changed U.dpi so that it contains actual monitor DPI and is no longer scaled by line width. This might be a time to *consider* changing other UI variable names to be less confusing as well. The term "dpi_fac" has always seemed a bit odd to me as its not *really* related to DPI and "fac" is a clumsy shortening of factor. My hope is the following makes it all a bit less confusing for new developers. This PR first renames the following: * U.dpi_fac 🡒 U.scale_factor * U.inv_dpi_fac 🡒 U.inv_scale_factor * UI_DPI_FAC 🡒 UI_SCALE_FAC * UI_DPI_ICON_SIZE 🡒 UI_ICON_SIZE Then **ALL** current uses of U.scale_factor are changed to instead use the macro UI_SCALE_FAC, making it easier to move or change this later. And all uses of U.inv_scale_factor use macro UI_INV_SCALE_FAC. Our current code contains a mixture of U.dpi_fac and UI_DPI_FAC, often in the same file. A review item that was previously "Not Done" is completed here. Campbell wanted a doc-string on UI_SCALE_FAC and it is now in DNA_userdef_types.h as "Scaling factor for all UI elements, based on the "Resolution Scale" user preference and the DPI/OS Scale of each monitor. This is a read-only, run-time value calculated by `WM_window_set_dpi` at various times, including between the drawing of each window and so can vary between monitors."

👍 1

Harley Acheson changed title from ~~Refactor: .dpi_fac -> U.scale_factor~~ to Refactor: U.dpi_fac -> U.scale_factor

2023-03-14 03:42:59 +01:00

Brecht Van Lommel requested changes 2023-03-14 16:36:02 +01:00

source/blender/makesdna/DNA_userdef_types.h Outdated

						
				@ -18,0 +21,4 @@

				 * `WM_window_set_dpi` at various times, including between the drawing of each window and so can

				 * vary between monitors.

				 */

				#define UI_SCALE_FAC ((void)0, U.scale_factor)

Brecht Van Lommel commented

2023-03-14 16:35:36 +01:00

Why is (void)0 used here, I don't think it's necessary?

Why is `(void)0` used here, I don't think it's necessary?

brecht marked this conversation as resolved

Germano Cavalcante commented

2023-03-14 16:58:47 +01:00

Since dpi_fac does not represent nor DPI, it seems convenient to rename it to scale_factor IMHO.

Perhaps we don't even need the _factor suffix.

However, I noticed that you already have a member with a similar name. The U.ui_scale. Not sure if it's something to be concerned about.

Since `dpi_fac` does not represent nor DPI, it seems convenient to rename it to `scale_factor` IMHO. Perhaps we don't even need the `_factor` suffix. However, I noticed that you already have a member with a similar name. The `U.ui_scale`. Not sure if it's something to be concerned about.

Harley Acheson commented

2023-03-14 22:06:43 +01:00

@blender-bot build

Harley Acheson commented

2023-03-15 17:36:27 +01:00

@brecht - Why is (void)0 used here, I don't think it's necessary?

I did that based on Campbell's suggestion that this would "avoid accidental assignment", unless I am misunderstanding. https://archive.blender.org/developer/differential/0016/0016807/#461990

> @brecht - Why is (void)0 used here, I don't think it's necessary? I did that based on Campbell's suggestion that this would "avoid accidental assignment", unless I am misunderstanding. https://archive.blender.org/developer/differential/0016/0016807/#461990

Brecht Van Lommel approved these changes 2023-03-15 17:45:28 +01:00

Brecht Van Lommel left a comment

Ok, that's fine then.

Harley Acheson force-pushed DpiFac from 30fb5ea3d7 to 5e6ec529c4

2023-03-16 22:52:39 +01:00

Compare

Harley Acheson added 1 commit 2023-03-17 03:06:56 +01:00