[FEAT] Define the options that will be used to configure the Renderer

[tbouffard]

We have several issues that requires to pass new options to the renderer.
To have something consistent, I suggest we review all requirements to propose a consistent Options definition.

Topics involved:

• [FEAT] Review Black Box Pool Rendering #718: finally not planned
• [FEAT] Provide a way to configure the defaults of the BPMN renderer #1993
• [FEAT] Provide an option to ignore BPMN LabelStyles #2468
• [FEAT] Provide an option to ignore BPMN Label Bounds of activities #2469
• [FEAT] General improvements for default styles management #2471 - not directly involved as it suggests to create a dedicated API
• feat: add support for 'BPMN in Color' #2614 - ignore colors defined in the BPMN diagram

Way of working

• starting from the initial proposal, let's discuss and comment this issue to amend the proposal
• keep track of the initial proposal to be able to see at a glance the diff between the current/final solution and the initial proposition
• during the design phase, the proposal will be updated until we converge

Final proposal

renderer: {
    /**
     * For #2469. We may need a `ignoreBpmnTaskLabelBounds` properties as well.
     * @default false
     */
    ignoreBpmnActivityLabelBounds?: boolean;
    /**
     * For #2614.
     * @default true
     */
    ignoreBpmnColors?: boolean;
    /**
     * For #2468.
     * @default false
     */
    ignoreBpmnLabelStyles?: boolean;
    /**
     * For #1993. Overlays configuration only.
     */
    overlayDefaults?: OverlayStyle;
};

Initial proposal

#718: I suggest to NOT make it an option. Decide whether to implement it or keep it as it is today. We could introduce the option later if someone request it and motivate their needs. Today I don't see the benefits of such an option.

