Re: [PATCH v2 02/23] kernel-shark-qt: Add Dual Marker for KernelShark GUI.

[Steven Rostedt <rostedt@goodmis.org>]

On Tue, 16 Oct 2018 15:52:58 +0000
Yordan Karadzhov <ykaradzhov@vmware.com> wrote:

> +
> +/** @brief Create a Dual Marker State Machine. */
> +KsDualMarkerSM::KsDualMarkerSM(QWidget *parent)
> +: QWidget(parent),
> +  _buttonA("Marker A", this),
> +  _buttonB("Marker B", this),
> +  _labelDeltaDescr("    A,B Delta: ", this),
> +  _markA(DualMarkerState::A, Qt::darkGreen),
> +  _markB(DualMarkerState::B, Qt::darkCyan),
> +  _scCtrlA(this),
> +  _scCtrlB(this)
> +{
> +	QString styleSheetA, styleSheetB;
> +
> +	_buttonA.setFixedWidth(STRING_WIDTH(" Marker A "));
> +	_buttonB.setFixedWidth(STRING_WIDTH(" Marker B "));
> +
> +	for (auto const &l: {&_labelMA, &_labelMB, &_labelDelta}) {
> +		l->setFrameStyle(QFrame::Panel | QFrame::Sunken);
> +		l->setStyleSheet("QLabel {background-color : white;}");
> +		l->setTextInteractionFlags(Qt::TextSelectableByMouse);
> +		l->setFixedWidth(FONT_WIDTH * 16);

As a general rule, no "magic numbers". Why 16?

> +	}
> +
> +	styleSheetA = "background : " +
> +		      _markA._color.name() +
> +		      "; color : white";
> +
> +	_stateA = new QState;
> +	_stateA->setObjectName("A");
> +	_stateA->assignProperty(&_buttonA,
> +				"styleSheet",
> +				styleSheetA);
> +
> +	_stateA->assignProperty(&_buttonB,
> +				"styleSheet",
> +				"color : rgb(70, 70, 70)");

Same for the 70s.

> +
> +	styleSheetB = "background : " +
> +		      _markB._color.name() +
> +		      "; color : white";
> +
> +	_stateB = new QState;
> +	_stateB->setObjectName("B");
> +	_stateB->assignProperty(&_buttonA,
> +				"styleSheet",
> +				"color : rgb(70, 70, 70)");

Here too.

> +
> +	_stateB->assignProperty(&_buttonB,
> +				"styleSheet",
> +				styleSheetB);
> +
> +	/* Define transitions from State A to State B. */
> +	_stateA->addTransition(this,	&KsDualMarkerSM::machineToB, _stateB);
> +
> +	_scCtrlA.setKey(Qt::CTRL + Qt::Key_A);
> +	_stateA->addTransition(&_scCtrlB, &QShortcut::activated, _stateB);
> +
> +	connect(&_scCtrlA,	&QShortcut::activated,
> +		this,		&KsDualMarkerSM::_doStateA);
> +
> +	_stateA->addTransition(&_buttonB, &QPushButton::clicked, _stateB);
> +
> +	connect(&_buttonB,	&QPushButton::clicked,
> +		this,		&KsDualMarkerSM::_doStateB);
> +
> +	/* Define transitions from State B to State A. */
> +	_stateB->addTransition(this,	&KsDualMarkerSM::machineToA, _stateA);
> +
> +	_scCtrlB.setKey(Qt::CTRL + Qt::Key_B);
> +	_stateB->addTransition(&_scCtrlA, &QShortcut::activated, _stateA);
> +
> +	connect(&_scCtrlB,	&QShortcut::activated,
> +		this,		&KsDualMarkerSM::_doStateB);
> +
> +	_stateB->addTransition(&_buttonA, &QPushButton::clicked, _stateA);
> +
> +	connect(&_buttonA,	&QPushButton::clicked,
> +		this,		&KsDualMarkerSM::_doStateA);
> +
> +	_machine.addState(_stateA);
> +	_machine.addState(_stateB);
> +	_machine.setInitialState(_stateA);
> +	_markState = DualMarkerState::A;
> +	_machine.start();
> +}
> +



> +/**
> + * @brief Use this function to update the labels when the state of the model
> + *	  has changed.
> + */
> +void KsDualMarkerSM::updateLabels()
> +{
> +	QString mark, delta;
> +
> +	// Marker A
> +	if (_markA._isSet) {
> +		mark = KsUtils::Ts2String(_markA._ts, 7);

Same for these 7s.

> +		_labelMA.setText(mark);
> +	} else {
> +		_labelMA.setText("");
> +	}
> +
> +	// Marker B
> +	if (_markB._isSet) {
> +		mark = KsUtils::Ts2String(_markB._ts, 7);
> +		_labelMB.setText(mark);
> +	} else {
> +		_labelMB.setText("");
> +	}
> +
> +	// Delta
> +	if (_markA._isSet && _markB._isSet) {
> +		delta = KsUtils::Ts2String(_markB._ts - _markA._ts, 7);
> +		_labelDelta.setText(delta);
> +	} else {
> +		_labelDelta.setText("");
> +	}
> +}

Don't resend this patch (I'm going to just take it). Send a patch on
top that just replaces the hard coded numbers with meaningful macros.

Thanks!

-- Steve