Skip to content

Add a filter based on effect#17

Merged
exochron merged 27 commits intoexochron:2.6from
chodyo:master
Nov 29, 2020
Merged

Add a filter based on effect#17
exochron merged 27 commits intoexochron:2.6from
chodyo:master

Conversation

@chodyo
Copy link
Copy Markdown

@chodyo chodyo commented Nov 18, 2020

Hi there! I'm not anywhere near done classifying toys but I wanted to bring this to the table early to get your thoughts.

Ever since the toybox was introduced I've found it to be clunky to use unless you know exactly what you're looking for. This change to your addon would add a filter to the toy box that allows players to filter the toybox based on what the toy does.

This change is something I've always wanted, so if you don't want it in your addon, I plan on maintaining this as a fork for myself.

All feedback is welcome!

@exochron
Copy link
Copy Markdown
Owner

Hi chodyo, thank you for your pull. You're actually the first code contributor in any of my projects. :)

This feature has been requested for a loong time (https://www.curseforge.com/wow/addons/toy-box-enhanced/issues/3). I was planning to add it in some time. I just didn't want to manually maintain another huge list of ids. So I was aiming for an automatic aproach (or database generator). But I haven't found any nice source to parse from, yet. Well, and so time went on and I kinda forgot about it. ._.
Anyway, the manual approach works definitely as well. So this pull is definetly going in after some fine tuning.

  • You're using "use" as key/name for this filter. I'd rather go with "effect" or something similar. Just to have a bit more meaningful name.
  • We can also add another layer to the filter menu. So there's a lot of space for potential categories. I'm thinking about something like "Transportation" with "Movement", "Heathstone" and "Teleport" as subcategories. So you can go wild with as much categories as you want. I can handle the filter menu later. :)
  • Somebody has already started with some categories in Issue 3 (link above). Maybe you can find some inspiration there.
  • There should exist some noisy or music related toys. Some categories for those might be nice. (Drums, whistles, juke boxes and so on)

@chodyo
Copy link
Copy Markdown
Author

chodyo commented Nov 21, 2020

Wow, I combed through what seems like every issue except for that one looking for this feature request! I can't believe I missed that haha.

I definitely feel you on not maintaining another list - it's incredibly time consuming and easy to misclassify.

These suggestions are great! Before I get going with more classification I'll study your comment and the feature request to make sure the approach I take considers these suggestions. Nested lists sound like a great improvement since the flat list is getting quite long.

One thing I'd like to ask from you since you're a seasoned addon developer: is there any way we can modify Database.lua from in-game? It would make toy classification significantly easier and faster if we could set toy categories directly in game with a few mouse clicks instead of alt+tabbing and moving text around. Plus I think that might be a nice addition to the release with the feature, to allow users to move toys to the effect categories they prefer.

@exochron
Copy link
Copy Markdown
Owner

There is no secret editor mode for this. ^^

@chodyo chodyo changed the title Add a filter based on use Add a filter based on effect Nov 22, 2020
@exochron
Copy link
Copy Markdown
Owner

Somebody just uploaded his version: https://www.curseforge.com/wow/addons/toy-box-enhanced/issues/22
It looks nearly complete. I would go with that as first version of categorisation. From there we can narrow down some more.

@chodyo
Copy link
Copy Markdown
Author

chodyo commented Nov 25, 2020

Great! Thank you for letting me know, I wouldn't have noticed. This will help immensely!

@chodyo chodyo marked this pull request as ready for review November 27, 2020 18:02
@chodyo
Copy link
Copy Markdown
Author

chodyo commented Nov 27, 2020

Alright, just marked this ready for review. Some notes:

  • The code I wrote is only compatible for one nested filter layer. If we want to do more layers I'd need to make some parts recursive, but I was assuming we wouldn't want to create too many layers.
  • Using the same name for filters is not supported, so I renamed profession effect categories slightly. This also got me when I created multiple Other categories so those got renamed too to be just as inclusive but more specific.
  • I forked a spreadsheet from issue 22 on curseforge to keep everything organized. This helped massively - I was able to validate itemIDs programmatically with their names to ensure all itemIDs are correct.
    • I'd like to keep it in sync with the addon as much as possible. Categorizing everything directly in the code like I was doing before was slow and tedious and prone to mistakes. I'd be happy to add you as an editor to it, or you can just fork it yourself if you'd like to keep your own separate copy.
  • If you turn off all effect filters, it will show everything that's not categorized by effect. This is very handy for checking if we didn't categorize any toys - I found that I had missed like 30 toys originally.
    • There is still just over 1 page of uncategorized toys. A lot of these are shadowlands toys that don't have much info on Wowhead yet.

I feel like this PR is pretty much complete other than the few uncategorized toys. Still open to suggestions of course.

@exochron exochron changed the base branch from master to 2.6 November 28, 2020 11:28
@exochron exochron merged commit 00de8a3 into exochron:2.6 Nov 29, 2020
@exochron
Copy link
Copy Markdown
Owner

Thank you so much for your work! 👍

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.

2 participants