// renderer is a new property in GlobalOptions
    renderer: {
        // in comments, the related name of entries currently in StyleDefault
        defaults: {
            // "general properties" - Do we introduce a "general" for the following properties?
            // TODO improve name, used in a lot of places
            margin?: number; //   DEFAULT_MARGIN = 0,
            // try to share with the Fill type used in the "Update Style" API
            fill?: {
                color?: string; //   DEFAULT_FILL_COLOR = 'White',
                opacity?: number;
            };
            // try to share with the Font type used in the "Update Style" API
            // I suggest we don't let set isBold, isXXX
            font?: {
                color?: string; //   DEFAULT_FONT_COLOR = 'Black',
                family?: string; //   DEFAULT_FONT_FAMILY = 'Arial, Helvetica, sans-serif', // define our own to not depend on eventual mxGraph default change
                opacity?: number;
                size?: number; //   DEFAULT_FONT_SIZE = 11,
            };
            // try to share with the Stroke type used in the "Update Style" API
            stroke: {
                color: string; //   DEFAULT_STROKE_COLOR = 'Black',
                opacity: number;
                width?: number;
            };
            // END of "general properties"
            // specific to BPMN shapes and edges
            bpmn: {
                activity: {
                    marginBottom?: number; //   SHAPE_ACTIVITY_BOTTOM_MARGIN = 7
                    marginFromCenter?: number;//   SHAPE_ACTIVITY_FROM_CENTER_MARGIN = 7, --> find a better name
                    marginLeft?: number;//   SHAPE_ACTIVITY_LEFT_MARGIN = 7,
                    marginTop?: number;//   SHAPE_ACTIVITY_TOP_MARGIN = 7,
                    markerIconMargin?: number;//   SHAPE_ACTIVITY_MARKER_ICON_MARGIN = 5,
                    markerIconSize?: number;//   SHAPE_ACTIVITY_MARKER_ICON_SIZE = 20,
                }

                groupFillColor?: string; //   GROUP_FILL_COLOR = 'none',
                laneLabelFillColor?: string; //   LANE_LABEL_FILL_COLOR = 'none',
                laneLabelSize?: number; //   LANE_LABEL_SIZE = 30, // most of BPMN lane are ok when setting it to 30
                poolLabelFillColor?: string; //   POOL_LABEL_FILL_COLOR = 'none',
                poolLabelSize?: number; //   POOL_LABEL_SIZE = 30, // most of BPMN pool are ok when setting it to 30
                subProcessTransactionInnerRectangleArcSize?: number; //   SUB_PROCESS_TRANSACTION_INNER_RECT_ARC_SIZE = 6,
                subProcessTransactionInnerRectangleOffset?: number; //   SUB_PROCESS_TRANSACTION_INNER_RECT_OFFSET = 4,
                textAnnotationBorderLength?: number; //   TEXT_ANNOTATION_BORDER_LENGTH = 10,
                textAnnotationFillColor?: string; //   TEXT_ANNOTATION_FILL_COLOR = 'none',
                // Do we create an intermediate property for "shapes"?
                shapes?: {
                    arcSize?: number; //   SHAPE_ARC_SIZE = 20,
                    strokeWidthThick?: number; //   STROKE_WIDTH_THICK = 5,
                    strokeWidthThin?: number; //   STROKE_WIDTH_THIN = 2,
                };
                // Do we create an intermediate property for "edges"?
                edges?: {
                    arcSize?: number; // currently, not configurable
                    messageFlowMarkerEndFillColor?: string; //   MESSAGE_FLOW_MARKER_END_FILL_COLOR = 'White',
                    messageFlowMarkerStartFillColor?: string; //   MESSAGE_FLOW_MARKER_START_FILL_COLOR = 'White',
                    sequenceFlowConditionalMarkerFillColor?: string; //   SEQUENCE_FLOW_CONDITIONAL_FROM_ACTIVITY_MARKER_FILL_COLOR = 'White',
                };
            };
            overlays: {
                // use fill, stroke and font structure as everywhere else?
                fillColor?: string; //   DEFAULT_OVERLAY_FILL_COLOR = DEFAULT_FILL_COLOR,
                fillOpacity?: string; //   DEFAULT_OVERLAY_FILL_OPACITY = 100,
                fontColor?: string; //   DEFAULT_OVERLAY_FONT_COLOR = DEFAULT_FONT_COLOR,
                fontSize?: number; //   DEFAULT_OVERLAY_FONT_SIZE = DEFAULT_FONT_SIZE,
                strokeColor?: string; //   DEFAULT_OVERLAY_STROKE_COLOR = DEFAULT_STROKE_COLOR,
                strokeWidth?: number; //   DEFAULT_OVERLAY_STROKE_WIDTH = 1,
            }
        };
        /**
         * For #2469. We may need a `ignoreActivitiesLabelBounds` properties as well.
         * @default false
         */
        ignoreBpmnActivityLabelBounds?: boolean;
        /**
         * For #2614.
         * @default false
         */
        ignoreBpmnColors?: boolean;
        /**
         * For #2468.
         * @default false
         */
        ignoreBpmnLabelStyles?: boolean;
    };

The most urgent decision must be taken to finalize the implementation of #2614

Guidelines

• use plural for properties
• for all properties related to ignoring data from the BPMN source, start with ignoreBpmn

Questions

• for the defaults property
  • the fill, font, stroke types may be shared with the types used in the "Update Style" API. However, this involves accepting the "default" value. What would that means here? the default value provided by the lib? This is the same issue as for [REFACTOR] Mutualize the programatic styling APIs of Shapes/Edges with that of Overlays #2457. We didn't see benefits for overlays, to be investigated for the default renderer options.
  • do we separate "general"/"shared" properties from the properties specific to dedicated BPMN elements?
  • do we really the "opacity" properties?

We may introduce a bpmnIgnores that holds all "ignored" properties. This seems overkill for 3 properties

// new property in GlobalOptions
  renderer: {
    bpmnIgnores: {
      activityLabelBounds
      colors: ....
      labelStyles: ....
    }
  }

past proposed names for #718

• forceHorizontalBlackBoxPool

past proposed names for #2614

• ignoreBpmnColors - singular or plural?
• ignoreModelColors
• ignoreBpmnInColor - name of the specification
• ignoreBpmnModelColor(s) - do we really need to mention BPMN here as we manipulate BPMN model only?
  Le

[akantcheff]

I don't understand why there is a "sub" attribute defaults that will contain all those new options? Why those options are not directly under render?
I mean that if I want to set a new value for an option, it is no more a default value :p

[jeromecambon]

