Refactor Compendium Browser to use a Predicate for filtering entries#20492
Refactor Compendium Browser to use a Predicate for filtering entries#20492CarlosFdez merged 3 commits intofoundryvtt:v13-devfrom
Predicate for filtering entries#20492Conversation
fa2ad28 to
6c06c19
Compare
992c27b to
d462bab
Compare
d462bab to
26af6e3
Compare
429125f to
7c5c7f6
Compare
|
Fix it one more time and poke me on discord. Is this the smallest you can get it? Are there no refactors here you can section off? |
|
I may be able to do this without refactoring the options from objects to arrays. That should make the diff a bit smaller. I'll take another look over the weekend. |
811f50c to
c3ed93e
Compare
| } | ||
|
|
||
| options.add(`level:${system.level.value}`); | ||
| options.add(`rarity:${system.traits.rarity}`); |
There was a problem hiding this comment.
Most everything here is fine so far but this gives me pause. What is the purpose of using predicates if you end up having to add the roll options anyways?
There was a problem hiding this comment.
Using a predicate should make it easier to implement new filters without the need for custom logic. It already covers pretty much anything that a filter could need and especially exclusion is much easier.
And for adding roll options, I'm pretty sure a predicate without options wouldn't do anything? I might be misunderstanding what you mean.
There was a problem hiding this comment.
It looks like you're creating options, rather than using the item's existing options. So there's potential for desync or forgetting that an option has to be added for the predication to even work.
|
Maybe make the options an array? You might be able to cut down on a lot of lines of setup code if you use |
c3ed93e to
8fb9796
Compare
|
Updated with the suggestion. Options are now converted to a Set as the last step. They could technically stay arrays in the filter data but the Predicate would convert them anyways on each filter operation. The basic shape of the roll (or filter) options can't really be changed as the Predicate expects a colon as a separator for some things. The only thing I could think of would be to add a |
| } | ||
|
|
||
| options.push(`type:${actionData.type}`); | ||
| options.push(`category:${system.category}`); |
There was a problem hiding this comment.
Can you keep the simple one liner additions together? In other words move this up to the other options.push? It saves only a bit of whitespace if you do it throughout, but right now the main wart of the new style is the manual addition of options, so if the code can be kept tight, I think it'll be fine.
You could do this as well I suppose to save on whitespace
const options: string[] = [
`action-type:${system.actionType.value}`,
...system.traits.value.map((t: string) => `trait:${t.replace(/^hb_/, "")}`),
`type:${actionData.type}`,
`category:${system.category}`
];
Try to keep option initialization LOC tight for each object type if possible.
There was a problem hiding this comment.
Should be better now.
Don't worry about it. While it being confused for genuine roll options is a problem, I don't think making them different is going to help anyways, nor is it enough reason on its own to deviate for the sake of deviating. |
152d133 to
65df195
Compare
CarlosFdez
left a comment
There was a problem hiding this comment.
Code looks fine to me, but I plan to check for bugs personally.
I finally had some more time to work on this. Pulled out from #20282
This PR only includes the switch to predication and no other additions or UI changes. If this is approved I can start to pull out the other changes from the old PR.
Current filter:

Predicate filter:
