earlSt1 / vtt-compendium-folders

Collapsable folders in the compendium directory and folder structures inside compendiums for FoundryVTT
21 stars 18 forks source link

Folder Path update for Temp Entities code has been removed #60

Closed esb closed 3 years ago

esb commented 3 years ago

I've just updated from 2.1.8 to 2.2.1 and it appears that some functionality has been removed.

I'm currently building compendiums dynamically with a nested folder structure, and when these are loaded in 2.1.8 the cf.folderPath is built automatically for all the temp entities in the updateFolderPathForTempEntity method.

This code has now been removed and now breaks things that were running on 2.1.8. Is there any reason for this?

earlSt1 commented 3 years ago

Hi, i removed a bunch of old migration code since it only existed to migrate older compendium folders versions to the newer ones, and it had been a few months.

The export folder structure functionality worked fine when I last checked, so I'm slightly confused. Are you using the compendium folders buttons or are you creating folder structures programmatically?

esb commented 3 years ago

I am creating folder structures dynamically via code that runs at startup. The entries are built from external data, with the folders being created as they are needed. When the compendium is loaded, the code used to build all the missing cf.folderPath entries. This code was deleted after line 2760 in the current version.

earlSt1 commented 3 years ago

If i'm honest, i wasn't planning on reintroducing that method as this sounds like a edge case, that was solved accidentally by my migration code changes.

The folderPath parameter works like an ordered array of folderIds, basically defining what the parent folders are. The end of the list is the folderId of the immediate parent folder. You could probably populate this with the correct information if you're creating folders, using the old method as a template:

https://github.com/earlSt1/vtt-compendium-folders/blob/25eba6e95796c1efadd4ebad81dae6da29052572/compendium-folders.js#L2538

This method takes a tempEntity and returns the updated version which you can then use pack.updateEntity(e) to update.

The next changes that are planned for this module will be focusing on the Folder In Compendium features, making them more accessible and easily changable. Until then I'm happy to help get your code working if you need any.

esb commented 3 years ago

The problem really is that there's no real defined API interface to build compendiums in the correct format. Sure, I've reverse engineered the basic structure to be able to build them successfully in 2.1.8, but it would be nicer if there was a proper interface to do this without exposing the internals of the package. Pushing out the responsibility for building another part of the internal structure in the form of the folderPath seems to me to be making this situation even worse.

Of course, it's your code and you can decide to make it do whatever you want. If it's going to be a pain for me, then I guess I'll just need to look at other options.

earlSt1 commented 3 years ago

What I was planning on doing was converting the folders to folder objects (similar to what i've done in the sidebar directory in 2.2.0), then provide helper functions to create/edit/delete them when necessary. I haven't started the planning stage for that yet, so the next few releases will be small fixes for the current release.

I could see about publishing a prerelease/private version or something, which has the updateFolderPath functionality included, that way your workflow wont be interrupted while i work on the helper functions.

earlSt1 commented 3 years ago

Can you try testing using this manifest? (uninstall your current compendium folders version) https://raw.githubusercontent.com/earlSt1/vtt-compendium-folders/master/test/module.json

esb commented 3 years ago

Sure, no problem.

There's another more general problem that I've been thinking about. Foundry doesn't currently offer much in the way of compendium support. For example, the index returned consists simply of the entry name, and there's no ability to include secondary information that could be used in searches or even to provide additional display information. I'm thinking of showing the source book for items in the index list, for example.

Also, the underlying Javascript database supports filtered queries, but this isn't really exported to the Foundry API, so there are limits on what you can search for.

It's uncertain when this will be improved, but in the meantime, I was wondering if there's a different way of approaching this by creating a special "INDEX" meta entry that contains the folder structures, plus additional index information for the entries.

I'm working towards building a way of accessing large amounts of reference data for a game system, and the current Foundry support is a little bit limiting.

esb commented 3 years ago

There's a small problem with the update.

After line 2791 add back

let updateData = [];

That stops the crash, but there's still something missing as all the entries end up at the top, with all the folders at the bottom. I'll have a play around with the code later today and see if I can work out what's missing.

earlSt1 commented 3 years ago

Sorry, just finished my game session. I'll push an update to the code soon adding back that bit.

esb commented 3 years ago

Worked out what's needed....

Add back this after line 2818

let allFolderIds = contents.filter(e => e.data.flags != null 
    && e.data.flags.cf != null
    && e.data.flags.cf.id != null 
    && e.name === TEMP_ENTITY_NAME).map(e => e.data.flags.cf.id)

Then restore

if (entry.data.flags.cf.id != null
    && entry.name != TEMP_ENTITY_NAME
    && !allFolderIds.includes(entry.data.flags.cf.id)){
    updateData.push(removeOrUpdateFolderIdForEntity(entry,contents));
    }

and restore the removeOrUpdateFolderForEntity method

earlSt1 commented 3 years ago

Ahh yep that makes sense from what you describe. Non-temp entities have a data.flags.cf.id which determine what folder they belong to.

earlSt1 commented 3 years ago

Just reintroduced those missing methods into the testing version. I've decided I'll leave those 2 methods in there until i work out a better solution for managing folders in compendiums.

esb commented 3 years ago

Great. Thanks, that helps a lot.

earlSt1 commented 3 years ago

Now included in v2.2.2, closing for now

esb commented 3 years ago

The updates for v2.2.2 have one small bug....

At line 2834, we have code that never gets called.

} else if (entry.data.flags.cf.id != null
           && entry.name != TEMP_ENTITY_NAME
           && !allFolderIds.includes(entry.data.flags.cf.id)){
           updateData.push(removeOrUpdateFolderIdForEntity(entry,contents));

This block is wrapped in a test at 2829

if (folderId != null){

The problem is that this block of code only needs to be run if folderId is NULL, but this has been prevented by the test at 2829.

Lines 2834 to 2837 need to be deleted and the following block inserted after 2828

if (entry.data.flags.cf.id != null
    && entry.name != TEMP_ENTITY_NAME
    && !allFolderIds.includes(entry.data.flags.cf.id)){
    updateData.push(removeOrUpdateFolderIdForEntity(entry,contents));
}
earlSt1 commented 3 years ago

Ah yep, i see what you're saying, missed that logic when reintroducing the migration code. I'll jump on this tomorrow and let you know once it's in the testing version.

earlSt1 commented 3 years ago

Should be sorted in v2.2.3