Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Shows if player have enough supplies #28

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

SeaifanAladdin
Copy link
Contributor

I had reviewed the comments from #25 and completed the requested change to PeaceBear0's implementation.

I apologize if this PR causes any issues, as I notice there has been no updates with #25 and I am unfamiliar with the proper procedure or etiquette in regards to finishing up someone else's PR.

PeaceBear0 and others added 5 commits March 13, 2023 20:10
Add an option to show the player if they have enough supplies in their
inventory to complete their current contract. If turned on, it shows it on the
right side of the overlay. If the Plank Sack plugin is installed, it also
counts planks in the plank sack.
…ntainSupplies

# Conflicts:
#	src/main/java/thestonedturtle/mahoganyhomes/MahoganyHomesPlugin.java
Copy link
Owner

@TheStonedTurtle TheStonedTurtle left a comment

Choose a reason for hiding this comment

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

I didn't have time to actually pull this branch down and test it so I may have additional changes later. Please address my current ones for now though.

keyName = "checkSupplies",
name = "Check Supplies",
description = "Checks if you have enough supplies in your inventory to complete your current contract.<br/>" +
"If the Plank Sack plugin is installed, it will include planks in the plank sack.",
Copy link
Owner

Choose a reason for hiding this comment

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

Might want to add the caveat of "regardless of if the planks in the sack are the correct type for your contract tier" or something here.

Copy link
Contributor Author

@SeaifanAladdin SeaifanAladdin Mar 26, 2024

Choose a reason for hiding this comment

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

Tried adding a short and concise caveat but let me know if you'd like me to be more specific (i.e. "for your contract tier")

I realized that check supplies wont work without "Display Required Materials", so I wanted to check with you if it should mention that it needs to be enabled. Similar to how Display Session Stats config mentions that it requires Display Text Overlay to be enabled

@Getter
private int numPlanksInInventory = 0;
@Getter
private int numSteelBarsInInventory = 0;
Copy link
Owner

Choose a reason for hiding this comment

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

Reset these onShutDown

@Subscribe
public void onItemContainerChanged(ItemContainerChanged event)
{
if (event.getContainerId() == InventoryID.INVENTORY.getId())
Copy link
Owner

Choose a reason for hiding this comment

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

Only do this is they currently have an active contract and the plugin hasn't "timed out". Many people leave the plugin on at all times even if they aren't actively doing the content.

@@ -157,6 +174,11 @@ public void shutDown()
@Subscribe
public void onConfigChanged(ConfigChanged c)
{
if (c.getGroup().equals(PLANK_SACK_GROUP_NAME) && c.getKey().equals(PLANK_COUNT_KEY))
Copy link
Owner

Choose a reason for hiding this comment

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

Only do this is they currently have an active contract and the plugin hasn't "timed out". Many people leave the plugin on at all times even if they aren't actively doing the content.

@@ -674,4 +706,35 @@ private void refreshTeleportItem(final WorldPoint playerPos)
teleportItem = null;
}
}

void updateResourcesInInventory()
Copy link
Owner

Choose a reason for hiding this comment

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

Due to the other requested changes we need to call this whenever a new contract is received.

Also, ensure this gets called when enabling the plugin if they have an active contract. This may need to be done inside onStartUp but I believe calling it in the above scenario should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Due to the other requested changes we need to call this whenever a new contract is received.

Done in checkForContractTierDialog()

Also, ensure this gets called when enabling the plugin if they have an active contract. This may need to be done inside onStartUp but I believe calling it in the above scenario should work.

I actually found calling this at the end of loadFromConfig() to be more ideal, as it additionally covers my other scenarios.

@TheStonedTurtle
Copy link
Owner

Well it's been 6 months and I'm not sure if this is ready for re-review. @SeaifanAladdin if you're still around is this ready for review again?

@SeaifanAladdin
Copy link
Contributor Author

Well it's been 6 months and I'm not sure if this is ready for re-review. @SeaifanAladdin if you're still around is this ready for review again?

It looks like there is a conflict that needs resolving. I also would like to look into any new OSRS changes since, and retest before having this reviewed again.
Please let me know if you have a preferred timeline. As of right now, I am currently not active in the game but am looking to re-subscribe in the coming months.

@TheStonedTurtle
Copy link
Owner

No rush on my end, whenever you want is fine with me

@woweow
Copy link

woweow commented Nov 17, 2024

Is there something I can do here? I have both 1) found myself interested in contributing to RuneLite and 2) have began the Mahog Homes grind on my iron and was looking to implement these exact features.

If I pull this down, resolve the conflict, do mahog homes for a few hours and ensure I'm not seeing anything wrong can I make a PR for this and get it reviewed and merged?

@TheStonedTurtle
Copy link
Owner

Is there something I can do here? I have both 1) found myself interested in contributing to RuneLite and 2) have began the Mahog Homes grind on my iron and was looking to implement these exact features.

If I pull this down, resolve the conflict, do mahog homes for a few hours and ensure I'm not seeing anything wrong can I make a PR for this and get it reviewed and merged?

Sounds fine to me.

@woweow
Copy link

woweow commented Nov 27, 2024

Hey there! I have finally gotten my RL dev environment set up. Would you like me to update this CR directly? I was able to bring it down to my own machine with gh pr checkout 28. I'm not sure how best to proceed. I could rebase the changes I am making into the original commit and make a second revision? Or make an entirely new fork?

Please advise! This is my first time contributing to RL + plugin hub. Thank you!

@SeaifanAladdin
Copy link
Contributor Author

Apologies for the late update.
I applied the merge changes, and included some more update resource calls to the recently added dialog changes.

@TheStonedTurtle , the PR is now ready for review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants