Fix #11655: Crash due to NWidgetMatrix modifying widget->index. (#11657)

NWidgetMatrix modifies its child widget's index to indicate which element
is to be drawn, which now causes issues with code that does not know about
stuffing extra data into the index.

Instead, let NWidgetMatrix store the currently processing element, and
retrieve this information from the matrix widget while child widgets are
being drawn.

This means only widgets that are children of NWidgetMatrix need to know
anything about their extra data.
wip-string
Peter Nelson 5 months ago committed by GitHub
parent 1e60734660
commit 6215e9bf77
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23

@ -306,7 +306,7 @@ public:
void DrawWidget(const Rect &r, WidgetID widget) const override
{
switch (GB(widget, 0, 16)) {
switch (widget) {
case WID_BO_CLASS_LIST: {
Rect mr = r.Shrink(WidgetDimensions::scaled.matrix);
uint pos = 0;
@ -333,7 +333,8 @@ public:
* look nice in all layouts, we use the 4x4 layout (smallest previews) as starting point. For the bigger
* previews in the layouts with less views we add space homogeneously on all sides, so the 4x4 preview-rectangle
* is centered in the 2x1, 1x2 resp. 1x1 buttons. */
uint matrix_height = this->GetWidget<NWidgetMatrix>(WID_BO_OBJECT_MATRIX)->current_y;
const NWidgetMatrix *matrix = this->GetWidget<NWidgetMatrix>(WID_BO_OBJECT_MATRIX);
uint matrix_height = matrix->current_y;
DrawPixelInfo tmp_dpi;
/* Set up a clipping area for the preview. */
@ -345,7 +346,7 @@ public:
const DrawTileSprites *dts = &_objects[spec->grf_prop.local_id];
DrawOrigTileSeqInGUI(ir.Width() / 2 - 1, (ir.Height() + matrix_height / 2) / 2 - this->object_margin - ScaleSpriteTrad(TILE_PIXELS), dts, PAL_NONE);
} else {
DrawNewObjectTileInGUI(ir.Width() / 2 - 1, (ir.Height() + matrix_height / 2) / 2 - this->object_margin - ScaleSpriteTrad(TILE_PIXELS), spec, GB(widget, 16, 16));
DrawNewObjectTileInGUI(ir.Width() / 2 - 1, (ir.Height() + matrix_height / 2) / 2 - this->object_margin - ScaleSpriteTrad(TILE_PIXELS), spec, matrix->GetCurrentElement());
}
}
break;
@ -353,7 +354,7 @@ public:
case WID_BO_SELECT_IMAGE: {
ObjectClass *objclass = ObjectClass::Get(_selected_object_class);
int obj_index = objclass->GetIndexFromUI(GB(widget, 16, 16));
int obj_index = objclass->GetIndexFromUI(this->GetWidget<NWidgetMatrix>(WID_BO_SELECT_MATRIX)->GetCurrentElement());
if (obj_index < 0) break;
const ObjectSpec *spec = objclass->GetSpec(obj_index);
if (spec == nullptr) break;
@ -496,7 +497,7 @@ public:
void OnClick([[maybe_unused]] Point pt, WidgetID widget, [[maybe_unused]] int click_count) override
{
switch (GB(widget, 0, 16)) {
switch (widget) {
case WID_BO_CLASS_LIST: {
auto it = this->vscroll->GetScrolledItemFromWidget(this->object_classes, pt.y, this, widget);
if (it == this->object_classes.end()) break;
@ -508,14 +509,14 @@ public:
case WID_BO_SELECT_IMAGE: {
ObjectClass *objclass = ObjectClass::Get(_selected_object_class);
int num_clicked = objclass->GetIndexFromUI(GB(widget, 16, 16));
int num_clicked = objclass->GetIndexFromUI(this->GetWidget<NWidgetMatrix>(WID_BO_SELECT_MATRIX)->GetCurrentElement());
if (num_clicked >= 0 && objclass->GetSpec(num_clicked)->IsAvailable()) this->SelectOtherObject(num_clicked);
break;
}
case WID_BO_OBJECT_SPRITE:
if (_selected_object_index != -1) {
_selected_object_view = GB(widget, 16, 16);
_selected_object_view = this->GetWidget<NWidgetMatrix>(WID_BO_OBJECT_MATRIX)->GetCurrentElement();
this->SelectOtherObject(_selected_object_index); // Re-select the object for a different view.
}
break;

@ -1240,7 +1240,7 @@ public:
{
DrawPixelInfo tmp_dpi;
switch (GB(widget, 0, 16)) {
switch (widget) {
case WID_BRAS_PLATFORM_DIR_X: {
/* Set up a clipping area for the '/' station preview */
Rect ir = r.Shrink(WidgetDimensions::scaled.bevel);
@ -1285,7 +1285,7 @@ public:
}
case WID_BRAS_IMAGE: {
uint16_t type = GB(widget, 16, 16);
uint16_t type = this->GetWidget<NWidgetMatrix>(WID_BRAS_MATRIX)->GetCurrentElement();
assert(type < _railstation.station_count);
/* Check station availability callback */
const StationSpec *statspec = StationClass::Get(_railstation.station_class)->GetSpec(type);
@ -1325,7 +1325,7 @@ public:
void OnClick([[maybe_unused]] Point pt, WidgetID widget, [[maybe_unused]] int click_count) override
{
switch (GB(widget, 0, 16)) {
switch (widget) {
case WID_BRAS_PLATFORM_DIR_X:
case WID_BRAS_PLATFORM_DIR_Y:
this->RaiseWidget(_railstation.orientation + WID_BRAS_PLATFORM_DIR_X);
@ -1470,7 +1470,7 @@ public:
}
case WID_BRAS_IMAGE: {
uint16_t y = GB(widget, 16, 16);
uint16_t y = this->GetWidget<NWidgetMatrix>(WID_BRAS_MATRIX)->GetCurrentElement();
if (y >= _railstation.station_count) return;
/* Check station availability callback */
@ -2095,9 +2095,9 @@ struct BuildRailWaypointWindow : PickerWindowBase {
void DrawWidget(const Rect &r, WidgetID widget) const override
{
switch (GB(widget, 0, 16)) {
switch (widget) {
case WID_BRW_WAYPOINT: {
uint16_t type = this->list.at(GB(widget, 16, 16));
uint16_t type = this->list.at(this->GetWidget<NWidgetMatrix>(WID_BRW_WAYPOINT_MATRIX)->GetCurrentElement());
const StationSpec *statspec = this->waypoints->GetSpec(type);
DrawPixelInfo tmp_dpi;
@ -2118,9 +2118,9 @@ struct BuildRailWaypointWindow : PickerWindowBase {
void OnClick([[maybe_unused]] Point pt, WidgetID widget, [[maybe_unused]] int click_count) override
{
switch (GB(widget, 0, 16)) {
switch (widget) {
case WID_BRW_WAYPOINT: {
uint16_t sel = GB(widget, 16, 16);
uint16_t sel = this->GetWidget<NWidgetMatrix>(WID_BRW_WAYPOINT_MATRIX)->GetCurrentElement();
assert(sel < this->list.size());
uint16_t type = this->list.at(sel);

@ -1409,7 +1409,7 @@ public:
void DrawWidget(const Rect &r, WidgetID widget) const override
{
switch (GB(widget, 0, 16)) {
switch (widget) {
case WID_BROS_STATION_NE:
case WID_BROS_STATION_SE:
case WID_BROS_STATION_SW:
@ -1451,7 +1451,7 @@ public:
}
case WID_BROS_IMAGE: {
uint16_t type = GB(widget, 16, 16);
uint16_t type = this->GetWidget<NWidgetMatrix>(WID_BROS_MATRIX)->GetCurrentElement();
assert(type < _roadstop_gui_settings.roadstop_count);
const RoadStopSpec *spec = RoadStopClass::Get(_roadstop_gui_settings.roadstop_class)->GetSpec(type);
@ -1497,7 +1497,7 @@ public:
void OnClick([[maybe_unused]] Point pt, WidgetID widget, [[maybe_unused]] int click_count) override
{
switch (GB(widget, 0, 16)) {
switch (widget) {
case WID_BROS_STATION_NE:
case WID_BROS_STATION_SE:
case WID_BROS_STATION_SW:
@ -1549,7 +1549,7 @@ public:
}
case WID_BROS_IMAGE: {
uint16_t y = GB(widget, 16, 16);
uint16_t y = this->GetWidget<NWidgetMatrix>(WID_BROS_MATRIX)->GetCurrentElement();
if (y >= _roadstop_gui_settings.roadstop_count) return;
const RoadStopSpec *spec = RoadStopClass::Get(_roadstop_gui_settings.roadstop_class)->GetSpec(y);

@ -1965,8 +1965,8 @@ void NWidgetMatrix::SetColour(Colours colour)
}
/**
* Sets the clicked widget in the matrix.
* @param clicked The clicked widget.
* Sets the clicked element in the matrix.
* @param clicked The clicked element.
*/
void NWidgetMatrix::SetClicked(int clicked)
{
@ -2014,15 +2014,20 @@ void NWidgetMatrix::SetScrollbar(Scrollbar *sb)
this->sb = sb;
}
/**
* Get current element.
* @returns index of current element.
*/
int NWidgetMatrix::GetCurrentElement() const
{
return this->current_element;
}
void NWidgetMatrix::SetupSmallestSize(Window *w)
{
assert(this->head != nullptr);
assert(this->head->next == nullptr);
/* Reset the widget number. */
NWidgetCore *nw = dynamic_cast<NWidgetCore *>(this->head);
assert(nw != nullptr);
SB(nw->index, 16, 16, 0);
this->head->SetupSmallestSize(w);
Dimension padding = { (uint)this->pip_pre + this->pip_post, (uint)this->pip_pre + this->pip_post};
@ -2087,8 +2092,8 @@ NWidgetCore *NWidgetMatrix::GetWidgetFromPos(int x, int y)
int widget_row = (y - base_offs_y - (int)this->pip_pre - this->pos_y) / this->widget_h;
int sub_wid = (widget_row + start_y) * this->widgets_x + start_x + widget_col;
if (sub_wid >= this->count) return nullptr;
this->current_element = (widget_row + start_y) * this->widgets_x + start_x + widget_col;
if (this->current_element >= this->count) return nullptr;
NWidgetCore *child = dynamic_cast<NWidgetCore *>(this->head);
assert(child != nullptr);
@ -2097,8 +2102,6 @@ NWidgetCore *NWidgetMatrix::GetWidgetFromPos(int x, int y)
this->pos_y + this->pip_pre + widget_row * this->widget_h + base_offs_y,
child->smallest_x, child->smallest_y, rtl);
SB(child->index, 16, 16, sub_wid);
return child->GetWidgetFromPos(x, y);
}
@ -2137,12 +2140,11 @@ NWidgetCore *NWidgetMatrix::GetWidgetFromPos(int x, int y)
if (offs_x >= (int)this->current_x) continue;
/* Do we have this many widgets? */
int sub_wid = y * this->widgets_x + x;
if (sub_wid >= this->count) break;
this->current_element = y * this->widgets_x + x;
if (this->current_element >= this->count) break;
child->AssignSizePosition(ST_RESIZE, offs_x, offs_y, child->smallest_x, child->smallest_y, rtl);
child->SetLowered(this->clicked == sub_wid);
SB(child->index, 16, 16, sub_wid);
child->SetLowered(this->clicked == this->current_element);
child->Draw(w);
}
}

@ -564,6 +564,7 @@ public:
void SetClicked(int clicked);
void SetCount(int count);
void SetScrollbar(Scrollbar *sb);
int GetCurrentElement() const;
void SetupSmallestSize(Window *w) override;
void AssignSizePosition(SizingType sizing, int x, int y, uint given_width, uint given_height, bool rtl) override;
@ -574,8 +575,9 @@ public:
protected:
WidgetID index; ///< If non-negative, index in the #Window::widget_lookup.
Colours colour; ///< Colour of this widget.
int clicked; ///< The currently clicked widget.
int count; ///< Amount of valid widgets.
int clicked; ///< The currently clicked element.
int count; ///< Amount of valid elements.
int current_element; ///< The element currently being processed.
Scrollbar *sb; ///< The scrollbar we're associated with.
private:
int widget_w; ///< The width of the child widget including inter spacing.

Loading…
Cancel
Save