Alberth wrote:

Some comments:

- bool sell_hovered; ///< A vehicle is being dragged/hovered over the sell button.
+ int current_hovered_widget;

Would be nice to have documentation here as well, in particular for the magic value -1.
Maybe add a constant to denote the special value?

Just "lowered_widget" or "hovered_widget" ? "current" is a bit implied already, and the name gets a bit long.

------------

+ if (widget != this->current_hovered_widget) {
+ if (this->current_hovered_widget == WID_D_SELL || this->current_hovered_widget == WID_D_SELL_CHAIN) {

Any particular reason not to use "!= -1" here?
As you wrote it now, the reader has to deduce that "!= -1" used in the previous hunk is equivalent to these equality tests. When the code gets changed in the future, somewhen this deduction will go wrong. Better be safe, and use the same condition when you mean the same thing.

------------

+ if (widget == WID_D_SELL || widget == WID_D_SELL_CHAIN) {
+ this->SetWidgetLoweredState(widget, true);
+ this->SetWidgetDirty(widget);
+ }
+ this->current_hovered_widget = widget;

I would swap this, first update 'current_hovered_widget', and then use the same condition and code as above with the same variable, making it plain and obviously it's doing the exact same thing with the new value.
You can go one step further, and fold the statements in a new function. Not sure this is useful though.

This comment was imported from FlySpray: https://bugs.openttd.org/task/6391#comment14094

Eearslya wrote:

I fixed your first and third comments, but as for the second comment, that's not what it's doing. -1 was defined as 'no drag-and-drop is in progress', and that's not what that if is checking for. It's checking to see if the currently hovered widget has changed from its last recorded value. If it hasn't changed, there's no reason to proceed into that block.

Attachments

• depress_sell_chain_button_too.diff (2.37 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/6391#comment14095

Alberth closed the ticket.

Reason for closing: Implemented

in r27450

This comment was imported from FlySpray: https://bugs.openttd.org/task/6391