evil-morfar / RCLootCouncil2

RCLootCouncil - addon for World of Warcraft
https://rclootcouncil.com
GNU Lesser General Public License v3.0
19 stars 29 forks source link

Coding structure of V3 #165

Closed SafeteeWoW closed 3 years ago

SafeteeWoW commented 6 years ago

There are lots of coding structure problems in v2.x

  1. File is too large
  2. Strange module management.
  3. lots of redundant code for GUI
  4. No UnitTest framework.
  5. etc

I have signed up TradeSkillMaster4 Beta and read its blog, trying to figure out the solution. See also https://blog.tradeskillmaster.com/classes/

SafeteeWoW commented 6 years ago

I want to manage v3 code in the following ways. Each item I have mentioned should be in a separate file.

  1. API: Provide convenient interface to the modules the use. API should not provide real functionalities to the users.
    1. Item: Provide apis to cache and query item info.
    2. PlayerInfo: Provide apis to cache and query player info.
    3. Loot: Provide apis to cache and query loot info.
    4. Communication: Interface to send and receive addon messages.
    5. Serializer: Serialize lua table better than AceSerializer
  2. Widgets: Small graphic frame that can be easily reused by modules.
    1. ItemIcon:
    2. ItemName:
    3. etc
  3. Modules: Provide real GUIs to the users.
    1. VotingFrame
    2. LootFrame
    3. SessionFrame
    4. OptionFrame
    5. SyncFrame
    6. VersionCheck
    7. LootHistory
    8. TradeFrame (planned)

I am wondering how should I implement an API file. There are several options:

  1. ItemAPI=LibStub:NewLibrary: The issue is that this exports the internal api to the outside world, which I don't like.
  2. ItemAPI=AceAddon;NewModule: This is a standard way, but I don't like them to be declared in the same way as MODULE.
  3. _G.RCAPI.ItemAPI = {}. This is the way I prefer the most currently.
evil-morfar commented 6 years ago
  1. LibStub:NewLibrary should only be used for separate libraries that can be shared between addons - RCSerializer might be a candidate for this, if you intent to make it public - otherwise it shouldn't be used.
  2. AceAddon:NewModule should only be used when something needs the inherited AceAddon functions (modules are in fact treated as separate addon objects, with a reference to the core). I used this for the "modules" to separate them cleanly and to give them access to the db object. I don't think it's a good idea to use it for API/utility functions.
  3. Separate tables. This is the way to go. How exactly it should be done is up for debate, but I'd probably prefer something simple like RCLootCouncil.Item, or maybe RCLootCouncil.API.Item to really separate it, but I'm not sure that extra table really is needed.

Next to consider is objects. When you're talking about API's, I'm thinking somewhat simple functions. For example, consider handling an item, with API I understand something like:

local itemName = RCLootCouncil.Item:GetItemName(itemLink)
local ilvl = RCLootCouncil.Item:GetItemLevel(itemLink)

I'd very much like a more object oriented approach, so that the following is possible:

local item = RCLootCouncil.Item:GetItem(itemLink)
-- Either this:
local itemName = item.name
-- or this:
local itemName = item:GetName()

local ilvl = item.ilvl or item:GetItemLevel()

Furthermore I think session should be added to your list of APIs, but otherwise I agree with you.

SafeteeWoW commented 6 years ago

i am working on the pure lua deflate compression,( google Rfc1951 for spec ). Current result is twice slower than libcompress, but 30% smaller.(not fullt tuned yet ) I will first finalize serializer and compressor, to be released as standalone lib, as the design of those must be frozen at the first release of v3 for backward compatibility reasons.

evil-morfar commented 6 years ago

Another thing I'd to change is the current award system. The amount of functions handling this has become too huge and segregated: rightClickMenu > PopupYes > ML:Award > (several paths, usually ML:CanGiveLoot) > ML:GiveLoot > Callback1 > awardSucces > Callback2 (potentionally another callback) > ML:TrackAndLogLoot > registerAndAnnounce > HasAllItemsBeenAwarded > potentially more i.e. at least 10 different functions, most of them with several fail states.

The biggest issue is, I recently discovered that doing "Award Later" on the last item in a session (probably not only the last) wouldn't end the session nor register anything useful that could end the session. I spend almost an hour trying to track through every step to identify where the fault was, and where it could be implemented (basically there's a missing variable or a complicated check of ml.lootTable.baggedEntry/baggedInSession). As per my roadmap in #116, this is not critical enough to warrant a change until after 3.0 though.

SafeteeWoW commented 6 years ago

Hope you can first work on new communication protocol, which transmit as less data as possible. For compatibility reasons, the protocol needs to be finalized before v3, which needs to be done first.l