search

AEFeinstein / mtg-familiar

An Android app for all things Magic: The Gathering
Other
283 stars 71 forks source link

Enable import of popular deck formats #216

Closed MarcelBucz closed 3 years ago

MarcelBucz commented 7 years ago

Add a way to deliver predefined deck content (like top8s and precons) and a way for third parties to publish content.

AEFeinstein commented 7 years ago

While interesting, I have no desire to turn MTG Familiar from a standalone app into a platform. I would like a way to import popular deck formats, like .mwDeck, .dec, and MTGO TXT. Here are some examples of exported decks from mtgtop8:

http://mtgtop8.com/mtgo?d=296670&f=Standard_Bg_Delirium_Constrictor_by_Lukas_Blohon http://mtgtop8.com/dec?d=296670&f=Standard_Bg_Delirium_Constrictor_by_Lukas_Blohon

Ideally the deck editor gets an "import" button (different than load), and maybe Familiar gets an intent-filter to auto-import files opened in the browser.

AEFeinstein commented 7 years ago

@MarcelBucz if you're OK with that, you can change the ticket description. Otherwise I'll and make another.

Maxtille commented 5 years ago

I'd find it very usefull since I've installed this app to help me create and manage my mtga decks (I believe it's the same txt format as in mtgo) and so would be the ability to export in this format 😉

Maxtille commented 5 years ago

After all it's a bit different, here's an example https://drive.google.com/file/d/19hSxEKwR56qwI9UpZqoJcJGCP99iBQ3x/view?usp=drivesdk

AEFeinstein commented 5 years ago

I know @XVicarious was working on this (https://github.com/XVicarious/mtg-familiar/tree/export-inport-deck) and that there was a botched merge along the way. Any updates there?

Arena decks may be tricky if they don't use the official set codes. From your example, On Serra's Wings has (DAR) for Dominaria, but the official code is (DOM). The trailing card number should be pretty easy.

There's another issue where the Arena only cards like Inspiring Commander just don't exist on Gatherer...

2 Ajani's Welcome (M19) 6
24 Plains (RIX) 192
2 Moment of Triumph (RIX) 15
3 Ajani's Pridemate (M19) 5
3 Bishop's Soldier (XLN) 6
4 Daybreak Chaplain (M19) 10
3 Impassioned Orator (RNA) 12
2 Inspiring Cleric (XLN) 16
1 Danitha Capashen, Paragon (DAR) 12
1 Resplendent Angel (M19) 34
3 Leonin Warleader (M19) 23
1 On Serra's Wings (DAR) 28
1 Shalai, Voice of Plenty (DAR) 35
3 Herald of Faith (M19) 13
2 Spiritual Guardian (ANA) 11
1 Inspiring Commander (ANA) 5
4 Ritual of Rejuvenation (XLN) 32

3 Call to the Feast (XLN) 219
1 Kaya's Wrath (RNA) 187
1 Regal Bloodlord (M19) 222
omarjuul commented 5 years ago

I have been wanting to contribute to this beautiful app as I have used it quite a bit (thanks for creating it!). However I have very little experience coding Android apps (mainly been coding in C# past 5 years).

Yesterday I happened to be looking for a deck import function and noticed there wasn't one (yet). I thought maybe this is a good starting point for me to get into Android programming. Seems relatively "back end"-ish so no presentation stuff to worry about, and should be an interesting little parsing challenge.

I don't know what the current status is of @XVicarious work, is there anything left of that? Maybe I can just look at that codebase to help me get started?

Can I just get started trying to implement this? Are there any pointers or things I should keep in mind?

AEFeinstein commented 5 years ago

Glad to hear you want to contribute! I don't know what the status of @XVicarious's work is. You'd have to pick through his branch to see what he's done, and how.

There is some presentation stuff to worry about. The trickiest part of this is probably selecting files to import. Android seems to have a way to pick files as of API level 19, but Familiar is on API level 15. I can probably be convinced to push this up, but I'd have to look at metrics and see how many users are <19. The alternative is to use a library for file picking, like this one https://github.com/DroidNinja/Android-FilePicker (version 2.1.5, pre-androidx).

As far as other things to keep in mind, you'll need to add an option to decklist_menu.xml and the corresponding logic to DecklistFragment::onOptionsItemSelected(). DecklistDialogFragment has the logic for creating a new deck, which is probably the first step to import a deck. You'll want to handle the case where an imported deck has the same name as a saved deck, probably with a dialog asking to overwrite or cancel (presentation again).

Don't worry about string translations. Just do everything in English and they'll get translated before release.

And if you have more questions during development, ask away.

omarjuul commented 5 years ago

There is some presentation stuff to worry about. The trickiest part of this is probably selecting files to import. Android seems to have a way to pick files as of API level 19, but Familiar is on API level 15. I can probably be convinced to push this up, but I'd have to look at metrics and see how many users are <19. The alternative is to use a library for file picking, like this one https://github.com/DroidNinja/Android-FilePicker (version 2.1.5, pre-androidx).

I've seen other apps use de clipboard to read stuff, no idea if that's easier / more user friendly?

As far as other things to keep in mind, you'll need to add an option to decklist_menu.xml and the corresponding logic to DecklistFragment::onOptionsItemSelected(). DecklistDialogFragment has the logic for creating a new deck, which is probably the first step to import a deck. You'll want to handle the case where an imported deck has the same name as a saved deck, probably with a dialog asking to overwrite or cancel (presentation again).

Or maybe an option to rename the new deck, I guess?

And if you have more questions during development, ask away.

Thanks! I'm sure I'll need some help somewhere along the way :)

AEFeinstein commented 5 years ago

Using the system clipboard is one option. In that case, it'd need a large EditText, which is pretty simple. Importing from a file would be more complicated because it would require a separate app to open it and copy the contents to the clipboard. @omarjuul where would personally you be importing decks from?

Yes, a third option would be to rename the new (or existing) deck. Windows 10 only gives the options to replace or skip a file when moving into a folder where it already exists, so it's OK to not have a rename option. This keeps it simple.

And another thing to keep in mind, what happens when a card being imported doesn't exist in Familiar? This could be a typing mistake or something like the Inspiring Commander problem (see above). Our options are to do a partial import and display errors for the bad cards, or not import at all and display an error. I think the partial import is better.

omarjuul commented 5 years ago

Using the system clipboard is one option. In that case, it'd need a large EditText, which is pretty simple. Importing from a file would be more complicated because it would require a separate app to open it and copy the contents to the clipboard. @omarjuul where would personally you be importing decks from?

I don't know if I'm getting you wrong but you seem to suggest having a text input that the user fills with use of clipboard? That's not what I meant: I meant just reading it out of the system clipboard. I know of at least one app that does that.

Personally I'd like to import decks from MTGA, but also the entire Ravnica Cube a friend of mine built. That last one does not have the card numbers with it, just card name + set abbreviation. So I planned on building a pretty versatile parser :)

And another thing to keep in mind, what happens when a card being imported doesn't exist in Familiar? This could be a typing mistake or something like the Inspiring Commander problem (see above). Our options are to do a partial import and display errors for the bad cards, or not import at all and display an error. I think the partial import is better.

Yes I agree partial import with a message would probably be best there.

AEFeinstein commented 5 years ago

What app uses text directly from the clipboard? That's not a bad idea, but I'm not sure how that functionality would be communicated to the user.

Importing a whole cube worries me a little, not for the import itself but for the performance of the deck manager with hundreds of cards. I made some efficiency improvements a while back, but it still may stutter a little.

omarjuul commented 5 years ago

Magic Arena Net Deck has a button Import deck from clipboard. It seems to directly read the clipboard.

I'll just have to wait and see how the deck manager performs with the cube (720 cards). I'll report back after I test it (could take a while) ;).

AEFeinstein commented 5 years ago

I'll check out Magic Arena Net Deck and see how I like their solution.

MailYouLater commented 5 years ago

I recently decided I'd like to put all of my MTG decks into this app, but I find entering cards to be ... rather time consuming. I found another app (TCGplayer) which allows me to enter cards by placing them in front of the camera one at a time, and I can easily and fairly quickly place a bunch out on a table, scan them (fix the ones where it mixed up which set their from), pick them up and move on to the next batch. While it would be awesome for MTG Familiar to have this feature, it would be much easier to simply allow the data collected in this other app to be imported to MTG Familiar. TCGplayer has the ability to export to a file (CSV), the clipboard, or to "Share" directly to another app, and it would be really cool if MTG Familiar could accept data it exported to the clipboard and/or import a deck list by accepting TCGplayer's 'share'.

Personally, I'd prefer to see the 'Import from clipboard' option be a text input which a user can paste into, because many phones these days allow for multiple clipboard items through a 'Clip Tray' (that's what my phone calls it) of some variety and I may want to export a few decks to the clipboard (from another app), then paste them (one at a time) into the import text field without having to switch back and forth between the two apps.

