Refactor container matcher implementations into interfaces #788

Emirlol · 2024-06-19T16:13:52Z

Simple explanation of what this PR separates these classes into:

The very base interface: AbstractContainerMatcher. The main method in it takes a screen and returns a boolean. There's an abstract class called RegexContainerMatcher implementing it that takes a pattern in and matches it against the given screen's title.

Then there's the 3 interfaces for the different types we have: AbstractContainerSolver, AbstractSlotTextAdder and AbstractTooltipAdder. These contain the methods relevant to their types, basically all the methods from the previous way.

Then there are 3 simple implementations of those 3 interfaces, ContainerSolver, SlotTextAdder and TooltipAdder, all of which also extend RegexContainerMatcher. These 3 classes don't have much logic to themselves and have constructors for the regex stuff. Most adders will want to extend these 3 classes. But the separation of the base methods into interfaces that all extend the base interface allows an implementing class to implement multiple of the interfaces.
End of wall of text.

This PR also moves the container stuff into their own directory inside of the utils package.
The RegexContainerMatcher abstract class also saves the matched groups in a variable, so any class extending it will have access to the groups matched by the title pattern. This removes the need to pass around a parameter to all ContainerSolvers when only 1 of them make use of it, and also makes it possible for the SlotTextAdders and the TooltipAdders to use it if it's ever needed.

There's one possibly unwanted result of this change, and that is the implementation of interfaces have to have a visibility modifier of public.

viciscat · 2024-06-19T18:36:25Z

Don't we love refactoring PRs? Anyways uh ig my only nitpick is that I personally don't like interfaces being called "Abstract". "Abstract" makes me think i'm dealing with an abstract class.

Julienraptor01 · 2024-06-19T18:41:47Z

Don't we love refactoring PRs? Anyways uh ig my only nitpick is that I personally don't like interfaces being called "Abstract". "Abstract" makes me think i'm dealing with an abstract class.

Don't really like that neither
I think just ContainerSolver is enough
Or we can do like Microsoft Java IContainerSolver /j

Emirlol · 2024-06-19T19:17:03Z

"Abstract" makes me think i'm dealing with an abstract class.

I get that, but there's no other way that I can think of, other than that and the I-prefixed way of doing it. It's also kind of correct in the sense that the actual methods are abstract, as all interface methods are abstract by default.

I think just ContainerSolver is enough

But then there would be 2 ContainerSolvers. We'd have to rename the other one to ContainerSolverImpl and that doesn't sound like a good solution either as usually -Impl classes mean that the interface is supposed to have only one implementor, and they are usually abstracted away behind builders or static methods in the interface.

viciscat · 2024-06-19T19:18:35Z

no other way that I can think of, other than that and the I-prefixed way of doing it

ContainerSolverInterface maybe? idk

BigloBot · 2024-06-19T19:18:50Z

"Abstract" makes me think i'm dealing with an abstract class.

I get that, but there's no other way that I can think of, other than that and the I-prefixed way of doing it. It's also kind of correct in the sense that the actual methods are abstract, as all interface methods are abstract by default.

I think just ContainerSolver is enough

But then there would be 2 ContainerSolvers. We'd have to rename the other one to ContainerSolverImpl and that doesn't sound like a good solution either as usually -Impl classes mean that the interface is supposed to have only one implementor, and they are usually abstracted away behind builders or static methods in the interface.

Interfaces are inherently abstract, so you are implictly naming the file an abstract abstractContainerSolver rn haha

Emirlol · 2024-06-19T19:21:09Z

Perhaps I could remove the Abstract from the interfaces and prefix the implementations with Simple?

BigloBot · 2024-06-19T19:27:22Z

Perhaps I could remove the Abstract from the interfaces and prefix the implementations with Simple?

From my understanding the java-centric way of doing it is to add the extra info into the implementation

i.e. the List interface is implemented by the LinkedList Class & ArrayList Class

kevinthegreat1 · 2024-06-24T07:51:02Z

Oh, I would have done abstract too, but since so many people are arguing against it, this is fine.

kevinthegreat1

Additional changes in this pr:

Adds key bind that shows different info.
Passes stack and slot id instead of slot instance to slot text adders.

src/main/java/de/hysky/skyblocker/utils/container/ContainerMatcher.java

src/main/java/de/hysky/skyblocker/skyblock/item/tooltip/TooltipManager.java

kevinthegreat1 · 2024-06-24T08:08:08Z

Need to wait for a few other prs due to merge conflict.

Emirlol · 2024-07-10T14:44:46Z

