[Discussion] EstTodayHonor column

[Slivo-fr]

I feel like this column is unnecessary and overcomplicate the GUI. In addition it's not always accurate, like when someone hasn't relogged for days or when the addon get an updated blizz honor value without the updated honor estimation value.

I would like to make a PR that either removes the column completely or puts it behind a separated toggle option. (Preferably the removal, less code) The former design with "honor" for blizz value and "estHonor" for estimated week honor is better in my opinion.

Would such a PR get accepted ?

[teelolws]

Its already behind the toggle option:

If you want to add a separate toggle option for each column, go ahead. But I don't support removal. I added that column because I need to see that data.

[Slivo-fr]

I was suggesting a separate toggle yeah.

May I ask what is the use case for those data ? I can't think of a way I could use it

[teelolws]

Its how I quickly check who didn't have their daily honor rollover. Also some people on Season of Mastery are injecting fake "estimated honor" data and that column makes it obvious I can ignore them.

[Slivo-fr]

I understand. Its kind of a niche usage however, for most users it seems overcomplicated to me. Maybe having a "Show advanced honor estimation colums" that would switch between the old and the new display could to the job, defaulted to false

[teelolws]

Before I changed it, the "EstTodayHonor" column was already there, locked behind that "Show Estimated Honor" button, but named "EstimatedHonor". I simply added another column that added KnownHonor + EstimatedHonor so I could see what the players actual total honor is. It helps on reset day for players who are pvping on reset day, so we can see the final standings before the calculations are done.

You're asking for a column to be removed or locked behind a different button, that was already there before, just had a different name.

[Slivo-fr]

I may be wrong, but I think the original estHonor column was related to the estimated week honor. It was showing the total estimated honor of each player for the week. If that's indeed the case, the estTodayHonor had been added and is the column I'm discussing about.

I will take a look at previous codebase see how it really was, maybe my memory is cheating me

[Slivo-fr]

Well I didn't go all the way through the code, but here is a screen shot where you can see the estHonor column shows the total estimated honor for the week aside to the blizz value

Here was the code snipet from the 1.7.7 to confirm

            if HonorSpy.db.factionrealm.estHonorCol.show then 
                button.EstHonor:SetText(colorize(estHonor, class)); 
                button.EstHonor:SetWidth(80);
            else 

[teelolws]

I may have misunderstood what that column was presenting, and in the process have changed what original_honor and estimated_honor are storing. Looking at Kakyshas most recent commit: https://github.com/kakysha/HonorSpy/blob/c474edb437a0182d897fc091f9d58622a96a782a/honorspy.lua if (HonorSpy.db.factionrealm.currentStandings[playerName] and HonorSpy.db.char.original_honor ~= HonorSpy.db.factionrealm.currentStandings[playerName].thisWeekHonor) then HonorSpy.db.char.original_honor = HonorSpy.db.factionrealm.currentStandings[playerName].thisWeekHonor HonorSpy.db.char.estimated_honor = HonorSpy.db.char.original_honor When HonorSpy updates after a daily honor rollover, both original_honor and estimated_honor were set to the same number. As the player pvps over the day, original_honor stays the same, estimated_honor increases.

In building the standing table: for playerName, player in pairs(HonorSpy.db.factionrealm.currentStandings) do table.insert(t, {playerName, player.class, player.thisWeekHonor or 0, player.estHonor or "", player.lastWeekHonor or 0, player.standing or 0, player.RP or 0, player.rank or 0, player.last_checked or 0}) player.estHonor comes from if ( inspectedPlayerName == playerName ) then player.estHonor = HonorSpy.db.char.estimated_honor end which is what you said, the weekly running total.

Thats my mistake, I misunderstood the reason for that column. But its okay. Ive separated it into "today" and "this week" and added toggle options for each. Check the pull requests in your fork. https://github.com/Slivo-fr/HonorSpy/pull/2

[Slivo-fr]

Thanks for the PR, I'm taking a look at it !

[Slivo-fr]

190 will include the changes that adds a toggle, thanks :)