JamienAU / RandomHearth

Random Hearthstone WoW Addon
0 stars 3 forks source link

Tooltip no longer reflects Hearth Toy #10

Closed zecmo closed 2 months ago

zecmo commented 3 months ago

Greetings. I really enjoy your work, RandomHearth and RandomCooking. I learned a lot from them which allowed me create my own add-on projects [all on git hub]. I can see much has been reworked internally with this project [the addition of Garrison and Dalaran clicks is nice].

Previously, the tooltip and icon would update with the next hearth toy. I really preferred that functionality so I took a pass at returning it. I forked this repo for your review.

I invite you to review it and if you approve, I'd be happy to create a PR on this repo's mainline.

Thanks for your add-ons and inspiration!

zecmo commented 3 months ago

Another note: I like the ability to directly select your preferred icon. You'll see that I did not change that. The icon is only updated if Random is selected.

I would love the ability to update the macro name as well. I've been updating it myself which I see it now located in the Localization file: L["MACRO_NAME"] = "Random Hearth" -> "Home"

JamienAU commented 3 months ago

Hey mate, thanks for the kind words!

That tip for the macro update wait is good, thanks! PostClick is probably a better option for the randomising, brings it back in line with what I originally had.

I'll look at implementing both of those shortly, and I'll work in a way to manually set the macro name rather than making you edit the LUA. Should be easy enough, just need to work it in with the savedvariables and some additional settings panels.

zecmo commented 2 months ago

Thanks : D

About the PostClick, while working I tried keeping the idea of right/middle clicks check and only update the macro when an actual hearth toy is used by tracking with a bool. Yet sometimes if i used either, but let's say Dalaran, then cancelled and tried to left click for hearth, it would trigger Dalaran one more time. Requiring a second left click to trigger the hearth. Couldn't figure out why, so I abandoned it and just call setRandom() on any click.

I mention all this bc I think it makes more sense to only update the macro when hearth is used and not when Garrison/Dalaran is used. It's a minor point but I thought it was worth sharing.

"I'll look at implementing both of those shortly"

Ooohh, excited. CustomName++ 👍 Thank you for considering my feedback. Looking fwd to the next version!

JamienAU commented 2 months ago

About the PostClick, while working I tried keeping the idea of right/middle clicks check and only update the macro when an actual hearth toy is used by tracking with a bool.

That one is an easy enough thing to implement - macroName is defined at the main script level, then gets set by setRandom(). Any other functions in the same file can reference that variable. When the player right or middle clicks all it's doing is updating the button attributes itself, changing the toy name value. On post click I'm just setting that toy name back to the value of macroName, as this hasn't changed. Copying the checks from the btn 2/3 pre-click means that it won't attempt to update the value if dalaran or garrison hearth aren't enabled.

On your timer thing, you shouldn't need to have a C_Timer.NewTimer inside C_Timer.After. Tested this one out, looks to work fine. The addition of macroTimer is to ensure that only one update gets queued. If the player clicks multiple times it will only start one timer, then action after that period ends. I haven't seen any text getting dumped into chat from using this method, so thanks again for mentioning it!

The version in addReqs branch should work and now does the randomising on postclick. Will get it into master once I've sorted the custom naming of the macro. Though that'll probably have to wait until the weekend, as IRL work is melting my brain :D

JamienAU commented 2 months ago

@zecmo - Haven't merged to master yet as I need to test to make sure it works in other languages, but the addReqs branch should be functional with the ability to set the macro name.

If you wouldn't mind testing it out and letting me know if you have any issues? That would be appreciated!

zecmo commented 2 months ago

@JamienAU I'll test it now!

zecmo commented 2 months ago

@JamienAU It's looking great. Changed name a few times without issue [good feedback on existing macro names.] Clicked a bunch of times with all three clicks and nothing seemed to break. PR approved!

Note: I did notice a little 2 subscript on the bottom right of the macro. Not a bug, yet was curious if this was intentional. Not sure I've seen that unless it indicates multiple charges.

Looking forward to the next release : D

JamienAU commented 2 months ago

Note: I did notice a little 2 subscript on the bottom right of the macro. Not a bug, yet was curious if this was intentional. Not sure I've seen that unless it indicates multiple charges. @zecmo are you able to provide a screenshot of this please? I'm not sure what you're referring to

zecmo commented 2 months ago

Sure. Hotkey = #7 Name = p [for readability] 2 = ??

Screen Shot 2024-08-24 at 6 53 22 PM
JamienAU commented 2 months ago

Ahh yeah, thanks. I've seen that happen before on one of my other characters but I didn't look into why.