Avoid consuming scroll event if mouse is in exclusion area#222
Conversation
ab3ce4a to
330af8f
Compare
|
before: before.mp4after: after.mp4 |
WaitingIdly
left a comment
There was a problem hiding this comment.
isMouseOver is used for a number of things, and removing this check means your goal of "not running multiple times" causes other other places to run checks when they shouldnt. should keep/add && !guiScreenHelper.isInGuiExclusionArea(mouseX, mouseY) to relevant isMouseOver checks. this also makes it the same as all of the other similar methods in \w*GridWithNavigation.
WaitingIdly
left a comment
There was a problem hiding this comment.
i dont see the need for defense comments about how particular checks arent required.
51e1193 to
54767bb
Compare
|
This PR is rewritten again for a working implementation with (hopefully) minimum code logic change. The usage of |
WaitingIdly
left a comment
There was a problem hiding this comment.
think IngredientListOverlay could be neater, but looks good
| boolean result; | ||
| if (isListDisplayed()) { | ||
| if (Config.isCenterSearchBarEnabled() && searchField.isMouseOver(mouseX, mouseY)) { | ||
| return true; | ||
| result = true; | ||
| } else { | ||
| result = displayArea.contains(mouseX, mouseY); | ||
| } | ||
| return displayArea.contains(mouseX, mouseY); | ||
| } else if (this.guiProperties != null) { | ||
| return this.configButton.isMouseOver(mouseX, mouseY); | ||
| result = this.configButton.isMouseOver(mouseX, mouseY); | ||
| } else { | ||
| result = false; | ||
| } | ||
| return false; | ||
| return result && !guiScreenHelper.isInGuiExclusionArea(mouseX, mouseY); |
There was a problem hiding this comment.
always need to check it, might as well do it first.
| boolean result; | |
| if (isListDisplayed()) { | |
| if (Config.isCenterSearchBarEnabled() && searchField.isMouseOver(mouseX, mouseY)) { | |
| return true; | |
| result = true; | |
| } else { | |
| result = displayArea.contains(mouseX, mouseY); | |
| } | |
| return displayArea.contains(mouseX, mouseY); | |
| } else if (this.guiProperties != null) { | |
| return this.configButton.isMouseOver(mouseX, mouseY); | |
| result = this.configButton.isMouseOver(mouseX, mouseY); | |
| } else { | |
| result = false; | |
| } | |
| return false; | |
| return result && !guiScreenHelper.isInGuiExclusionArea(mouseX, mouseY); | |
| if (guiScreenHelper.isInGuiExclusionArea(mouseX, mouseY)) { | |
| return false; | |
| } | |
| if (isListDisplayed()) { | |
| if (Config.isCenterSearchBarEnabled() && searchField.isMouseOver(mouseX, mouseY)) { | |
| return true; | |
| } | |
| return displayArea.contains(mouseX, mouseY); | |
| } | |
| if (this.guiProperties != null) { | |
| return this.configButton.isMouseOver(mouseX, mouseY); | |
| } | |
| return false; |
There was a problem hiding this comment.
The weird code logic here is because when I was writing it, I considered exclusion area check as something expensive and should be done last, just like other usages. But probably it doesn't matter, since it won't be called thousands of times per second?
This PR added an additional exclusion area check to avoid consuming scroll event, if the mouse is in exclusion area. Other exclusion area check in
IMouseHandler.isMouseOver(int,int)is removed, to 1. keep implementations consistent, and 2. avoid performing the same check multiple timesShould be able to fix some mods having their scrolling handler not working, as long as they declared correct exclusion areas.