omarjuul commented 5 years ago

I checked to see the export format for TCGplayer:

TOTAL: 36 cards - $45.16
5 Swamp [RNA]
6 Forest [GRN]
3 Evolving Wilds [AKH]
4 Hissing Quagmire [OGW]
4 Blooming Marsh [KLD]
4 Fatal Push [AER]
4 Dissenter's Deliverance [AKH]
4 Traverse the Ulvenwald [SOI]
2 Never // Return [AKH]

So for that format we'd have to ignore (or parse separately) the first line. I think that's doable.

@AEFeinstein Is there a release schedule that would give me some kind of deadline to finish this before the next release, or do you just release as new content or bugfixes are ready? edit: oh and about the sharing, I would guess that's something that needs to be changed in the manifest or somewhere like that, right? Just to indicate that MTGF accepts sharing (via text)?

AEFeinstein commented 5 years ago

@MailYouLater card scanning is cool indeed, but I don't have the expertise to do it on-phone, nor do I want to maintain a server to do it.

There are two ways to import data from the clipboard. Magic Arena Net Deck has a single button which imports whatever is on the clipboard. I don't know how this interacts with a Clip Tray, but I suspect it uses the most recent item. The other way to do it is to have a text field to paste into. The single button is faster (i.e. requires less interaction), but the text field plays better with a Clip Tray, and any unparsed lines can remain, while parsed lines are removed, which gives the user an opportunity to fix lines.

@omarjuul I'd just ignore that first line (or leave it as an unparsed line in a text field). There's no release schedule because I work on Familiar only when I feel like it 😃. An app doesn't need special permissions to access the clipboard. The whole point of a clipboard is to be system-wide!

omarjuul commented 5 years ago

Good point about the lines that aren't successfully parsed staying in the text field to get fixed, I like that!

Nah I meant to support the 'share via other app' feature, MTGF somehow needs to let android (or some manager) know that it can import and which function needs to be called to do that. I don't know very well what the manifest is for, so that was probably confusing. I just meant that we'd probably have to indicate somewhere what that share feature needs to call. I guess something like this? (only had a quick look as I really have to go sleep right now)

AEFeinstein commented 5 years ago

In general, Android manifests declare hardware requirements, permissions, icons, activities, services, widgets, etc. You don't need any special permissions or declarations for getting text from the clipboard, so you don't need to mess with the manifest. That can all be done inside the import function.

Intent filters are useful for other things. Familiar has some so that when a web link is clicked, if it's for a card, it can open in Familiar.

omarjuul commented 5 years ago

I was refering to this part of the comment above:

it would be really cool if MTG Familiar could accept data it exported to the clipboard and/or import a deck list by accepting TCGplayer's 'share'.

After reading my earlier link a bit more thoroughly, I think that accepting a share from another app would be accomplished by an intent filter, right?

