v2.7
Closed this issue · 135 comments
Just a thread to discuss some changes for 2.7
I changed the layout/text for the descriptions in cdb4600, but I'm not sure if it could be better still. Any inputs @SafeteeWoW ?
Look good for me.
Found a bug:
Because we set the equipLoc of the armor token, :AutoPassCheck no longer works for the cloak armor token because of the first line of this function.
function RCLootCouncil:AutoPassCheck(subType, equipLoc, link, isToken, isRelic)
if not tContains(autopassOverride, equipLoc) then
This breaks backward compatibility.
We have 2 solutions:
- Break compatibility and fix the
:AutoPassCheck- Pros: We store the slot information of armor token in the loot history
- Cons: Break backward compatibility
- Dont break it and ML does not set the
equipLocof armor type and we change:GetItemTypeText().- Pros: We keep the original meaning of
equipLocthat only equipable item has its value set and keeps the backward compatibility. - Cons: no slot information in the loot history?
- Pros: We keep the original meaning of
I haven't really decided if we should break backward compability this version or wait. The list of "nice-to-have-breaking-changes" is getting bigger though - I just want to make sure to cover as many as possible so I won't have to do again next update.
As for the "don't break con": we don't save nor use it in the history anyway, and we could with relative ease restore the equipLoc client side in say :LocalizeLootTable().
As for the release schedule of v2.7: I hope to have an alpha version up by tomorrow. Blizzcon is next week, and everyone thinks patch 7.3.5 drops by the reset after that, i.e. 7-8 november; that would make an opportune moment for 2.7 to be released. But I'd probably prefer to release next week to have some time for potential errors before 7.3.5.
Change the frame height of loot frame to 80 is not good for some locale. It looks good for English.
No vertical space in Chinese Simplified (zhCN).
Move up the ilvl text does not solve the problem because it creates display issue in other locale.
Back to 85 it is.
How's b58bbdd looking?
Here's the english:
Still using 80px entry height, but with the icon scaled to 78% it gives more space: 80*0,78 = 62,4 vs 85*0,73 = 62,02.
Looks fine for zhCN. testing other locale. Can you test it on other locale too? Not hard to switch and install other language in Battle.net.
Done testing zhCN, zhTW, koKR. Looks good.
Making a PR.
I could, but afaik it's only zhCN/zhTW that special due to it's different font sizes.
Remove L["g1"] and L["g2"] is not a good idea. Just reverse that commit edae461
Similar to loot history, can you add messages in
Given the limited time frame till 7.3.5 I don't want to break backwards compability in v2.7. It really doesn't cause any problems nor any massive overhead all the changes considered.
Just let you be aware that sometimes the filter texts is colored, even if nothing is filtered. Happened before you merged my PRs today.
Saw some long standing feature requests on the curseforge. Will you implement the feature to enable RC with ML (in v2.8)? I think it's not hard to do. The addon just needs to treat the group leader as the ML if the loot method is not master loot and several option need to be added.(And this does not break backward compatibility, because guild using personal loot don't install old versions of RCLootCouncil.)
In RCLootCouncil-EPGP, I plan to add text "GP:xxx" in the itemlvl text of the Loot Frame. How should I implement it if the item gp is sent via the loot table?
I think prehooks to LootFrame.EntryManager:GetEntry then posthooks to entry:Update will do it and I need a addon:GetLootTable() function which does not exist right now. (There is RCVotingFrame:GetLootTable(), but this does not work if you are not council)
Filters
I haven't been able to replicate that. Can you consistently replicate it?
Override
Take a look at 8930f24. I did prepare for it, but never got around to doing much with it, as it seemed weird, with a lot of potential for breaking stuff. As far as i remember, this did work as intended though.
LootFrame ilvltext
If the item link is all you need to extract the GP value, then it's available in the arguments of Entry:Update(entry, item) with item.link. Anywhere else you can get it with entry.item.link. In fact, all the item info delivered to lootFrame is available in both.
I think the easiest way though, is to extract the entry from EntryManager:GetEntry, and then just edit entry.itemLvl directly. This needs to be a posthook though, as it would otherwise be overridden by the original call.
Filters
Cant consistently replicate it. Happens randomly. only when a session just started.
Override
If you add a variable override. There is a lot of potential to break stuff because it adds complexity. I mean that addon.IsMasterLooter= if i am the group leader; addon.masterLooter=group leader when the loot method is not master loot. This changes apply to everyone.
This still guarantees that there is only one "ML" at any given time in a group, and this is a simple solution that shouldn't break stuff.
I think you just need to modify :NewMLCheck, :GetML, :OnRaidEnter a bit, and add several option.
LootFrame ilvltext
This could work. I just want to get gp value from the loottable(because ML can have different gp settings), which needs to be globalized.
Can you change the note icon on the lootframe a bit, so it looks different after note is done?
Filters
So it happens often? I don't really see how this can happen, nor have I seen it myself yet.
Override
Doing what you're suggesting would always make the group leader the ML, which affects stuff like the popup when entering a dungeon, and depending on settings, auto enable RCLootCouncil on every dungeon. I'm pretty sure there's some other things as well, but I can't remember any on top of my head - I do remember, though, that an opt in was the "best" way to do it when I started to implement it.
ilvl text
Well, after I changed the way the lootTable is handed to votingFrame, the lootTable in core.lua and votingframe.lua is the same table, so :GetLootTable() should just be moved to core.lua, just to make it more accesible and not being dublicated.
Note
Yeah - any ideas? The only one I can think of right now is to desaturate it.
2.7
How extensive have you tested #39, #43 and #44 ? I haven't had much time lately, so I'm thinking about doing a quick alpha release today or tomorrow, and then doing a release sunday, just to have a few days with a broader userbase for potential bugs before Antorus opens. Those 3 features are the only ones I'm nervous about adding without having raid tested them myself. Otherwise the might have to wait for the next update (this one is kinda huge already).
All pull requests are tested extensively in '/rc test'. Haven't tested in the real raid. I do need to test again after all PRs are merged though.
Note
I think loot frame should show the content of note in some way after note is entered. How about show the editbox on the right of loot frame after note is entered?
It does if you mouse over it. I agree there should be an indicator for when the noted is there, but I don't think it should be displayed all the time.
Gonna make a PR that shows note when we are mouseovering response button
Done. #51
Override
Doing what you're suggesting would always make the group leader the ML, which affects stuff like the popup when entering a dungeon, and depending on settings, auto enable RCLootCouncil on every dungeon. I'm pretty sure there's some other things as well, but I can't remember any on top of my head - I do remember, though, that an opt in was the "best" way to do it when I started to implement it.
- need to add several options to let the group leader to choose whether show popup when entering the dugeon/auto enable RC, when loot method is not master loot.
- Need to disable
ML:LootOpenedwhen loot method is not master loot
See PR #52
I have implemented all open feature requests of RCLootCouncil on Curseforge. Hope we can see these features on v2.7
I suggest you to make a TLDR section in the changelog
I have to sleep now. If there is any issue, I will resolve those when I get up.
I've decided to make the cut here - mainly based on my comments in individual PRs, but also due to the massive amount of changes there's already in the update. It's not good to add too much at once - it becomes overwhelming pretty fast.
I'm planning to run the current test version until sunday and then release it. That'll give a few days with more testers before Antorus. I'm not home until sunday afternoon (GMT+1 btw), but feel free to leave your comments.
Also, a huge thanks for all your great work!
- Don't like the way you wrote the export code. It's messy.
I haven't had much time lately, and after finding out Antorus isn't released until 28/11 I haven't been in a rush to finish 2.7. Hopefully I'll get some more time for it the coming days, and I still plan to release it at least a week before.
I still believe we should push some of the remaining "major" changes to 2.8 to avoid making an overwhelming update, and I pretty much agree with your list.
Feel free to suggest changes to the export, but I frankly don't see much room for improvement.
I think #52 (Allow addon to work without ml) should be in v2.7 and that PR is really short and should be very easy to review.
I personally do want to get v2.8 released before Antorus, if v2.7 can be released before next reset
It's not really about the size nor review - my main concern is burying users with new features and changes - followed by how a change/addition might affect other parts of the addon (both now and in the future). I trust you've tested your work, so I'm not that concerned about the latter, although I still like to review everything.
I have been considering just shipping everything you've made, since it's ready, and to pack all major ui changes in one update.
Encountered a rare error, happened when I doubleclick the title bar of voting frame. I believe I have reported this to you before, got this again randomly just now.
2x ...s\RCLootCouncil\Libs\LibWindow-1.1\LibWindow-1.1-8.lua:128: attempt to perform arithmetic on a nil value
...s\RCLootCouncil\Libs\LibWindow-1.1\LibWindow-1.1-8.lua:128: in function `SavePosition'
RCLootCouncil\core.lua:2009: in function <RCLootCouncil\core.lua:2006>
The line reports bug is the following:
if frame.minimized then frame:Maximize() else frame:Minimize() end
I think 29b346d will fix it - I forgot to add that after your last error.
I'd still like your feedback on whether to add everything to 2.7 or not.
I agree to put everything in v2.7, but we would need more beta version before release it, at least I need to test the release candidate version in the real raid.
No time to make another version before Antorus.
-- Check if council is received
if not tContains(self.council, self.masterLooter) then
self:Debug("Received loot table without ML in the council", sender)
I know this is a cleaner code to check this, but I don't like to do raw comparison to the name, especially when wrong capitalization locks the user in an infinite loop.
#82
23570c4
I think note on the response button enables double check before sending the note, which is good feature to have.
ecc69f1
I don't think disable the rightclick menu when the item has been awarded is a good idea. It happens frequently that the person who received the loot gives up the loot. We'd better offer support to reaward instead.
I also want to add an award later button in the right click menu, and I think we should add an announcement for award later.
v2.7
While I don't like doing such a big release, I think it's better than having two big (ish) releases within two weeks of each other. I'm starting to merge the remaining PRs now. Adding support for reawarding an already awarded item should probably be delayed for another release, as I imagine it requiring some structural changes. Also do note I was only talking about awarding again (not the same) in my previous comments, just in case that wasn't clear.
Notes on buttons
I don't agree - in fact I found it very annoying having it sticking around the buttons.
Award later
Could definitely be done - it would be as simple as adding a new entry to the menu and award it will nil winner. I'm just not sure I see a real need for it? Wouldn't people interested in awarding items later just do it instead of starting a session?
Allow to change award in voting frame is very convenient though, and I have almost done it.
There are already lots of structure changes in award section, I don't mind to add a little more.
Change award immediately after award is given happens frequently in the real raid, I do want to implement that. No additional structure changes are required.
I found a bug and I need to make a decision.
RCLootCouncilML:AddUserItem(item) does not add the item info self.lootInBags, so I have to differ the bagged items that are added by RCLootCouncilML:AddUserItem(item) and award later.
My current idea is that :AddUserItem uses self:AddItem(item, true), bagged is boolean, while :SessionFromBags uses self:AddItem(item, i), bagged is a number to differ them
And I want to add two simple commands:
/rc addAward: Add the item into ML.lootInBags for award later
/rc removeAward: Remove the item from ML.lootInBags for award later.
/rc add: This command name is confusing.
I am working on PR #78 today. I didn't do much on other PRs
I ment it would require alot of changes to the already changed structure - but feel free to do it.
There's no reason to differentiate them?
lootInBags are unawarded items that should be used to start a session at some point.
awardedInBags are items that have been through a session and awarded, that just needs to be given out.
If a user wants to remind himself about items, he could do a "/rc add [item]" and click "Award Later" to have the item added to lootInBags thus having rc remember it.
Deleting items from lootInBags should be done with the delete button in the session frame (when doing "/rc start") - there's no need for commands for that imo.
You misunderstand me.
I mean '/rc add' does not add items into ML.lootInBags, which causes problem for the following code.
-- ml_core.lua, master branch
if self.lootTable[session].bagged then "/rc add" set bagged as true
-- Add to the list of awarded items in MLs bags, and remove it from lootInBags
tinsert(self.awardedInBags, {link = self.lootTable[session].link, winner = winner})
tremove(self.lootInBags, session) -- But "/rc add" don't add the item to ML.lootInBags, but we remove them here???
awarded = true
No - it should be added to awardedInBags when awarded - that doesn't mean it needs to exist in lootInBags. The tremove won't even produce an error. lootInBags is only for items we've looted from a boss that should be awarded at a later point.
See my edited comment.
BTW, tremove does not work, even if "/rc add" add the item into lootInBags, because awards in other session changes the index of lootInBags.
You can check my attempt to fix this in #78
I see what you mean now. It's true awarding an item (say session 1) from AddUserItem() would remove any existing session 1 from the lootInBags. Should be relatively easy to fix though.
Somewhat, I'm having some issues with it - it doesn't seem to work. Haven't found out why yet.
ok. I'm testing the current v2.7 branch now.
Fixed. #85
/rc add: This command name is confusing.
Do you have anything better in mind? It's definitely better than "addItem" as short commands == good commands.
I don't bother this problem now.
Can you merge #71?
I need to resolve the conflicts in other PRs before you can merge.
Merged #71.
Btw, I agree with your comments in #52 , I just didn't work on it yet.
I moved self:ScheduleTimer("Timer", 15, "MLdb_check") into NewMLCheck, but it should be better to let it stays in GetML
I'm working on it now, so I'll just add them in
Alright - isn't #73 ready though?
#73 is ready
7addbd3
This is not a good solution. You'd better force all columns to sort by session number, effectively disable sorting.
Making a PR right now
I'd prefer not to merge the different PRs together and just solve the merge conflicts when they appear, unless you really need it. It just makes for a better commit log.
ok. Those PRs will be separate. Working on #53, which should be done torrmow.
See you tomorrow
Still working on it, almost done but haven't done any tests yet.
I think lots of localization needs to be adjusted or added to better explain the new features.
I agree. I'll take a look at it when going through the changes.
L["chat_commands"] = [=[
- config - Open the options interface
- council - Open the council interface
- history - Open the Loot History (alt. 'h' or 'his')
- version - Open the Version Checker (alt. 'v' or 'ver')
- open - Open the voting frame
- reset - Resets the addon's frames' positions
- test (#) - Emulate a loot session with # items, 1 if omitted
- whisper - Displays help to whisper commands
- add [item]- Add an item to the session frame
- award - Start a session with items looted to your inventory
- winners - Display the winners of awarded items looted to your inventory
- sync - Open the synchronizer view
]=]
This should really be splitted into multipline locale entries
Do you know any API that get the information of "You may trade this item with players that were also eligible to loot this item for the next %s." in the tooltip?
Adding the item to the trade windows fail if two instances of the same item exists in the inventory, and one is completed binded and the other is within 2-hour window
Never mind, just going to do a tooltip parsing
We'd better give a new name to "master looter" in the addon's locale, otherwise there is a lot of confusion, especially in the usage options.
What should be the name? I think "Group admin" is a good name.
I think we should have a tutorial mode which prints extra text.
LibWindow error happens again today.
4x ...s\RCLootCouncil\Libs\LibWindow-1.1\LibWindow-1.1-8.lua:128: attempt to perform arithmetic on a nil value
...s\RCLootCouncil\Libs\LibWindow-1.1\LibWindow-1.1-8.lua:128: in function `SavePosition'
RCLootCouncil\core.lua:2100: in function <RCLootCouncil\core.lua:2097>
I just haven't had the time to change the chat commands locales. I'd like to to release 2.7 at the next reset if all goes well with the beta versions. I'll put in the locale changes, but I probably won't use them unless they're translated at the next reset.
As for the tradeability of items - it seems weird if there isn't any API to check for this, but I haven't been able to find any.
We're definitely not going to call it "Group Admin". You have to remember the majority of the users actually use ML and a sudden name change would alienate them. The addon is ment to be used with ML enabled - all the other features are just niche, that's only added to support the occasional user that wants to use it without ML. We've already done more than enough for them, and no matter what we do, it'll never be as useful as when using ML.
Imo we've already exceeded the point where the features far extends the needs of the average user.
I don't like the idea of lots of prints for a tutorial. I once considered using Blizzard's tutorial overlay method, but I deemed it to big an addition for too little gain. The basics should be self explanatory (which, imo, it is). I did consider writing a guide/tutorial for the more advanced use cases, but noone have ever really asked about anything that couldn't be explained in an Faq fasion. With the amount of changes in 2.7 it is probably more needed than ever.
Then probably some mention of master looter settings in RCLootCouncil can also apply to the group leader when the loot method is not master is enough.
Tutorial:
I believe most people using this addon aren't aware of the features: /rc open, /rc sync, /rc add, /rc winners, filter, auto award. nice features, but not everyone knows.
I hope these PRs can be merged if everything is ok. I did change some locale, but probably I didn't make it much better. If possible, we should have beta3 and beta4 before release.
I expect people to read the description on Curse. Some things might lack a proper explaination though, for which a tutorial would probably be nice. If people doesn't know about a feature, they're probably not interested in anything but the basics. Just typing "/rc" will also introduce the user to those features. I don't think an in game tutorial is worth it.
I'll do a beta3 today. #88 is on hold for now - I still haven't decided how I like it.
Well, I just feel have to do '/rc add' manually when the loot method is not master feels incomplete, so I create that feature. It's common that guild don't use master loot, or basically cant because the group is not guild group. I think it's a good feature to have. It's not people won't use the feature, it's just not existing before.
I really don't care much for people intentionally using PL - that's their decision, and they most likely have their reasons to do it. Those forced to use PL is another story though. If Blizzard hadn't made the whole guild ML thing, then I probably never would consider adding support for anything but ML.
The feature of this PR will be changed. Instead of group leader sees everyone's loot in the session frame, raid member can link the item to the raid chat use a command in chat, in order to add the item to the session frame of the ML.
TLDR, This PR will alter "/rc add" so it non-ML can also use it and send the command to ML, depend on the setting of ML.
How is this? I will implement tomorrow if looks good for you
Seems curseforge cant parse Beta3 changelog
Did a master loot test, didn't find major bug. I just didn't test award later.
The current manual roll feature is not synchronized across council and is not convenient, I will change that.
Rarely, the loot frame becomes empty:
There is error before I add nil check, but no error after I add nil check for libwindow:SavePostion
Any possible reason why this happens?
Btw, voting frame also had become empty before.
It seems it can only happen when I alt+tab into that WOW window.
Probably just put this into the known issues on curse page. reload fixes it.
Happened second time today
Alright, I have identified the issue.
If the ML reloads and resend the lootTable when non-ML's lootFrame isn't closed, the next time when the non-ML's lootFrame is shown, lootFrame is bugged.
See #93
We'd better print sth on login and ask the user to read the changelog
PL
I'd probably prefer just to have it automatic - that's easier than teaching everyone in the raid how to do it, but I suppose there's pro's and cons for both.
Curse
Curse have once again changed their design without ensuring stuff's actually working. I'm really starting to hate them. I'll make a post on their forums.
Empty loot frame
This didn't happen before 2.7, as I tested that specific bug extensively last time I changed the loot frame.
Print on login
Yeah, probably.
PL
I can make it so raid members gets a popup when he gets a tradable item, but it could be very annoying. I don't have an idea how to properly automatic but not annoying.
I think that allowing automatic loot handling from group leader is the only way to do that. I still would like to let the RC to have automatic loot handling feature when there is no ML.
I still want to have fully automatically loot handling feature when the loot method is not master.
After merging #88 and #94, whose feature is very related to master loot, loot handling in non-master loot can be very easily added.
The offer to this feature can affect how guilds play this game.
#88 and #94 have some conflicts between them, you can only merge one of them right now.
The other reason is I am against to put auto PL loot handling in another module is that it is not worth the effort to put it into another module. A lot more code is needed to ensure combatibility between modules and it is harder to maintain the module. And this feature involves the core feature of loot handling, which is harder to offer compatibility.
Can you do a module up-to-date check in "verTest" and add tVersion in GetInstalledModulesFormattedData()? I will drop my code about verTest in RC-EPGP if you do.
I am using the variable testVersion instead of tVersion in the current version of RC-EPGP. I am changing this.
There is a bug after the profile of the addon is changed:
Error in Master Looter-> Buttons and Responses
Similar error in Extra Utilities.
12x RCLootCouncil\Modules\options-Options.lua:1364: attempt to compare number with nil
RCLootCouncil\Modules\options-Options.lua:1364: in function `member'
...nfig-3.0\AceConfigDialog-3.0\AceConfigDialog-3.0-64.lua:249: in function <...nfig-3.0\AceConfigDialog-3.0\AceConfigDialog-3.0.lua:197>
Simpliest and the most secure way to fix it will be doing a reload automatically when profile is changed.
I am finishing the new version of RCLootCouncil-EPGP.
With the Antorus coming in few days, I will merge all my recent PRs into one PR tomorrow, if you are still busy.
I'm not sure about adding module version checking to the verTest command. For that to be really useful it needs to send even more data (i.e. table of modules instead of a simple string) than it already does, which is already too much for a simple version check. I might change that at some point.
Do not merge the PRs. Except for potential merge conflicts, merging them does not make anything easier. The plan was to release 2.7 wednesday, and I've only been waiting for you to get epgp updated, and changes according to my comments on some of the PRs. If all goes well, I'll release it today.
I'll release 2.7 at around 8:00 GMT+1 tomorrow (around 12hours from now). I think it's fair you get one more say before release since you made most of the changes.
EPGP needs some more time to finish, I really want to add remaining PRs to RC.
Making a PR to verTest now, it is not hard to do it by parsing the string.
Probably need to delay the release by 24h.
EPGP release can be a little bit slower than RC v2.7 though, I think at this point, as long as I release the new version of EPGP before reset, it's fine.
See #99