This is a good idea to have a way to define the styles independent of the underlying techno (mxGraph) .
Is your initial proposal an exhaustive set of properties?
I think we should make sure that it covers all the properties supported by mxGraph styleSheet.
We used the current "hack" to define the style of our diagrams. While this is not very user friendly, we managed to use it without too much difficulties.
For information we used the following styling:

       configureStyle() {
            const styleSheet = this.graph.getStylesheet(); // mxStylesheet

            const startEventStyle = styleSheet.styles[bpmnvisu.ShapeBpmnElementKind.EVENT_START];
            startEventStyle[StyleIdentifiers.STYLE_GRADIENT_DIRECTION] = Directions.DIRECTION_WEST;
            startEventStyle[StyleIdentifiers.STYLE_GRADIENTCOLOR] = '#FBFBEE';
            startEventStyle[StyleIdentifiers.STYLE_FILLCOLOR] = '#E9ECB1';
            startEventStyle[StyleIdentifiers.STYLE_STROKECOLOR] = '#62A928';

            [bpmnvisu.ShapeBpmnElementKind.EVENT_INTERMEDIATE_CATCH, bpmnvisu.ShapeBpmnElementKind.EVENT_INTERMEDIATE_THROW].forEach(kind => {
                const intermediateEventStyle = styleSheet.styles[kind];
                intermediateEventStyle[StyleIdentifiers.STYLE_STROKECOLOR] = '#2E6DA3';
                intermediateEventStyle[StyleIdentifiers.STYLE_FILLCOLOR] = '#ffffff'; // ensure reset if the style is redefined before
            })

            const boundaryEventStyle = styleSheet.styles[bpmnvisu.ShapeBpmnElementKind.EVENT_BOUNDARY];
            boundaryEventStyle[StyleIdentifiers.STYLE_FILLCOLOR] = '#ffffff';
            boundaryEventStyle[StyleIdentifiers.STYLE_STROKECOLOR] = '#2E6DA3';

            const endEventStyle = styleSheet.styles[bpmnvisu.ShapeBpmnElementKind.EVENT_END];
            endEventStyle[StyleIdentifiers.STYLE_GRADIENT_DIRECTION] = Directions.DIRECTION_WEST;
            endEventStyle[StyleIdentifiers.STYLE_GRADIENTCOLOR] = '#FFFFFF';
            endEventStyle[StyleIdentifiers.STYLE_FILLCOLOR] = '#F9D0C6';
            endEventStyle[StyleIdentifiers.STYLE_STROKECOLOR] = '#89151A';

            bpmnvisu.ShapeUtil.taskKinds().forEach(kind => {
                const style = styleSheet.styles[kind];
                style[StyleIdentifiers.STYLE_GRADIENT_DIRECTION] = Directions.DIRECTION_WEST;
                style[StyleIdentifiers.STYLE_GRADIENTCOLOR] = '#FBFBEE';
                style[StyleIdentifiers.STYLE_FILLCOLOR] = '#E5E6F1';
                style[StyleIdentifiers.STYLE_STROKECOLOR] = '#2C6DA3';
            });

            bpmnvisu.ShapeUtil.gatewayKinds().forEach(kind => {
                const style = styleSheet.styles[kind];
                style[StyleIdentifiers.STYLE_GRADIENT_DIRECTION] = Directions.DIRECTION_WEST;
                style[StyleIdentifiers.STYLE_GRADIENTCOLOR] = '#FBFBEE';
                style[StyleIdentifiers.STYLE_FILLCOLOR] = '#E9ECB1';
                style[StyleIdentifiers.STYLE_STROKECOLOR] = '#96A826';
            });
         }
    }

Apart from that, I agree with the the comment above, the "defaults" property does not seem useful.

[abirembaut]

if fill, font and stroke are groups with subproperties in general properties, I feel it should be the same in bpmn and overlays.
If it is not the case, then consider also using fillColor, fillOpacity, etc in general prop to avoid having different ways of setting properties.
Both approaches are ok for me as long as it is consistent.
Other than that, prop names are clear to me

[tbouffard]

Thank you all for your comments. 😍 We will take them into account.

The currently named defaults property is to support #1993.
@akantcheff We'll be looking for a better name to make things more explicit.
@jeromecambon As explained in #1993, defaults won't be used for what you describe. It is defined for shared style properties, not to change the default style of specific BPMN types. #2471 will handle it instead.

