Skip to content

Conversation

@warjort
Copy link
Contributor

@warjort warjort commented May 28, 2021

What:
My fix for slots inside a scrollable list #1558 has broken the item list tab in the crafting station.
The reason is that the new check for isWidgetClickable() uses isBoxInsideScissor().

isBoxInsideScissor() requires the whole widget to be inside the scissor because it checks both the top left and bottom right corners are inside the scissor.

For the crafting station it is even worse, because the item list tab actually has each row of items (a widget group) as the direct subwidget of the scrollable list widget. And the way it is setup, the bottom right corner of that group was not considered inside the scissor because it overlaps the scroll bar.

How solved:
Changed isWidgetClickable() to use a new method that only requires one of the 4 corners to be inside the scissor.

This matches the logic added to SlotWidget.isEnabled() where it checks the scissor overlaps the rectangle of the widget, rather than requiring the whole widget to be inside the scissor.

Outcome:
You can correctly click items in the crafting station's item list tab again.

@warjort
Copy link
Contributor Author

warjort commented May 28, 2021

And the way it is setup, the bottom right corner of that group was not considered inside the scissor because it overlaps the scroll bar.

I am thinking the original logic in isBoxInsideScissor() and thus also my new code is wrong?

    private boolean isBoxInsideScissor(Rectangle rectangle) {
        return isPositionInsideScissor(rectangle.x, rectangle.y) &&
            isPositionInsideScissor(rectangle.x + rectangle.width, rectangle.y + rectangle.height);
    }

For a slot starting at (0,0) and width/height (18,18) it is testing the top left (0, 0) and bottom right (18,18)
But the bottom right should really be (17,17) ?!

@warjort
Copy link
Contributor Author

warjort commented May 28, 2021

I have added corrections to the calculation of the bottom and right co-ords.

I tested this change with the old way of using isBoxInScissor() for clickable widgets and it no longer thinks the widget group overlaps the scroll bar.

AFAICT, nothing in base GTCE actually uses this isBoxInScissor() method since it was only ever used when there is a phantom slot widget in a scroll list widget.
I am using it in my KeepInStock cover which has such a widget. But it is not a location where testing the bottom/right is important.

Copy link
Member

@LAGIdiot LAGIdiot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes seems reasonable to me.

@LAGIdiot LAGIdiot added rsr: revision Release size requirements: Revision status: accepted subsystem: gui type: bug Something isn't working labels May 30, 2021
@LAGIdiot LAGIdiot merged commit 864fe45 into GregTechCE:master May 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rsr: revision Release size requirements: Revision status: accepted subsystem: gui type: bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants