UI Code Quality: General, Smaller Changes #74432

New Issue

Julian Eisel · 2020-03-04T14:03:09+01:00

Julian Eisel commented

2020-03-04 14:03:09 +01:00

UI Code Quality: General, Smaller Changes

NOTE: This is a proposal, not an agreed on plan.

Naming conventions

We should avoid ambiguous names. We should also avoid abbreviations, esp. extreme ones, for variables with non-small scope. That should further remove ambiguity and make code more readable.

For example:

	Type	Old naming (convention)	Proposed
{icon check-square-o}	`ARegion`	`ar`	`region`
{icon check-square-o}	`ScrArea`	`sa`	`area`
{icon check-square-o}	`bScreen`	`sc`, `scr`	`screen`
{icon square-o}	`PanelType`	`pt`	`panel_type`
{icon square-o}	`MenuType`	`mt`	`menu_type`
{icon square-o}	`HeaderType`	`ht`	`header_type`
{icon square-o}	`ColorBand`	`coba`	`color_band`
{icon check-square-o}	`ExtensionRNA`	`ext`	`rna_extension`
{icon square-o}	`int`	`x1`, `x2`, `y1`, `y2`	`xmin`, `width`, `ymin`, `height`

Note that all these examples can be changed without touching DNA (AFAICS).

Further:

uiListDyn -> uiList_Runtime

Various

Add iterators for screen data. E.g. iterating all regions (visible or not, including popups and global area regions), iterating all areas (visible or not, including all global areas), iterating all screen regions (as often done by versioning), etc. Could be done in both C (macro) and C++ (range-based for loop )
Introduce wrappers for commonly bundled variables (e.g. PointerRNA & PropertyRNA, wmOperatorType & PointerRNA, ScrArea & ARegion).
Split up bigger, "generic" types. For example uiBut contains data that only applies to certain button-types. uiButTab is an example of how this can be avoided.
Wrap related struct members into nested structs. E.g. rather than uiBut.editstr, uiBut.editval, uiBut.editvec, etc, it could be uiBut.edit_values.foo.
Replace booleans in parameter lists with enums. Having a parameter list like foo(true, false, false) is cumbersome to use (what does each boolean mean) and leads to mistakes (using wrong order of booleans). It's better to be explicit what each value refers to by using enum types. Bitflags are also an option.
Remove old game-engine leftovers from Ghost, e.g. GHOST_FullScreen(), GHOST_EndFullscreen(), embedder windows (GHOST_TEmbedderWindowID + non-dialog child windows)
Split big functions into more manageable chunks.
Add const where applicable
Of course improving naming, comments and documentation is always a good idea!
Use EVT_ prefix for all event types, to complete work started in D7164.

## UI Code Quality: General, Smaller Changes NOTE: This is a proposal, not an agreed on plan. **Naming conventions** We should avoid ambiguous names. We should also avoid abbreviations, esp. extreme ones, for variables with non-small scope. That should further remove ambiguity and make code more readable. For example: | | Type | Old naming (convention) | Proposed | -- | ---- | --------------------- | -------- | {icon check-square-o} | `ARegion` | `ar` | `region` | {icon check-square-o} | `ScrArea` | `sa` | `area` | {icon check-square-o} | `bScreen` | `sc`, `scr` | `screen` | {icon square-o} | `PanelType` | `pt` | `panel_type` | {icon square-o} | `MenuType` | `mt` | `menu_type` | {icon square-o} | `HeaderType` | `ht` | `header_type` | {icon square-o} | `ColorBand` | `coba` | `color_band` | {icon check-square-o} | `ExtensionRNA` | `ext` | `rna_extension` | {icon square-o} | `int` | `x1`, `x2`, `y1`, `y2` | `xmin`, `width`, `ymin`, `height` Note that all these examples can be changed without touching DNA (AFAICS). Further: - [ ] `uiListDyn` -> `uiList_Runtime` **Various** * Add iterators for screen data. E.g. iterating all regions (visible or not, including popups and global area regions), iterating all areas (visible or not, including all global areas), iterating all screen regions (as often done by versioning), etc. Could be done in both C (macro) and C++ ([range-based for loop ](https://en.cppreference.com/w/cpp/language/range-for)) * Introduce wrappers for commonly bundled variables (e.g. `PointerRNA` & `PropertyRNA`, `wmOperatorType` & `PointerRNA`, `ScrArea` & `ARegion`). * Split up bigger, "generic" types. For example `uiBut` contains data that only applies to certain button-types. `uiButTab` is an example of how this can be avoided. * Wrap related struct members into nested structs. E.g. rather than `uiBut.editstr`, `uiBut.editval`, `uiBut.editvec`, etc, it could be `uiBut.edit_values.foo`. * Replace booleans in parameter lists with enums. Having a parameter list like `foo(true, false, false)` is cumbersome to use (what does each boolean mean) and leads to mistakes (using wrong order of booleans). It's better to be explicit what each value refers to by using enum types. Bitflags are also an option. * Remove old game-engine leftovers from Ghost, e.g. `GHOST_FullScreen()`, `GHOST_EndFullscreen()`, embedder windows (`GHOST_TEmbedderWindowID` + non-dialog child windows) * Split big functions into more manageable chunks. * Add `const` where applicable * Of course improving naming, comments and documentation is always a good idea! * Use `EVT_` prefix for all event types, to complete work started in [D7164](https://archive.blender.org/developer/D7164).

Julian Eisel commented

2020-03-04 14:03:09 +01:00

Added subscriber: @JulianEisel

Campbell Barton commented

2020-03-05 02:58:59 +01:00

Added subscriber: @ideasman42

Campbell Barton commented

2020-03-05 02:58:59 +01:00

WM Names: +1 for naming region, area, screen.
Type Names:

Personally I don't find the heavily abbreviated types to be an issue, (`pt`, `mt`, `ht`). In context there is often only one, and it's convenient not to have overly verbose names, especially if you have to differentiate between `panel_type_child`, `panel_type_parent`, `panel_type_next` ... etc.

Consider event handling code that calls operators `op` and their type `ot`.
It's a short terse convention that happens to be convenient, in practice I don't find this confusing and means names such as `ot_prop_basic_count`, don't turn into `operator_type_prop_basic_count`.

ColorBand: +1 to name color_band.
ExtensionRNA: prefer rna_ext since it's a common enough abbreviation (used 200+ times in our code already).
x1, x2, y1, y2:

Prefer not to mix x/y width height, could be `xmin`, `ymin`, `xsize`, `ysize` instead.

Although for UI code we could pass in `rctf` / `rcti` instead.
We could have a utility to create the rectangle from xmin,ymin,xsize,ysize - since rectangles use min/max values.

- **WM Names:** +1 for naming `region`, `area`, `screen`. - **Type Names:** ``` Personally I don't find the heavily abbreviated types to be an issue, (`pt`, `mt`, `ht`). In context there is often only one, and it's convenient not to have overly verbose names, especially if you have to differentiate between `panel_type_child`, `panel_type_parent`, `panel_type_next` ... etc. ``` ``` Consider event handling code that calls operators `op` and their type `ot`. It's a short terse convention that happens to be convenient, in practice I don't find this confusing and means names such as `ot_prop_basic_count`, don't turn into `operator_type_prop_basic_count`. ``` - **ColorBand:** +1 to name `color_band`. - **ExtensionRNA:** prefer `rna_ext` since it's a common enough abbreviation (used 200+ times in our code already). - **x1, x2, y1, y2**: ``` Prefer not to mix x/y width height, could be `xmin`, `ymin`, `xsize`, `ysize` instead. ``` ``` Although for UI code we could pass in `rctf` / `rcti` instead. We could have a utility to create the rectangle from xmin,ymin,xsize,ysize - since rectangles use min/max values.

Harley Acheson commented

2020-03-16 23:39:31 +01:00

Added subscriber: @Harley

Harley Acheson commented

2020-03-16 23:39:31 +01:00

I would love to see refactor and standardization of our use of words like "Active" and "Selected" in the UI code. We (strangely) use "active" for the state of having the mouse hovering over. Sometimes "Selected" to simulate having focus and therefore activated by hitting "Enter". Other times "Selected" is when it is being actively acted upon. Elsewhere we use Active and Selected in a large variety of different ways. Would love for us to define how we use them in way that is similar to html pseudoclasses:

Hover - when something is being passed over by some input device, like mouse cursor moving over it.
Focus - when an item is selected for, or ready for, keyboard entry. Moved with Tab. A button with focus is executed with Enter.
Default - A preselected item, even if it does not currently have focus.
Active - when something is currently activated by the user, like when a button is being depressed
Selected - Chosen by the user
Disabled - not currently usable or applicable

I realize that the above would not eliminate the overlap with our use of Active verses Selected objects. But even in those I wish we were always just operating on a list of Selected objects, the first of which is what we now consider "Active". That could help in possibly (eventually) getting to a future where we could pass any list of objects to operators without any relation to what is currently selected by the user.

I would love to see refactor and standardization of our use of words like "Active" and "Selected" in the UI code. We (strangely) use "active" for the state of having the mouse hovering over. Sometimes "Selected" to simulate having focus and therefore activated by hitting "Enter". Other times "Selected" is when it is being actively acted upon. Elsewhere we use Active and Selected in a large variety of different ways. Would love for us to define how we use them in way that is similar to html pseudoclasses: Hover - when something is being passed over by some input device, like mouse cursor moving over it. Focus - when an item is selected for, or ready for, keyboard entry. Moved with Tab. A button with focus is executed with Enter. Default - A preselected item, even if it does not currently have focus. Active - when something is currently activated by the user, like when a button is being depressed Selected - Chosen by the user Disabled - not currently usable or applicable I realize that the above would not eliminate the overlap with our use of Active verses Selected *objects*. But even in those I wish we were always just operating on a list of Selected objects, the first of which is what we now consider "Active". That could help in possibly (eventually) getting to a future where we *could* pass any list of objects to operators without any relation to what is currently selected by the user.

Philipp Oeser removed the

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

UI Code Quality: General, Smaller Changes #74432

UI Code Quality: General, Smaller Changes