@abirembaut yes we must be consistent across properties: flat properties or structured objects (this is what we use in existing API).

[csouchet]

Some propositions

GlobalOptions

interface GlobalOptions {
  // existing properties...

	renderer?: RendererOptions;
}

RendererOptions

interface RendererOptions {
	default?: DefaultOptions;
	bpmnIgnore?: BpmnIgnoreOptions;
}

DefaultOptions

interface DefaultOptions {
	general?: GeneralOptions;
	bpmn?: BpmnOptions;
	overlay?: OverlayOptions;
}

GeneralOptions

interface GeneralOptions {
	margin?: number; // DEFAULT_MARGIN = 0
	fill?: Fill;
	font?: Font;
	stroke?: Stroke;
}

BpmnOptions

interface BpmnOptions {
	activity?: ActivityOptions;
	group?: {
		fill?: Pick<Fill, 'color'>; // GROUP_FILL_COLOR = 'none'
	};
	lane?: {
		label?: LabelOptions; // LANE_LABEL_FILL_COLOR = 'none'; LANE_LABEL_SIZE = 30
	};
	pool?: {
		label?: LabelOptions; // POOL_LABEL_FILL_COLOR = 'none'; POOL_LABEL_SIZE = 30
	};
	subProcess?: SubProcessOptions;
	textAnnotation?: TextAnnotationOptions;
	messageFlow?: MessageFlowOptions;
	sequenceFlow?: SequenceFlowOptions;

	shape?: {
		arcSize?: number; // SHAPE_ARC_SIZE = 20
		strokeWidth?: {
			thick?: number; // STROKE_WIDTH_THICK = 5
			thin?: number; // STROKE_WIDTH_THIN = 2
		};
	};
	edge?: { // Or flow?: { 
		arcSize?: number; // currently not configurable
	};
}

BpmnIgnoreOptions

interface BpmnIgnoreOptions {
	activityLabelBounds?: boolean; // For #2469, default: false
	colors?: boolean; // For #2614, default: false
	labelStyles?: boolean; // For #2468, default: false
}

ActivityOptions

interface ActivityOptions {
	margin?: {
		bottom?: number; // SHAPE_ACTIVITY_BOTTOM_MARGIN = 7
		top?: number; // SHAPE_ACTIVITY_TOP_MARGIN = 7
		left?: number; // SHAPE_ACTIVITY_LEFT_MARGIN = 7
		fromCenter?: number; // SHAPE_ACTIVITY_FROM_CENTER_MARGIN = 7
	};
	markerIcon?: {
		margin?: number; // SHAPE_ACTIVITY_MARKER_ICON_MARGIN = 5
		size?: number; // SHAPE_ACTIVITY_MARKER_ICON_SIZE = 20
	};
}

SubProcessOptions

interface SubProcessOptions {
	transaction?: {
		innerRectangle?: {
			arcSize?: number; // SUB_PROCESS_TRANSACTION_INNER_RECT_ARC_SIZE = 6
			offset?: number; // SUB_PROCESS_TRANSACTION_INNER_RECT_OFFSET = 4
		};
	};
}

TextAnnotationOptions

interface TextAnnotationOptions {
	borderLength?: number; // TEXT_ANNOTATION_BORDER_LENGTH = 10
	fill?: Pick<Fill, 'color'>; // TEXT_ANNOTATION_FILL_COLOR = 'none'
}

MessageFlowOptions

interface MessageFlowOptions {
	fillColor?: {
		endMarker?: string; // MESSAGE_FLOW_MARKER_END_FILL_COLOR = 'White'
		startMarker?: string; // MESSAGE_FLOW_MARKER_START_FILL_COLOR = 'White'
	};
}

SequenceFlowOptions

interface SequenceFlowOptions {
	fillColor?: {
		conditionalMarker?: string; // SEQUENCE_FLOW_CONDITIONAL_FROM_ACTIVITY_MARKER_FILL_COLOR = 'White'
	};
}

LabelOptions

interface LabelOptions {
	fill?: Pick<Fill, 'color'>; 
	size?: number; 
}

OverlayOptions

interface OverlayOptions {
	fill?: Fill;  // DEFAULT_OVERLAY_FILL_COLOR = DEFAULT_FILL_COLOR; DEFAULT_OVERLAY_FILL_OPACITY = 100
	font?: Pick<Font, 'color' | 'size'>; // DEFAULT_OVERLAY_FONT_COLOR = DEFAULT_FONT_COLOR; DEFAULT_OVERLAY_FONT_SIZE = DEFAULT_FONT_SIZE
	stroke?: Pick<Stroke, 'color' | 'width'>;  // DEFAULT_OVERLAY_STROKE_COLOR = DEFAULT_STROKE_COLOR; DEFAULT_OVERLAY_STROKE_WIDTH = 1
}

[tbouffard]

@csouchet I have seen what you propose to split the type/interface into multiple type/interface.

For the time being, we won't be reusing them. So, for simplicity's sake, I'm proposing a single type/interface. This will produce a better result in the API's HTML documentation and make things easier to understand.

While the usage of Pick, Font, Stroke, ... types may be finally implemented to remove duplication, this makes things harder to understand here. Remember that we cannot reuse the existing types of the Overlay and the "Update Style" APIs (see #2457)

We should also review if we use flat properties vs object as this makes the configuration more verbose.

I am not convinced about the need for a BpmnIgnoreOptions type: it would requires a configuration migration (as ignoreBpmnColors already exists #2614) and as for other properties, it would introduce type complexity.

[csouchet]

A POC of the implementation from my last proposition: #2761

Maybe, we should consider to not implement the updating of our default internal value at the initialization of the library, and focus on a new API for the theme.

[tbouffard]

Decision taken with @csouchet on 2023-07-11

We realized that most of the needs for the renderer configuration come from #1993. This is because we opened the StyleDefault constant in the early days of bpmn-visualization.
Updating a value in StyleDefault lets easily change the style. But StyleDefault is global, so it impacts all instances of BpmnVisualization. In the examples, we had to develop some utility functions to reset the initial values,

Having a default configuration when initializing the library will not solve this problem. It would require adding a mechanism to remove the global state, given that there is no extension point in mxGraph shapes to access this default configuration without using a global state.

Updating default values is not a common use case and there are other ways of modifying them, for example by changing default styles.
These new options would be simpler to use than redefining global styles. However, we don't think that the need for simplification brings enough value (infrequent use case) compared to the effort required for development (including the elimination of the use of a global state).

So, we decide that we won't let user change the default style values at library initialization. Instead, we will document how this can be done by overriding the global BPMN (mxGraph) styles (we already have example involving such styles).
Some default values can no longer be modified, as they are currently not configurable with a style (for example, SHAPE_ACTIVITY_BOTTOM_MARGIN or TEXT_ANNOTATION_BORDER_LENGTH). In the future, we may be able to make them configurable if someone requests it. These are very marginal use cases, so the impact of this change should concern a very small number of users, if any at all.

We will only let configure the defaults of the style of the overlays:

• they are not managed by mxGraph, so they are not handled by mxGraph global styles and users cannot configure them in another way
• we can then merge the user defaults with the lib defaults in a clean way

Here is the new proposal for the properties of the GlobalOptions.renderer properties
Note: the name of the properties is subject to change

renderer: {
    /**
     * For #2469. We may need a `ignoreBpmnTaskLabelBounds` properties as well.
     * @default false
     */
    ignoreBpmnActivityLabelBounds?: boolean;
    /**
     * For #2614.
     * @default false
     */
    ignoreBpmnColors?: boolean;
    /**
     * For #2468.
     * @default false
     */
    ignoreBpmnLabelStyles?: boolean;
    /**
     * For #1993. Overlays configuration only.
     */
    overlayDefaults?: OverlayStyle;
};

[csouchet]

@tbouffard The name of the ignore properties are fine for me 🙂

For the overlays, I propose simply overlay.

[tbouffard]

@csouchet thanks for your feedback.
About the overlay property, if we omit the defaults suffix, we loose an important information IMHO. It doesn't mention it is to configure the defaults of the overlay rendering.

[tbouffard]

All decisions have been taken and the final proposal has been documented.
So closing.