-
Notifications
You must be signed in to change notification settings - Fork 430
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add finalAlert (new Addon) #2191
base: dev
Are you sure you want to change the base?
Conversation
Update roe.lua
Dev > Live Merge
Typo fix `clear_moogle` to `clear_moogles`
Added a comment instructing users not to add Pilgrim Moogles manually, as it won't work.
Version bump and Pilgrim Moogle removal.
Dev > Live
[Battlemod] Several updates and fixes.
Dev > Live
Update for incoming packet 0x075
Update craft.lua
I never logged in to test if this would load, but I did test it locally to see if the functions could pull the correct IDs from DAT files sent by a friend. (c) dos2unix, struct to lpack, read *a to read 2
fix flatten and extend function calls for non-meta tables
Now will properly swap for valid commands such as `/ma "Garuda"<me>` and `/ma "Elemental Siphon"<me>` Also will now properly handle (ignore) more valid forms of waits such as `<wait>` and `<wait 1.2>` and waits that are not preceded by a space `/ma garuda <me><wait 2.2>`
Adding Levin to Slips lib
Adding Levin to Slips lib
There are duplicates in the tables. That's a pain.
Update Slips libs for September FFXI update
Update slips.lua
Two new songs added in the September 2021 update. Names are not known yet.
Fix weird interaction with Gearswap
Update digger messages for 2022-04 version
…2204 Update boxdestroyer for the 2022-04 version
Adding the items from the April 2022 FFXI Update.
[Slips lib] April 2022 Update
Occasionally addon will try to resume crafting too quickly after interacting with support npc causing entire queue to be dumped with "you cannot craft at this time" errors. Added a short delay in the check_queue function after poke_npc() call.
Support bug fix
Update to properly handle more valid commands
FindAll -- Allow for start/end pattern matching
…packet-parsing GearSwap: Expand party packet parsing
Update setbgm.lua
Don't know why she was commented out in the first place
…cal-clock-drift Add Vana Time offset
Co-authored-by: RubenatorX <[email protected]>
…mestamp_fix [gearswap] Fix buff duration calculation in packet_parsing.lua
Added items to slips libs for the 10 May, 2022 FFXI Update.
[Slips lib] Update for May 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments, most of them are only suggestions. One thing I didn't mention at any one specific line, because it repeats throughout the addon, please use single quotes for all strings, to keep in line with our current style throughout the addon repo.
@@ -0,0 +1,303 @@ | |||
Copyright © 2022, Godchain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the addon name here :)
|
||
## Commands: | ||
|
||
### Test window |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. :)