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

Create recommended-equipment #6012

Merged
merged 4 commits into from
Jul 26, 2024
Merged

Create recommended-equipment #6012

merged 4 commits into from
Jul 26, 2024

Conversation

t8or
Copy link
Contributor

@t8or t8or commented May 15, 2024

Added Recommended Equipment plugin. This awesome plugin takes your OSRS gameplay to the next level by seamlessly integrating equipment recommendations from the wiki, via the approved wiki scraper, directly into your RuneLite client and then filtering your bank, similar to Quest Helper.

It helps you gear up quickly for bossing and mini-games by using wiki data without ever leaving RuneLite.

Added Recommended Equipment plugin
@runelite-github-app
Copy link

runelite-github-app bot commented May 15, 2024

@adamk33n3r
Copy link
Contributor

image
I'm not sure why it's saying it needs to wrap at 120 when it's at 114 🤔

@raiyni
Copy link
Member

raiyni commented May 15, 2024

@adamk33n3r
Copy link
Contributor

oh fun.....lol why EIGHT for tabs :lul:

@Nightfirecat
Copy link
Member

that's the default size of tabs in github's ui

@adamk33n3r
Copy link
Contributor

alright, i converted the file to spaces so hopefully that works. but i dont have permissions for @t8or's fork at the moment to update the hash, but he can do it in the morning. t8or/runelite-recommended-equipment@a41ed77

t8or added 2 commits May 15, 2024 06:43
Fixed Gradle format error
@Nightfirecat
Copy link
Member

You should absolutely not be mutating event objects, that would negatively impact every other plugin which would be listening for it.
What's going on with RecEquipLAF? Why does it require reflection? How is that needed here? I'm generally a little confused about all this LAF code (both for this file, and the use of RuneLiteLAF) you have going on.
You add your nav button here which is never removed, even on plugin shutdown. Please ensure it is removed when disabling your plugin.

@adamk33n3r
Copy link
Contributor

Hey nfc, I'm the dev on this project.

You should absolutely not be mutating event objects, that would negatively impact every other plugin which would be listening for it.

The code in the banktab package is more or less copied from QuestHelper. That line specifically is also in quest helper here: https://github.com/Zoinkwiz/quest-helper/blob/master/src/main/java/com/questhelper/bank/banktab/QuestBankTab.java#L227 I don't know exactly what it's doing, or if it's needed for our case. I just tried to keep everything there.

What's going on with RecEquipLAF? Why does it require reflection? How is that needed here? I'm generally a little confused about all this LAF code (both for this file, and the use of RuneLiteLAF) you have going on.

The reflection specifically is copy/pasted from the RuneLiteLAF to get the colors from the colorscheme into the style file. As for what RecEquipLAF is for, it's to customize the style for our own panel. Whenever our panel is open, we switch to RecEquipLAF and when our panel is closed we switch back to RuneLiteLAF. You can see that here. Unfortunately, swing doesn't really support multiple LAF's so this was the cleanest solution I could come up with. You can see our stylesheet here

You add your nav button here which is never removed, even on plugin shutdown. Please ensure it is removed when disabling your plugin.

Yep, that's totally my bad. Speaking of shutdown, should we be removing the sprite override as well? (if possible)

@raiyni
Copy link
Member

raiyni commented May 24, 2024

  1. You should be doing what bank tag layouts does

  2. You should be able to set whatever properties on the object. Swapping in a new LAF is not good.

@t8or
Copy link
Contributor Author

t8or commented May 24, 2024

Hey Rayini,

Can you explain why we should be doing what bank tags does instead of quests helper?

Can you point us in the direction to set visual design properties on the objects instead of using a LAF? We couldn't find an easy way to adjust objects that repeat many many times; hence creating the LAF.

@adamk33n3r
Copy link
Contributor

adamk33n3r commented May 24, 2024

More specifically, rounded buttons. Can easily make a rounded border, but the background would still be a rect. flatlaf has the capability to do full rounded buttons. Maybe some manual paintComponent override or something? 🤮

I think you can set a client prop on a button to get the rounded button visual, it was just way easier to combine all styles into a stylesheet. I can look into what it would take to do everything in code.

I do wonder why swapping in a new LAF is not good though. Isn't that how apps switch themes?

@abextm
Copy link
Member

abextm commented May 24, 2024

the laf is global, you are not allowed to set it

@abextm
Copy link
Member

abextm commented May 24, 2024

and really, you should not be using rounded buttons because nothing else in the client uses rounded buttons; your plugin integrates into the client, not the other way around.

@t8or
Copy link
Contributor Author

t8or commented May 24, 2024

This is how the plug-in should look. Would love direction on how to edit each object so it inherits this styling.

image

@abextm
Copy link
Member

abextm commented May 24, 2024

Swing doesn't support multiple styles in a single application, you should inherit the styles provided by the LAF so it looks like the rest of the application.

@t8or
Copy link
Contributor Author

t8or commented May 24, 2024

Appreciate the direction Abex!

@Intecpsp
Copy link

Intecpsp commented May 24, 2024

I'm not a developer/maintainer of this particular repo/application, nor do I have any say in the approval of this PR. I'm just a software engineer who follows chatter on repos and PRs I find interesting (like RL and this plugin in particular).

Another way to word this is that you want your plugin's panel to feel like it's part of the core RL app (even though we all know it isn't). And if you stray too far from the core with your own LAF it can feel jarring to the end user and feel like it's own app inside of RL (instead of a core app/plugin, like mentioned before).

That is all, back to lurking. Good luck with your plugin, I'm looking forward to seeing it on the hub!

@adamk33n3r
Copy link
Contributor

Alrighty, removed the custom LAF and removed that event mutation 👍🏻

@t8or
Copy link
Contributor Author

t8or commented Jun 16, 2024

It's been a little while. Just wanted to check in and see if there is anything else to fix before merging into the plug-in hub. Thank you!

@raiyni
Copy link
Member

raiyni commented Jul 2, 2024

Core is adding bank tag layouts and will be exposing an API to register your own layouts, recommend looking at that.

@t8or
Copy link
Contributor Author

t8or commented Jul 2, 2024

@raiyni do you know when that API will be available?

I'm anxious to get this plugin released as I know it will be a great QoL update for all the new equipment and combat changes that just came out.

@YvesW
Copy link
Member

YvesW commented Jul 2, 2024

It's already on master, so next release.
runelite/runelite@ee0c998

@t8or
Copy link
Contributor Author

t8or commented Jul 3, 2024

Just so I can make sure I understand, this PR won't be merged until the plugin is changed to use the API that will be out soon?

@adamk33n3r
Copy link
Contributor

Does this new API let us create sections and dividers like we have in this plugin?
image

I looked through the commit you linked @YvesW but I'm not seeing anything that would let us do that, the sections especially are important.

@geheur
Copy link
Contributor

geheur commented Jul 5, 2024

That is not currently supported.

@t8or
Copy link
Contributor Author

t8or commented Jul 25, 2024

With the announcement of Bank Tags in the new official client, is that using the same API as the one above? I hope that this plugin can be integrated before the great plugin migration.

@Nightfirecat Nightfirecat merged commit 4c1db29 into runelite:master Jul 26, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants