Skip to content
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

Added a new addon Cards to track job AFs and Omen cards #2402

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

Nalfey
Copy link

@Nalfey Nalfey commented Nov 12, 2024

Addon that tracks the AF cards you have and need for upgrades.
It looks for the AFs NQ, +1, +2, +3 that you have for a specific job and tells you how many cards you already have and need to augment them to +3.

Command:
//cards

Examples:

//cards GEO
//cards WHM

Capture
Capture2

@z16
Copy link
Member

z16 commented Nov 13, 2024

The reason it was suggested to make a new addon for this, is because findAll does a lot of things that are unnecessary for this. You should remove all the features of findAll that are not needed, and change the ones you do need for your purpose. You also need to stick to the rules for creating a new addon, which include handling the license properly, and not committing any files to the data sub directory. See this guide for more details on that.

@Nalfey
Copy link
Author

Nalfey commented Nov 13, 2024

Thanks for your concerns @z16 ,
It is already the case in the current code for this addon Cards. Please check the code or try the addon to see for yourself.
I've removed all of the findAll functions that look for items based on an input name or word.
I am only leveraging findAll for it's /data/ folder in case the user is already using findAll so that we don't duplicate unnecessary /data/charcter.lua files. Only if findAll is not used will the Cards/data/character.lua file be created.

@lili-ffxi
Copy link
Contributor

You might want to double check, because in this commit you are uploading a data\settings.xml file, and your cards.lua has all of findall functions in the code, even if they're inaccessible from the command. Furthermore, the way it is written, I'm pretty sure it's going to overwrite findall's own item data storage files!

@Nalfey
Copy link
Author

Nalfey commented Nov 13, 2024

@lili-ffxi

  • Cool cool, I've removed the search and string.query_escape functions to clean things up.

  • Yes I was uploading a data folder with the previous commit because it will get created regardless the first time a user loads the addon as it stores a settings file, it does not mean that this data folder it will get populated with any .lua file when the command is run. This will only happen if you do not have the findAll addon.
    I can remove that data folder from the commit if you'd rather not have it there, but it will get created for the user as soon as they load the addon.

  • Yes correct when running the //cards command it has to both access and update the findAll data files. If it doesn't do the update you'll end up in a situation where the you might have different items than the last time you ran a //findall command.

  • So it's either / or - if you want me to fully go away from findAll I can totally do it and it's actually much easier.
    It will always generate the data files for Cards in it's own data folder and never access or update the findall data file.

@lili-ffxi
Copy link
Contributor

  • Yes I was uploading a data folder with the previous commit because it will get created regardless the first time a user loads the addon as it stores a settings file, it does not mean that this data folder it will get populated with any .lua file when the command is run. This will only happen if you do not have the findAll addon.
    I can remove that data folder from the commit if you'd rather not have it there, but it will get created for the user as soon as they load the addon.

You did not only upload a folder, that folder contained a settings.xml file. As per the addon guidelines that Arcon mentioned, we do not upload any content of any data folder, otherwise at each update of the client the users will find their own data folders overwritten with the files contained in the repo.
The proper behavior is for the addon to use the config library, which manages addon settings, and will create the data folder and the settings.xml and other files, when the addon is first loaded (or in other ways, it's pretty versatile).

So yes, the data folder and any file in it absolutely must go in order for the PR to be viable at all.

  • Yes correct when running the //cards command it has to both access and update the findAll data files. If it doesn't do the update you'll end up in a situation where the you might have different items than the last time you ran a //findall command.

This is a horrible idea because it will clobber (conflict during read/write) the file and will eventually end up corrupting it, causing all sorts of issues. Findall is excellent at keeping its own internal storage updated in real time, and should stay the only thing that edits its own files. Not to mention, what will you do if findAll eventually changes its own format (unlikely, but not impossible), and your addon does not?

  • So it's either / or - if you want me to fully go away from findAll I can totally do it and it's actually much easier.
    It will always generate the data files for Cards in it's own data folder and never access or update the findall data file.

That is up to you, but my recommendation is to make your addon access the findall disk storage only if you want the addon to be able to search offline characters. For the character that is currently logged in, the best thing to do is to access the inventory directly through windower.ffxi.get_items() and all the api functions related to it.

Dupefind accesses findall's offline storage in order to be able to tell you if you have items scattered around alts and mules - that is an example of an usecase where using the offline storage is useful. If you're only examining the current character, there's absolutely no reason to do so - see https://github.com/lili-ffxi/FFXI-Addons/blob/master/collector/ for an example of another addon that only examines the inventory of the character currently logged in.

@Nalfey
Copy link
Author

Nalfey commented Nov 13, 2024

Thanks for taking the time to reply and explain everything @lili-ffxi

This is a horrible idea because it will clobber (conflict during read/write) the file and will eventually end up corrupting it, causing all sorts of issues. Findall is excellent at keeping its own internal storage updated in real time, and should stay the only thing that edits its own files. Not to mention, what will you do if findAll eventually changes its own format (unlikely, but not impossible), and your addon does not?

Yup I totally agree, it's kinda why I wanted to make Cards just as an extra command for findAll in the first place.
And not have to reference findAll from an other addon, I just didn't do it in an elegant way.

That is up to you, but my recommendation is to make your addon access the findall disk storage only if you want the addon to be able to search offline characters. For the character that is currently logged in, the best thing to do is to access the inventory directly through windower.ffxi.get_items() and all the api functions related to it.

I wanted to have the ability to also find job cards stored on mules for a future update ( I tend to send my card to mules to free up space), so having the same system as findAll was my preferred starting point.

I think I'd rather split away from findAll completely as it's causing more issues than anticipated, I'll do some tweaks to the code and do a new commit soon.

Thanks again !

@Nalfey
Copy link
Author

Nalfey commented Nov 16, 2024

@lili-ffxi
whenever you free up would you mind checking my latest code and commit please.

@lili-ffxi
Copy link
Contributor

Hello, life threw me a curveball so I don't really have the time to look at windower stuff lately. I'll be back at it in a few days (hopefully), in the meantime if other devs are willing to help you I'll have to pass the ball to them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants