AznamirWoW / PallyPower

The official repository of PallyPower for retail version of World of Warcraft Classic/TBC
Other
6 stars 6 forks source link

Issue 2 save normal blessings / allow Buff Presets #10

Closed SaschaJankowiak closed 1 year ago

SaschaJankowiak commented 1 year ago

As written in #2 there is a big request in saving presets for your raid - especially the normalBlessings. I've done a basic Buff Preset function. Works by clicking "Preset" while holding shift to store current configuration and clicking without modifier to restore the buffs.

I've coded this with extendability in mind (e.g. do a submenu with multiple profiles later on). But for now this should work. For the implementation i used the unused "PallyPower_SavedPresets" Variable allready referenced by the TOCs. As for Localization there are 2 new Strings to add (my proposals):

L["Preset"] = "Preset"
L["PRESET_TOOLIP"] =  [=[Load the last saved Preset 
and send them to all Pallys if Leader.

|cffffffff[Shift-Left-Click]|r Save a preset 
of all Greater and Normal Blessings 
currently configured.]=]
Treeston commented 1 year ago

one pull request per feature, please

SaschaJankowiak commented 1 year ago

but it is - it's just 2 commits as the commit "lay-on-hands" has nothing to do with the feature itself - just noticed is was missing in the current master

Treeston commented 1 year ago

then make a separate PR for the lay on hands change, or remove it from this branch

SaschaJankowiak commented 1 year ago

then make a separate PR for the lay on hands change, or remove it from this branch

done

AznamirWoW commented 1 year ago

Just from a usability standpoint, it would make more sense to have two buttons - one for saving and one for loading of presets. Also would be helpful if you could give a name of the preset, new or existing, for saving and be able to choose which one to load.

SaschaJankowiak commented 1 year ago

Yeah there is a lot of room for improvement when you think of this kind of feature. The Question is: is this an usable Feature which can extend current functionality without much hassle and a big ole "Profile Manager" or would this be enough in the first place to see if the big one will be requested much.

About two Buttons: I thought of People who will use this Feature - mostly Class-Leaders/Officers/Raidleaders and their assistants (which i hope they can read Tooltips) and therefore i would like to not blow up this interface with a lot of Buttons.

As for multiple Profiles, there would have to be another UI-Layer or some kind of submenu for managing these with good usability in mind

SaschaJankowiak commented 1 year ago

the main usecase probably will be raiding with mostly the same People over and over again - when the roster changes alot you will have to manually do the setup anyway

Treeston commented 1 year ago

isn't the significantly easier fix just storing assigned small blessings by unit name?

SaschaJankowiak commented 1 year ago

they would change in 10-man where u will have between 1-3 pallys on board - thus overwriting the 25-man assignments :)

Treeston commented 1 year ago

fair, i suppose...

still feels like this is fixing around the actual problem, which is a lack of functional auto-assign

SaschaJankowiak commented 1 year ago

Auto-Assign in its current form was optimized for classic-wow before normal-blessing-management existed and for a setup of 4-6+ Pallys in AQ40-Raids. And when it was not changed too much - it should do its job quite well in SoM.

Doing a really well designed auto-assign would either need some kind of generally accepted static buff priority in each TBC/WOTLK by spec or a custom priority editor.

Treeston commented 1 year ago

LGTM code wise. did you test it, both as leader and not as leader? are you sure there aren't desyncs if you try to restore a preset as non-leader where other paladins' assignments do not agree with current?

ex. Bob's currently set to big Kings, your preset has Bob set to big Might, what does your PP show after loading the preset? does it agree with what Bob's PP shows?

SaschaJankowiak commented 1 year ago

I'll do some enhancements with this in mind (current push)- but that's untested and i cannot test this today

SaschaJankowiak commented 1 year ago

finished feature (one-button-solution) with preventing async. tested in group and raid environment. Stays backward-compatible because no new comm- or datastructure is needed on foreign pallys

SaschaJankowiak commented 1 year ago

would like to do a push with alle the changes, but i cannot login - serverlist not loading for testing...

Treeston commented 1 year ago

PTR not up either?

SaschaJankowiak commented 1 year ago

PTR works, but without another pally to test with nothing will really happen :)

Treeston commented 1 year ago

you can have up to 4 PTR accounts, it's great for testing

Treeston commented 1 year ago

I think the Aura thing makes sense.

SaschaJankowiak commented 1 year ago

Any more feedback on this? :)

when there isn't anything against this we could probably push an alpha version to let the community give feedback.

SaschaJankowiak commented 1 year ago

how does the localization-Stuff on curse work? Has the Key to be added manually or does curse scan the files to create the keys itself?

Treeston commented 1 year ago

I'll add it manually before merging the PR.

SaschaJankowiak commented 1 year ago

functionality lgtm, just some minor cleanups needed

did cleanups and tested on ptr

Treeston commented 1 year ago

v1.4.6-classic-alpha2 package kicked off