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

Add (not) between modifiers for number criterion #1559

Merged
merged 7 commits into from
Aug 12, 2021

Conversation

gitgiggety
Copy link
Contributor

@gitgiggety gitgiggety commented Jul 6, 2021

This adds support for between and not between filters for number and duration criterions.

The implemtation is a bit quirky on the following points:

  • value is splitted in three, exact, lower and upper, for respectively (not) equals, greater than and less than. In case of (not) between both will be used obviously. But this means that switching between these types "removes" the set value. So changing equals 10 to less than (or greater than) means another input is shown which doesn't have a value set, and the same applies when switching between less/greater than. This most likely will also mean that saved filters will be broken. I did think about converting value to a string so a range could be specified as X-Y but that would mean changing a lot more code, would accept more broken input (being any string which must be validated) and possibly still break saved filters.
  • All three fields are required and default to 0. Initially I made all three optional. But this would make them pointers server side which would require more trickery to get the value, require validation if the value was actually set, etc.
  • Because the current filters are greater than and less than, so being exclusive, and SQL between being inclusive I've "hacked" around this. In other words: setting the between criterion to 2 - 5 will only match values 3 and 4 (instead of 2, 3, 4 and 5 which a normal SQL between would do). And this obviously works the same for not between, 2 - 5 does match any value exact 3 and 4, so it does match 1, 2, 5, 6, ....

At the moment I'm marking this PR as WIP as I still want to do some refactoring on the client side. This being splitting the AddFilterDialog into multiple components for the different criterion types and making a factory method / ... which will create the correct component for that criterion. This because the HierarchicalCriterion already made it a bit of a mess, and adding a specific branch for numeric, and both numeric and duration having three inputs makes it a mess.

Fixes #1504
Fixes #1557

@gitgiggety gitgiggety force-pushed the int-range-criterion branch 2 times, most recently from 8271be9 to 1ee1b0f Compare August 8, 2021 18:23
@gitgiggety gitgiggety marked this pull request as ready for review August 8, 2021 18:24
@gitgiggety
Copy link
Contributor Author

Didn't check this branch in a while. But did some testing and bug fixing and I think it's good to go now (although it might be up for discussion whether more refactoring should be done on the filter inputs, but the "between" filter is ready).

@WithoutPants
Copy link
Collaborator

This approach is going to break existing clients and (as you mentioned) saved filters. The client shouldn't have to remember to put the value in lower for less than queries, and upper for greater than queries. The interface is confusing.

The approach I would prefer is that value is left unchanged, and a second optional value2 (or alternative named) field is added. This will be used exclusively for between/not between filtering, leaving the rest of the behaviour unchanged.

@gitgiggety
Copy link
Contributor Author

Rebased and made the requested change. After all the refactoring I already did it was still pretty simple to convert back to an optional value2. This new field is obviously optional, when using the (not) between filter and not providing this field it will default to 0 (which means any (not) between filter with a non negative value will yield no results).

@WithoutPants
Copy link
Collaborator

  • Because the current filters are greater than and less than, so being exclusive, and SQL between being inclusive I've "hacked" around this. In other words: setting the between criterion to 2 - 5 will only match values 3 and 4 (instead of 2, 3, 4 and 5 which a normal SQL between would do). And this obviously works the same for not between, 2 - 5 does match any value exact 3 and 4, so it does match 1, 2, 5, 6, ....

I don't think this is intuitive behaviour. If I select "between" as a filter option, I'd personally expect it to be inclusive, regardless of the rest of the filter options. Conversely, "not between" would be exclusive of the two values.

Extract the filters from the AddFiltersDialog into custom components.
This allows for further refactorring where components will be bound to
criterions.
@gitgiggety
Copy link
Contributor Author

Updated so between is inclusive instead of exclusive, as requested.

@WithoutPants WithoutPants added the feature Pull requests that add a new feature label Aug 12, 2021
@WithoutPants WithoutPants added this to the Version 0.9.0 milestone Aug 12, 2021
Copy link
Collaborator

@WithoutPants WithoutPants left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made a minor change so that existing saved filters with number criteria continue to work. Otherwise, looks good.

@WithoutPants WithoutPants merged commit 7cb3d05 into stashapp:develop Aug 12, 2021
@gitgiggety
Copy link
Contributor Author

Made a minor change so that existing saved filters with number criteria continue to work. Otherwise, looks good.

Whoops, obviously should have tested that during development.

And sorry about the getLabelValue() mess as well. When updating from exact/lower/upper I thought at one point that all those branches/ternaries could be removed and only (not) between was needed, but forgot to actually make those changes.

@gitgiggety gitgiggety deleted the int-range-criterion branch August 30, 2021 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Pull requests that add a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Can't Filter Duration Range of Scenes [Bug Report] Filter for 'Duration Range' in Scenes
2 participants