Add finalAlert (new Addon) #2191
Conversation
| @@ -0,0 +1,303 @@ | |||
| Copyright © 2022, Godchain | |||
There was a problem hiding this comment.
The entire copyright notice is not wrapped in comments. This way, the addon won't load. Wrap it in --[[ and ]].
addons/finalAlert/finalAlert.lua
Outdated
| * Redistributions in binary form must reproduce the above copyright | ||
| notice, this list of conditions and the following disclaimer in the | ||
| documentation and/or other materials provided with the distribution. | ||
| * Neither the name of <addon name> nor the |
|
|
||
| ## Commands: | ||
|
|
||
| ### Test window |
There was a problem hiding this comment.
I'm honestly not sure if this command should be even present in the finished addon, feel even less enthused about it being documented. But up to you. It seems like a debug command and those should not be user facing imo.
| -- IDs | ||
| weapon_skill_category = 7 | ||
| magic_category = 8 | ||
| interrupt_id = 28787 |
There was a problem hiding this comment.
How was this determined? If this is a zone resource, it might be different for every zone, and subject to changes after updates.
| defaults.background_size = "regular" | ||
| defaults.emphasize = S {} | ||
| defaults.trigger_duration = 3 | ||
| defaults.sounds = "on" |
There was a problem hiding this comment.
I would use boolean values instead of on/off, and even rather than binary enum values, like for the background_size above. I'd reword it to large with true/false as possible values or small/compact, with the same (but reversed) values. Users are notoriously clumsy with handling a single set of allowed values, you'll not only get users who misspell the allowed values, but also users who will argue about case insensitivity. The boolean approach saves you from a bunch of possible issues.
| background_emphasize = "background_emphasize" | ||
|
|
||
| -- Timing | ||
| showing = false |
There was a problem hiding this comment.
This variable can be removed entirely and be replaced with caption:visible(). It's a very cheap function call.
| end | ||
| elseif act.category == magic_category and act.actor_id == target then | ||
| local spell_name = | ||
| res.spells[act.targets[1].actions[1].param] and res.spells[act.targets[1].actions[1].param].name or |
There was a problem hiding this comment.
It bothers me a bit that this is broken up into three lines in the WS block, but only two lines in the magic block :\ I'm starting to get the feeling you're using a fixed width and wrap lines exceeding it, in which case I'd vote to just leave them be. Fixed-width line wraps are never a good idea, despite what the Python style guide says :|
| create_backgrounds(settings.x_position - 250, settings.y_position) | ||
| end | ||
|
|
||
| function create_backgrounds(x, y) |
There was a problem hiding this comment.
This function uses the windower.prim.* API heavily. I'd recommend looking into the images library, which is essentially to windower.prim.* what the texts library is to windower.text.*. Just abstracts away some of the boilerplate.
| function show_caption(text, type) | ||
| local event_type | ||
|
|
||
| hide_caption() |
There was a problem hiding this comment.
Feels weird that you hide it only to show it again. If this is about hiding the currently active box, I'd either maintain which box is currently active in its own variable, or extract the box-hiding code into a new function and call that from both here and hide_caption.
| caption:text(text) | ||
| caption:show() | ||
|
|
||
| if (type == "ws") then |
There was a problem hiding this comment.
No need for the parantheses here, as well as in the following if/elseif sections.
finalAlert is an addon that displays FF7-style toast windows for enemy WS/magic. It can display whether they're interrupted, and emphasize skills by name. I just learned it's become useful to many people, so I thought I'd check it into the Windower codebase. Screenshots below. First PR here - hopefully everything checks out. :)