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

Token dialog fixes #3711

Merged
merged 4 commits into from
May 9, 2019
Merged

Token dialog fixes #3711

merged 4 commits into from
May 9, 2019

Conversation

ctrlaltca
Copy link
Contributor

@ctrlaltca ctrlaltca commented May 5, 2019

Related Ticket(s)

Short roundup of the initial problem

This PR fixes a few problems found during usage of the "edit tokens" dialog

What will change with this Pull Request?

@ebbit1q
Copy link
Member

ebbit1q commented May 5, 2019

Is the edit token dialog even still useful?

@ctrlaltca
Copy link
Contributor Author

ctrlaltca commented May 5, 2019

I don't think a lot of people use it, but at least now it's not crashing anymore :)

@ebbit1q
Copy link
Member

ebbit1q commented May 5, 2019

removing it also stops it from crashing

@ZeldaZach
Copy link
Member

I like the fix vs the removal. Why rip out things when you dont have to?

@ebbit1q
Copy link
Member

ebbit1q commented May 5, 2019

because nobody needs them

@tooomm
Copy link
Member

tooomm commented May 5, 2019

In my issue post I also suggested to rename that dialog to Edit custom tokens to stress what it's limited to.

There could also be a helpful sentence on top of the dialog maybe. Like
Create and edit custom tokens. All Changes go to the TK.xml file in your "customsets" folder.


@ebbit1q hmm, maybe it might be useful for other games?

@ebbit1q
Copy link
Member

ebbit1q commented May 6, 2019

In that case rework it into a "custom card maker"

@tooomm
Copy link
Member

tooomm commented May 6, 2019

Interesting idea, that would make a lot of sense!

An URL field for pictures would be handy then as well.
And it should be CC.xml, instead TK.xml maybe.
Also, we have to make sure people don't use it to create all custom sets with that since it's a predefined file and have some hint at least. Or make the dialog even more powerful to allow easy handling of that case.

I guess we need a new issue and have some discussion there and go with the fixes (and rename of the dialog) in this PR for now?

@ctrlaltca
Copy link
Contributor Author

An editor for custom sets (customsets/*.xml) would definitely be a nice addition, but it deserves itw own issue (maybe one already exists).
About this PR, we can either merge it and fix the current dialog or close it and create a new PR for removing the dialog. This last option would piss off any user actually using that dialog, even if it's just a few of them, until we add the proposed alternative.

@tooomm
Copy link
Member

tooomm commented May 6, 2019

Can you still rename the user facing dialog title, menu entry and shortcuts name please?

--> Edit custom tokens


I'm fine with merging this PR as long as we have no custom card/set dialog working! 👍

Copy link
Member

@tooomm tooomm left a comment

Choose a reason for hiding this comment

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

Thanks ❤️

Copy link
Member

@ebbit1q ebbit1q left a comment

Choose a reason for hiding this comment

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

Tested, working, for some reason tokens aren't loaded from the normal file in a built version.

@tooomm
Copy link
Member

tooomm commented May 8, 2019

Hmm, we need to check the influence on the Create token dialog in a game.
It looks like they share the same list and tokens from tokens.xml don't show up there now - only custom created ones from TK.xml (Edit custom tokens dialog).
I don't think that's a big problem because we have related cards feature and everybody uses that.
But if you add tokens to a deck they don't show up there any longer too.

@ctrlaltca
Copy link
Contributor Author

Restored the old in-game tokens dialog behavior of showing all tokens.
While the "related cards" feature is handy, manually creating tokens can still be a necessity for a lot of games.

@tooomm
Copy link
Member

tooomm commented May 8, 2019

Both issues are fixed now. 👍

Edit custom token remains unchanged and only shows TK.xml data
Create token now shows tokens.xml & TK.xml data, and also recognizes tokens added to the deck

@ctrlaltca ctrlaltca merged commit 6c21855 into Cockatrice:master May 9, 2019
@ctrlaltca ctrlaltca deleted the tokenz branch May 9, 2019 07:37
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.

"Edit tokens..." dialog needs update
4 participants