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

Allow dual-face cards to be found by their collector number #613

Closed
wants to merge 1 commit into from

Conversation

blazingkin
Copy link

@blazingkin blazingkin commented Apr 20, 2023

The collector numbers of dual-face cards are internally represented as NNNa and NNNb for the front and back face respectively.

This means that a user searching for the collector number NNN will not find either face.

Correct this by including the 'a' and 'b' variants in our search when the user searches for only a number.

Ideally these would be two separate structured fields, but that is left to a future refactor.

Small notes

Performance impact seems to be minimal (This already fast search may perform up to 3x slower). In practice a search over the entire history of magic cards still completes in milliseconds.

The 'a' and 'b' scheme is somewhat hardcoded now, hopefully no new cards are printed with a 3rd face.

I'm sure the string concatenation here is slow, but it's a small price to pay for its convenience.

Fixes #611

The collector numbers of dual-face cards are internally represented as
NNNa and NNNb for the front and back face respectively.

This means that a user searching for the collector number NNN will not
find either face.

Correct this by including the 'a' and 'b' variants in our search when
the user searches for only a number.

Ideally these would be two separate structured fields, but that
is left to a future refactor.

Small notes
----------------------------------------------------------------------
Performance impact seems to be minimal (This already fast search may
perform up to 3x slower). In practice a search over the entire
history of magic cards still completes in milliseconds.

The 'a' and 'b' scheme is somewhat hardcoded now, hopefully no new
cards are printed with a 3rd face.

I'm sure the string concatenation here is slow, but it's a small price
to pay for its convenience.
@AEFeinstein
Copy link
Owner

Un- sets are the worst, and "Who // What // Where // When // Why" has a, b, c, d, and e numbers. Instead of appending the search with more OR statements, if part is a numeric, you can append a % to the end of part (i.e. 123 becomes 123%). That will match the number and anything after it. This will catch any letters or other stuff in the number string, like ⭐ which pops up in some promos.

@blazingkin
Copy link
Author

I considered this, except a % on a 1 search would then also grab 11, 12, 13, ..., etc. I can change it to a capture group of [abcd...z]. Not sure if the ⭐ could be included in that.

It's a bummer it is actually more complicated. That may mean that it makes sense to try to handle the data as it gets imported and keep the collector number row numeric and have separate rows for these modifiers. I just didn't want to have to deal with the fact that would need a whole database migration (via dropping and rebuilding or some kind of patch). I may look at that more seriously.

Thanks for the feedback!

@AEFeinstein
Copy link
Owner

Fair point with the wildcard.

I checked the db and here's the list of non-numeric collector's numbers: odd-numbers.txt. You get the and characters, Arena prefixes some numbers with letters, and sometimes combos like A-85a or 67†a. Nothing is ever simple :)

@AEFeinstein
Copy link
Owner

Shame that the REGEXP operator isn't supported for Android's SQLite. I'm not sure what the best approach is here, but only checking 'a' and 'b' probably isn't it.

@blazingkin
Copy link
Author

Yup, you are definitely right. I think the right approach here is to parse the data into more rows when importing it.

I'm not sure I'm going to be able to do that without a bit of help finding how the data is imported and a bit of knowledge about how database migrations typically are run. Do they happen on app update? Is there a common pattern for them?

@AEFeinstein
Copy link
Owner

There's an informal pattern for adding card data columns to the database

  1. Modify the statement to create the table (DATABASE_CREATE_CARDS)
  2. Modify the function that adds cards to the database to initialize the new column (createCard())
  3. Look for all usages of a similar key to see where that data needs to be initialized or copied or whatever (right click on a key -> Find Usages)
  4. Increment DATABASE_VERSION
  5. Run Familiar and long press the "Force Update" button to rebuild the database with your changes.

The database is compressed and included in the APK, so everyone will just get it with an app update. I'll handle that part.

What column(s) exactly are you looking to add?

@blazingkin blazingkin closed this May 7, 2023
@AEFeinstein
Copy link
Owner

Not worth the effort?

@blazingkin
Copy link
Author

Oh, I'm just closing this pull request because the changes here aren't going to be the right solution.

I'll open a new one once I've got a better diff :)

I do have to find a few hours to sit down and do it though, hopefully I can find that in a week or two.

My thought as far as a patch is to re-organize how data is imported. There should be a column that's strictly numeric, one for the card face, maybe one for the promo star, and then a string that's the raw collector number. I'll have to write a parser that takes all the crazy data from the data source and splits up into those columns

@AEFeinstein
Copy link
Owner

Ah gotcha. Just remember that whatever solution you come up with should be as future-proof as possible (which is tough when WotC does new stuff all the time). The simple two-column solution (one is only numeric, the other is the full string) may be good enough here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants