New feature/add tag filters

kanielrkirby commented 7 months ago

Added Documentation bit for Filtering
Override the list.Filter and TUIItem.FilterValue
Make enter work as expected when working with list filter mode
Added an extensible Filterer type to hold information related to filtering
Added method Filterer.GetItem to extract the inferred TUIItem from what's returned by TUIItem.FilterValue()
Added Filterer.ExtractFiltersFor method to pull tags that start with tagname: from search term
Added Filterer.FilterBy method to actually pull ranks out based on filterValues and targetFilterValues passed in

If you have any criticism or concerns, please let me know, I'm happy to rework this as needed. I tried to make it easy to extend this for other filter values, and test based on my personal dataset, but I definitely didn't test everything possible.

I imagine if we want to, we could easily extend / move functionality for the boolean types like the read and new favourite attributes, just by tweaking the function to be something similar to FilterByBool or something.

guyfedwards commented 7 months ago

I imagine if we want to, we could easily extend / move functionality for the boolean types like the read and new favourite attributes, just by tweaking the function to be something similar to FilterByBool or something.

Yea definitely, I was thinking about some improved solutions when adding favourites. Utilising this filtering method seems a lot more natural though so something to refactor down the line.

guyfedwards commented 7 months ago

I've just merged https://github.com/guyfedwards/nom/pull/62 as well, shouldn't conflict but worth rebasing as I've touched the README and TUI.go, let me know if any issues.

kanielrkirby commented 7 months ago

Looks great, thanks @kanielrkirby. Couple of minor points, otherwise looks/works great.

currently q goes back to the list when you are in an article, and playing with this a bit I feel that it would be good for q to work like esc in the filtering view. So when IsFiltered(), making q just clear the filter, rather than quiting the program. I think you can do this with a custom keymap in the model init, or adding another case to the keypress switch.

After filtering and selecting an article, then pressing q/esc you are taken to an empty list when this should be showing the filtered list.

Not a problem, I'll see about implementing that tomorrow when I get some time, shouldn't be a huge issue at all.

kanielrkirby commented 7 months ago

I imagine if we want to, we could easily extend / move functionality for the boolean types like the read and new favourite attributes, just by tweaking the function to be something similar to FilterByBool or something.

Yea definitely, I was thinking about some improved solutions when adding favourites. Utilising this filtering method seems a lot more natural though so something to refactor down the line.

Something else I was wanting to potentially bring up in relation to this is the keybindings. Have you considered using the bubbles/key for this, as the keybind situation is growing a little bit more?

guyfedwards commented 7 months ago

Something else I was wanting to potentially bring up in relation to this is the keybindings. Have you considered using the bubbles/key for this, as the keybind situation is growing a little bit more?

For sure. I haven't really looked at the keybinds stuff properly . Could you add an issue with your thoughts as I agree this could do with some love.

kanielrkirby commented 7 months ago

Looks great, thanks @kanielrkirby. Couple of minor points, otherwise looks/works great.

currently q goes back to the list when you are in an article, and playing with this a bit I feel that it would be good for q to work like esc in the filtering view. So when IsFiltered(), making q just clear the filter, rather than quiting the program. I think you can do this with a custom keymap in the model init, or adding another case to the keypress switch.

After filtering and selecting an article, then pressing q/esc you are taken to an empty list when this should be showing the filtered list.

Do you think "ctrl+c" should also do this?

kanielrkirby commented 7 months ago

Hmmm, on second thought, I think it would be good maybe to put these keybind-related concerns in a separate issue, if that's alright. Primarily, because there's quite a bit of boilerplate when doing custom keybinds in general, and this will likely be better in a separate file with all the keybinds when we decide to refactor that section (which I'm happy to start on after this). What do you think? @guyfedwards

guyfedwards commented 7 months ago

Do you think "ctrl+c" should also do this?

I don't think so as having that as interrupt feels aligned with a terminal program more than the vim-esque keybinds in nom.

Hmmm, on second thought, I think it would be good maybe to put these keybind-related concerns in a separate issue, if that's alright.

Yea that's fine with me

kanielrkirby commented 7 months ago

@guyfedwards I believe I've addressed the concerns, though please let me know if there's any problems.

guyfedwards commented 7 months ago

@kanielrkirby Just two cleanups left,

could you remove the 'usage' section from README in your changes, looks like a relic of the conflicts (sorry for that!)
and can you remove the nom binary from this, I've added it to gitignore in another PR

Then good to merge, thanks again

kanielrkirby commented 7 months ago

Oh no! Totally my bad.

kanielrkirby commented 7 months ago

I might have messed something up there, Git to me is like Regex is for you lol.

guyfedwards commented 7 months ago

Thanks for your work @kanielrkirby, will push a tagged release shortly.

kanielrkirby commented 7 months ago

Np, love to help out!

guyfedwards / nom

New feature/add tag filters #61