Then I have another question: How do we want to deal with split cards? I think currently when viewing the card there is a other half button at the bottom of the screen, but each spell is considered a separate card, and I can add either half to a deck. Shouldn't it be represented as the split/fuse/... card it is, in the deck list (so that you can see both spells that are on the card)?

AEFeinstein commented 5 years ago

Oh, I get it now. You are correct, to receive text from an Android share you'd need to update the manifest. The page you linked has the correct documentation.

As for split, double faced, and flip cards, just add the first half to the deck. Showing both the names is nice, but that doesn't happen in the deck builder currently, so I wouldn't bother. I also think it'd be more work than you're expecting to do so.

MailYouLater commented 5 years ago

I'd say the question of how cards with multiple spells should be dealt with should really be discussed in it's own separate issue.

Anyway, on topic here: It sounds like you guys have mostly figured out appropriate plans for what each of the three features being discussed here would be like, and honestly, any of them would work fine for my purposes, and if in the long run multiple become available, having choices in things like this is nice.

omarjuul commented 5 years ago

I've been looking at the code and working on this in between other things, the past week. I think I have the parsing bit mostly finished but I'll need some more time "glueing" it to the rest of the app. I've made a new Fragment with a big text field (and a small name for the deck name). I was just wondering what would be the best way to look up the cards in the database. I think for now I'll go with just using the MtgCard constructor that has the activity passed in, which means a lot of database opening and closing, but I guess we'll have to see if that gives any (performance) problems... the advantage with this method is that it'll be easier to report which cards are not found. Any thoughts on that @AEFeinstein ? I noticed there's also a batch version of the card lookup, which I guess was made for a reason? If you think I should go with the batch version then I can probably make that work as well, will just be a bit more complex I think.

Performance brings me to another point. Should I do the import in a Task? I saw the LegalityCheckerTask and wondered if I needed something similar? Up till now I've just changed the text of the button to "Importing" and started to loop through the lines... Maybe it's better to offload that to another thread? On the other hand, there won't be anything for the user to do, anyway, so if the app freezes up momentarily it might not be that bad?

As an aside, should I assign myself to this issue, to signify that I'm working on it? And if so, where can I do that?

AEFeinstein commented 5 years ago

Importing via text field and import by accepting text intent should be sufficient. Users can paste into the text field, so a button to pull directly from the clipboard is redundant. Ideally the text intent opens up and fills the text field, so it all funnels to the same place anyway.

@omarjuul initCardListFromDb() may be what you want. If you have an ArrayList<MtgCard> where mName and mExpansion are set, it'll fill the rest of the card information with a single query. If a card was actually found, it should have a nonzero mMultiverseId. The function currently requires those two fields though, so you might have to modify it for deck imports without expansions. Do you know enough SQLite to do that?

Performance wise, you should probably use an AsyncTask. Familiar wasn't written with performance in mind, so it does do some stuff on the UI thread it shouldn't (mostly database access). You can do better than me! I'd recommend using setLoading() and clearLoading() to visually indicate work is being done.

AEFeinstein commented 5 years ago

To your aside, I can only assign tickets to collaborators. But collaborators can also push directly to master, and I had issues with code quality a long time ago, so I'm the only collaborator now. It's assigned to you in my ❤️

omarjuul commented 5 years ago

@omarjuul initCardListFromDb() may be what you want. If you have an ArrayList<MtgCard> where mName and mExpansion are set, it'll fill the rest of the card information with a single query. If a card was actually found, it should have a nonzero mMultiverseId. The function currently requires those two fields though, so you might have to modify it for deck imports without expansions. Do you know enough SQLite to do that?