This PR needs a way to fix the config to change value of the slottext config from a boolean to one of the SlotTextState enums. I know this is possible with some datafix shenanigans but I have no idea how they work or where to begin from, so any guidance would be appreciated.

kevinthegreat1 · 2024-07-18T13:53:22Z

Oh hey, this is next. Do you want me to rebase this or you will? I can handle the data fixer parts but I need to know which options map to which. It seems like slotText = false -> DISABLED, slotText = true -> ENABLED, and slotTextToggled is a new option? Or should I map slotText = false to slotText = HOLD_TO_SHOW?

Emirlol · 2024-07-18T15:20:31Z

I'd appreciate it. I've been procrastinating on my internship project a little, and now most of my time is spent on that to make up for that. Don't exactly have the time or motivation for this and probably won't for another week or two.

It seems like slotText = false -> DISABLED, slotText = true -> ENABLED

Exactly.

and slotTextToggled is a new option?

It holds the toggle state for the press to toggle option, so that it persists through reboots. It's not meant to be configured by the user.

- Refactored SlotTextAdders to take in ItemStack and int slotId rather than Slot - Refactored the renderSlotText method from HandledScreenMixin into SlotTextManager and added an overloading function that takes in the itemstack, slot id, x and y and the previous function with the Slot parameter just delegates to it with the relevant fields from that Slot object - As a result of the above 2 changes the logic in CommunityShopAdder had to be changed too, now it figures out the screen from the method calls to `getText` rather than the Slot's inventory. - Fixed slot text not being rendered in backpack preview - Added private constructor for BackpackPreview (because it's all static methods anyway)

…e the `Abstract` prefix and rename simple implementations with a `Simple` prefix

The enum SlotTextState is very self-descriptive if you wonder what the states are

kevinthegreat1 · 2024-07-22T06:39:18Z

Btw, next time anyone does a refactor pr, please try not to break the api this badly. Follow conventions like keeping old methods and marking them as deprecated.

Emirlol · 2024-07-22T06:56:48Z

Btw, next time anyone does a refactor pr, please try not to break the api this badly. Follow conventions like keeping old methods and marking them as deprecated.

I thought that since the mod doesn't have any external APIs, changing it directly without deprecating the previous way is fine as long as I change all usages. I guess that resulted in way more merge conflicts than necessary. Sorry, will be more careful next time.

kevinthegreat1 · 2024-07-22T07:00:49Z

Yes, we don't really have an api, but it's more about other prs that use the old "internal api" which can be a bit annoying. It's all good now.

kevinthegreat1

Still need to look at container solver and manager classes.

src/main/java/de/hysky/skyblocker/skyblock/dungeon/CroesusProfit.java

src/main/java/de/hysky/skyblocker/skyblock/item/slottext/adders/PotionLevelAdder.java

src/main/java/de/hysky/skyblocker/skyblock/item/tooltip/adders/MuseumTooltip.java

This reverts commit c6c3f30.

kevinthegreat1 · 2024-07-23T16:25:02Z

src/main/java/de/hysky/skyblocker/config/categories/GeneralCategory.java

+                                .name(Text.translatable("skyblocker.config.shortcutToKeybindsSettings"))
+                                .action((screen, opt) -> MinecraftClient.getInstance().setScreen(new KeybindsScreen(screen, MinecraftClient.getInstance().options)))
+                                .text(Text.translatable("skyblocker.config.shortcutToKeybindsSettings.@Text"))


These translation keys don't exist btw. Do you not have mcdev installed?

kevinthegreat1

Lgtm.

LifeIsAParadox added the reviews needed This PR needs reviews label Jun 19, 2024

kevinthegreat1 reviewed Jun 24, 2024

View reviewed changes

src/main/java/de/hysky/skyblocker/utils/container/ContainerMatcher.java Outdated Show resolved Hide resolved

src/main/java/de/hysky/skyblocker/skyblock/item/tooltip/TooltipManager.java Outdated Show resolved Hide resolved

LifeIsAParadox added merge conflicts This PR has merge conflicts that need solving. and removed reviews needed This PR needs reviews labels Jun 24, 2024

Emirlol force-pushed the container-matcher-refactor branch from deadf33 to e5a04c4 Compare July 10, 2024 14:28

LifeIsAParadox added reviews needed This PR needs reviews and removed merge conflicts This PR has merge conflicts that need solving. labels Jul 10, 2024

Emirlol added 8 commits July 22, 2024 14:20

Refactor container matcher implementations into interfaces

4c39833

Rename AbstractContainerMatcher and its extending interfaces to remov…

99dd2eb

…e the `Abstract` prefix and rename simple implementations with a `Simple` prefix

Oops, missed a few

f26ac1e

Change slot text to have multiple states

3de616f

The enum SlotTextState is very self-descriptive if you wonder what the states are

Un-debug some stuff in Utils for convenience

1a550c6

Rebase onto master

147ff2e

Change tooltip manager to allow adding tooltips on non-handled screens

e316a16

Clean up rebased code

a846b52

kevinthegreat1 force-pushed the container-matcher-refactor branch from d261bca to a846b52 Compare July 22, 2024 06:56

kevinthegreat1 reviewed Jul 22, 2024

View reviewed changes

kevinthegreat1 added 2 commits July 22, 2024 15:46

Code review part 1

14d4abe

Add slot text config data fixer

c6c3f30

kevinthegreat1 previously approved these changes Jul 22, 2024

View reviewed changes

LifeIsAParadox added merge me please Pull requests that are ready to merge and removed reviews needed This PR needs reviews labels Jul 22, 2024

Revert "Add slot text config data fixer"

156e708

This reverts commit c6c3f30.

kevinthegreat1 reviewed Jul 23, 2024

View reviewed changes

Rename slot text option to slotTextMode

eb2ac7c

kevinthegreat1 dismissed their stale review via eb2ac7c July 23, 2024 16:41

LifeIsAParadox added reviews needed This PR needs reviews and removed merge me please Pull requests that are ready to merge labels Jul 23, 2024

kevinthegreat1 approved these changes Jul 23, 2024

View reviewed changes

LifeIsAParadox added merge me please Pull requests that are ready to merge and removed reviews needed This PR needs reviews labels Jul 23, 2024

kevinthegreat1 merged commit 55349c5 into SkyblockerMod:master Jul 25, 2024
1 check passed

LifeIsAParadox removed the merge me please Pull requests that are ready to merge label Jul 25, 2024

Emirlol deleted the container-matcher-refactor branch July 26, 2024 18:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor container matcher implementations into interfaces #788

Refactor container matcher implementations into interfaces #788

Emirlol commented Jun 19, 2024 •

edited

Loading

viciscat commented Jun 19, 2024

Julienraptor01 commented Jun 19, 2024 •

edited

Loading

Emirlol commented Jun 19, 2024

viciscat commented Jun 19, 2024

BigloBot commented Jun 19, 2024 •

edited

Loading

Emirlol commented Jun 19, 2024

BigloBot commented Jun 19, 2024

kevinthegreat1 commented Jun 24, 2024

kevinthegreat1 left a comment

kevinthegreat1 commented Jun 24, 2024

Emirlol commented Jul 10, 2024

kevinthegreat1 commented Jul 18, 2024

Emirlol commented Jul 18, 2024

kevinthegreat1 commented Jul 22, 2024

Emirlol commented Jul 22, 2024

kevinthegreat1 commented Jul 22, 2024 •

edited

Loading

kevinthegreat1 left a comment

kevinthegreat1 Jul 23, 2024

kevinthegreat1 left a comment

Refactor container matcher implementations into interfaces #788

Refactor container matcher implementations into interfaces #788

Conversation

Emirlol commented Jun 19, 2024 • edited Loading

Simple explanation of what this PR separates these classes into:

viciscat commented Jun 19, 2024

Julienraptor01 commented Jun 19, 2024 • edited Loading

Emirlol commented Jun 19, 2024

viciscat commented Jun 19, 2024

BigloBot commented Jun 19, 2024 • edited Loading

Emirlol commented Jun 19, 2024

BigloBot commented Jun 19, 2024

kevinthegreat1 commented Jun 24, 2024

kevinthegreat1 left a comment

Choose a reason for hiding this comment

kevinthegreat1 commented Jun 24, 2024

Emirlol commented Jul 10, 2024

kevinthegreat1 commented Jul 18, 2024

Emirlol commented Jul 18, 2024

kevinthegreat1 commented Jul 22, 2024

Emirlol commented Jul 22, 2024

kevinthegreat1 commented Jul 22, 2024 • edited Loading

kevinthegreat1 left a comment

Choose a reason for hiding this comment

kevinthegreat1 Jul 23, 2024

Choose a reason for hiding this comment

kevinthegreat1 left a comment

Choose a reason for hiding this comment

Emirlol commented Jun 19, 2024 •

edited

Loading

Julienraptor01 commented Jun 19, 2024 •

edited

Loading

BigloBot commented Jun 19, 2024 •

edited

Loading

kevinthegreat1 commented Jul 22, 2024 •

edited

Loading