I don't think the SQL will be a problem for me. However I don't think I would do the full filtering in SQL (that would require making a sub table for each card(name) with all its sets and adding "LIMIT 1". Or at least that's the only way I can think of)... I'd probably end up fetching all sets of the card(name)s, and then just take the first (latest) set for each card(name). I wonder if it might be easier - and about as fast - to fetch the cards one by one, keeping the db connection open in the meantime. Because I think opening and closing the connection is the time consuming part of it, isn't it? I'm assuming the KEY_DATE column is the release date of the set, so that the newest sets get put first (in the current implementation with ORDER BY DESC).

Performance wise, you should probably use an AsyncTask. Familiar wasn't written with performance in mind, so it does do some stuff on the UI thread it shouldn't (mostly database access). You can do better than me! I'd recommend using setLoading() and clearLoading() to visually indicate work is being done.

I'll see if I can get that working.

AEFeinstein commented 5 years ago

I'm not exactly sure what you're trying to do, but in my experience using fewer SQLite queries is always faster, even when they return more data than multiple single queries would. Do you have a list of card names and optional expansions, or something else? It may help if you push code to your fork so I can look at it.

KEY_DATE is the release date for an expansion, so ordering by that is good. I would recommend searching for KEY_ONLINE_ONLY == 0 too, so that cards from Magic Online exclusive expansions don't get imported.

Also, Android uses SQLite, not SQL. It's a slight difference, but it matters.

omarjuul commented 5 years ago

At the moment I have just one list of MtgCards (the simple ones that still need to get data from the database). The set code can be empty (not null though). I could easily split the list into two lists: one with set codes and one without. That way two lookups in the database will suffice. Or I could update the existing fetchCardByNamesAndSets to skip specifying the set in the query if set code is empty. Would that break stuff?

I will try to push to GitHub once more, last time I tried Android Studio gave me shit about having provided an invalid login...

Why would it be bad to import Magic Online exclusive expansions?

omarjuul commented 5 years ago

So apparently GitHub login via Android Studio doesn't work if I have 2FA turned on... da heck.

AEFeinstein commented 5 years ago

I recommend modifying initCardListFromDb() to always query by name and query by expansion if not empty, rather than requiring both name and expansion. That flexibility shouldn't break anything, and it should fill in all your missing data without breaking anything.

After calling that function you'd have to iterate through the list of MtgCard and check if mMultiverseId is nonzero. If it is zero, then the lookup failed, and the line should be left in the text box.

Familiar can't look up prices for online-only cards, so I try to avoid them. The TCGPlayer.com API only works for actual paper.

Github authentication sounds silly.

MailYouLater commented 5 years ago

To your aside, I can only assign tickets to collaborators. But collaborators can also push directly to master, and I had issues with code quality a long time ago, so I'm the only collaborator now. It's assigned to you in my ❤️

It's totally up to you if you want to mess with any of this, but if you are interested in adding contributors, but not allowing them to directly push to master, then you might find the info on these pages interesting:

Familiar can't look up prices for online-only cards, so I try to avoid them. The TCGPlayer.com API only works for actual paper.

I'm not sure why people would want to use MTG Familiar with Online only cards, but in case people do, I suppose it would be cool for them to be able to enable it in an option. However that's entirely unrelated to this discussion, so if someone wants that feature they should open a new issue requesting it.

AEFeinstein commented 5 years ago

I'll look into protected branches and status checks after work today, that looks promising.

Online only cards were added so that Pauper could be properly supported. There are some cards, like Chainer's Edict which only have a common version online, and therefore are legal in Pauper. Without the online expansions, Familiar wouldn't know this. I try to avoid adding online-only cards to lists when the expansion is ambiguous.

omarjuul commented 5 years ago

I recommend modifying initCardListFromDb() to always query by name and query by expansion if not empty, rather than requiring both name and expansion. That flexibility shouldn't break anything, and it should fill in all your missing data without breaking anything.

If I'm seeing it correctly, fetchCardByNamesAndSets() is only called from initCardListFromDb(). So I'd also make changes there, right? To keep it to just one db call instead of two?

Github authentication sounds silly.

that it does

AEFeinstein commented 5 years ago

If I'm seeing it correctly, fetchCardByNamesAndSets() is only called from initCardListFromDb(). So I'd also make changes there, right? To keep it to just one db call instead of two?

Correct.

AEFeinstein commented 5 years ago

Hi @omarjuul any progress on this since the last comment?

MailYouLater commented 5 years ago
Using Github's branch and commit comparison features:
Author master...branch commit...commit (as of July 12 2019)
@XVicarious https://github.com/AEFeinstein/mtg-familiar/compare/master...XVicarious:export-inport-deck https://github.com/AEFeinstein/mtg-familiar/compare/0aa561625ca94d6ca0f3f6315b42aedadd33dcaf...eead29679ccf104a5f42a5d0fdb9539cc905601a
@omarjuul https://github.com/AEFeinstein/mtg-familiar/compare/master...omarjuul:import-deck https://github.com/AEFeinstein/mtg-familiar/compare/ea8e3d77337ddc58db5c53627bc7732821b7ba73...b81a2ae7572a17cdaa6f44c5a83657c674ce5988

I too am looking forward to seeing how this has been / will be progressing.

AEFeinstein commented 5 years ago

Thanks, though sometimes it's easier to get a summary than piece through a diff.

MailYouLater commented 5 years ago

Yeah, well there haven't been any new commits published since @omarjuul's last comment, though he could still be working on it, and just hasn't pushed to GitHub lately.

I posted the commit...commit link for the branch @XVicarious was working on too, because after getting merged & reverted, master...branch link for that branch shows nothing, and there's a chance @omarjuul may find some of @XVicarious' work helpful.

omarjuul commented 5 years ago

Hey guys, I've been a bit busy with other things lately (mtga is not good for my spare time 😬) so unfortunately haven't worked on mtgFam for a while. I think I have a "finished" version, but when I start the app on the VM and click the import button nothing seems to happen. I haven't extensively debugged this yet, I've probably wired something wrong, but as I said I haven't looked at is the past couple weeks.

I just pushed my latest changes to GitHub. I will return to this if no one else picks it up, but I don't know when.

AEFeinstein commented 5 years ago

MTGA sure isn't good for spare time! It's unlikely that I'll pick up where you left off any time soon, but it's good to know your changes are pushed for a later date.

AEFeinstein commented 5 years ago

A user requested wishlist import as well. It should be easy to apply the same logic & UI from the decks to the wishlist.

m8ram commented 5 years ago

Hi,

I'm interested in importing data as well. I have my current list of cards (2104 in total) in deckbox.org that I would like to try and import. Unfortunately the export options of single decks do not include the expansion. An export of the entire inventory does include the expansion but not the deck). If there is a beta version I can test to provide feedback please let me know.

AEFeinstein commented 5 years ago

There isn't a beta version for this feature. Work on it seems to be pretty start-stop...

edrutte commented 3 years ago

532

MailYouLater commented 3 years ago

There isn't a beta version for this feature. Work on it seems to be pretty start-stop...

So... now that #532 has been merged, is there any chance of getting a beta build? (or even an untested CI build?)

MailYouLater commented 3 years ago

There isn't a beta version for this feature. Work on it seems to be pretty start-stop...

So... now that #532 has been merged, is there any chance of getting a beta build? (or even an untested CI build?)

AEFeinstein commented 3 years ago

There are CI builds over here: https://circleci.com/gh/AEFeinstein/mtg-familiar. I'm not sure how the permissions work, but for me I:

  1. Click "Success" for a build
  2. Click "build" with the time next to it
  3. Click the "Artifacts" tab
  4. Download "apks/debug/mobile-debug.apk"

If you don't have access, I can get you a one-off build. Also of note, the CI builds don't have the TGCP API key, so price lookups do not work.