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

Deck versioning #599

Closed
wants to merge 14 commits into from
Closed

Deck versioning #599

wants to merge 14 commits into from

Conversation

MarioZG
Copy link

@MarioZG MarioZG commented Jan 22, 2015

I have added something that was bugging me for some time - deck versioning. You can now change few cards, and instead creating new deck, save new version and keep old one for reference. Stats for both versions will accumulate, but you can see also stats for each version separately.

Most of the code and changes are done "to make it work for myself". I just thought it might be also useful for others.

@azeier
Copy link
Member

azeier commented Jan 22, 2015

I really like the idea but it feels.. confusing?

A few issues I see right now:

  • Without having any changes, there's just an empty panel on the right, not at all telling you what it's supposed to be. Maybe there should even be an extra button to expand this?
  • The version indicator in edit mode should probably not be in the menu bar.
  • Yet another dialog after saving a edited deck ("I just wanna edit a card, leave me alone!").
  • That -1! lol (we need to clean up the numbers in general though)
  • The history panel still contains the last edited deck when creating a new one or editing one without a history.
  • Stats overview needs some changing, height increase mainly.
  • Version numbers in help and the tray tooltip now display "0.7.-1"

What if we moved the version indication and increment to the top of the history panel? That would pretty much solve the first 3 of the problems.
Something along the lines of "[Textbox with current version] -> [Combobox containing current, minor increment and major increment]"?

@GiggliG
Copy link

GiggliG commented Jan 22, 2015

I cant comment on how this works, but the idea is something I find EXTREMELY useful.

@azeier
Copy link
Member

azeier commented Jan 22, 2015

Hehe, I figured you might like this one!

@MarioZG
Copy link
Author

MarioZG commented Jan 22, 2015

I didn't spend too much time with UI, just made it work and displayable. If you think this might get pulled to master, i'll tidy this up.

In terms of -1 icon, that sums up my icon design skills :). I cannot guarantee anything much better than that.

As for additional message box during save - i had button initially to increase version, but I always forgot to click it before save, hence message box. Drop down idea might work, with 3 options as in dialog (Major/Minor/None). None by default, and when deck is edited switch it to major.

@azeier
Copy link
Member

azeier commented Jan 22, 2015

I would love to merge this into master, yes!

If you need any help fixing things up, let me know. I should have some spare time this weekend.

@MarioZG
Copy link
Author

MarioZG commented Jan 22, 2015

I should have some time later today/tomorrow to tidy UI - keep an eye for new commits ;)

@MarioZG
Copy link
Author

MarioZG commented Jan 23, 2015

I have polished this a little bit.

  • now it's just major version (1,2,3....)
  • added tickbox next to save "increase version"
  • improved history panel display
  • changed layout of deck stats. Works same way as overall stats, with separate tab for game list
  • added deck version to deck's game list
  • fixed version issue in help and try icon
  • -1 icon is still the same :), can't work the graphics...

Added, when moving game to other deck, game is assigned to current version of target deck

Fell free to do any UI changes - i believe you have better feel for it ;)

@azeier
Copy link
Member

azeier commented Jan 24, 2015

Pushed this to feature/DeckVersioning to at least have the merge conflicts out of the way.

Looks a lot better now. Still needs quite a bit of work but we're getting there :).

I also liked having major and minor, I think I'm gonna put that back in.

@azeier
Copy link
Member

azeier commented Jan 24, 2015

How about graying out the cards instead of having red numbers?

screenshot 2015-01-24 09 33 52

@azeier
Copy link
Member

azeier commented Jan 24, 2015

The history panel now matches the rest a bit better, reintroduced minors and changed the version selection a bit:

screenshot 2015-01-24 10 28 26

@MarioZG
Copy link
Author

MarioZG commented Jan 24, 2015

That looks much better now.

I would move dropdown with version selection next to save button. They seems to belong together, since they are part of save process.

@azeier
Copy link
Member

azeier commented Jan 24, 2015

You've got a point. Like this maybe?
save

@MarioZG
Copy link
Author

MarioZG commented Jan 24, 2015

Exactly. Looks perfect.

@azeier
Copy link
Member

azeier commented Jan 24, 2015

@GiggliG
Copy link

GiggliG commented Jan 24, 2015

lol, I'm up for testing.
If i install it in a separate folder, will it use its own data store, and is there a way I can import a COPY of my old data into it so it doesn't change that data in use? I'm worried about data corruption.

@azeier
Copy link
Member

azeier commented Jan 24, 2015

Easiest will be to just create a backup of you appdata folder. I went back and forth between this and v0.7.5 a few times without problems, should be fine.

Just to be save, probably still a good idea.

@GiggliG
Copy link

GiggliG commented Jan 24, 2015

Okay, backed that up and will test this over the weekend.

@GiggliG
Copy link

GiggliG commented Jan 24, 2015

I've done a quick test, and it looks good so far.
I made a deck change in HDT, and saved it as v1.1.
I then played a game in hearthstone, and realised I'd forgotten to make the change the deck there. That game then got saved as a v1.1 game, though it was still using the v1.0 deck list.

So I think you'll need a button to "Move Game to other version of this deck" like you have with "Move to other deck" because people will make the same mistake I did. I think it makes sense for these to be two different buttons, and the "change version" button should be greyed out if the active deck only has one version.

Some feature suggestions:

I'd like the (optional, not required) ability to put a text descriptor with a version name, like
Secrets Mage, v1.1, Anti-aggro

I think you could have a Version Switcher, at the top of the deck display, so you can see what versions exist, and change which one is active and displayed. This would not be in edit mode, but in normal mode.

This would allow people to flip through their versions, and also change which one HDT detects as the active one when playing hearthstone. I'm surer there are people who will create multiple versions, and then go back to an earlier version as the most effective one.

If you have this view version mode, you could have a popup or panel showing the differences between the current view, and the last version that was active - like on hearthpwn, where they have updates, like "+1 Knife Juggler, -1 Cold Light Oracle" or whataver"
It could be a hovering tooltip somewhere , maybe, to avoid clutter, or could be a simple text added to the bottom of the deck list.

Here's one out-there feature suggestion that I think will be very useful for some (like me) but maybe not for most, so i could understand if you skip this.
Have an option for the program to treat major versions (v1, v2, v3) as separate decks, which each can appear as choices in the deck detection/selection screen when you play in hearthstone.
For example, I know some people maintain multiple versions of a deck that are almost identical, but switch between them depending on changes in the meta. I maintain two versions of several decks for playing in the US and EU servers, which are very similar, but have differences because my card collections aren't the same in those two servers. I'd love to be able to combine them into a single deck, and have HDT suggest the two major versions when I start a game (or use the correct one, when it detects the cards that only exist in that deck).

@GiggliG
Copy link

GiggliG commented Jan 24, 2015

I'll continue testing, as I play more games :)

@GiggliG
Copy link

GiggliG commented Jan 24, 2015

Another suggestion: with this change, all decks get (v1.0) shown as part of their name in the deck list.
I suggest having that only show up if there is more than one version. Allowing people to see at a glance which of their decks have multiple versions.

Will you also be integrating versions with the "Update from Web" feature? So that if a deck has changed online, the original version is saved, and a new version is added with the changes?

@azeier
Copy link
Member

azeier commented Jan 24, 2015

"Update from web" should work just as any editing would. Or am I misunderstand you somehow?

@MarioZG
Copy link
Author

MarioZG commented Jan 24, 2015

In feature branch, there is [XmlIgnore] Attrbute on PlayerDeckVersion propert of GameStats class, which means that deck version is not stored with game. All stats are assigned to v 1.0 when you start application.

@GiggliG
Copy link

GiggliG commented Jan 26, 2015

I agree keeping games for historic reasons makes sense, but if you delete the deck it's very hard to get to view those games. Maybe the Stats/Overall frame could do a Class Played filter, to make that easier.

Something to also consider.

When you save the deck, you still get that "How do you wish to save the deck popup" with Overwrite and Save as New options.
I assume that dialog will be phased out? I hope so anyway, it always made me panic a little (thinking, "If I overwrite, will I erase all my saved deck data?"). Will there still be a need for it?

There's another popup that occurs when you clone a deck, that asks if you want to keep the history. It is very confusingly written.

The popup says,
Clone Game Stats?

  • "Cloned games do not count towards class or overall stats.
  • Yes / No

I am never sure which button to press to make a copy that has no stats, and it could be interpreted to mean that if you click Yes, you are agreeing with the text (Cloned games do not count against stats) and if you click No you are disagreeing with that.

This IMO should be rewritten as something like:
Title: History of Games Played
Text: "Do you want to clone the history of games played, as well as the cards? (Whether you click yes or no, cloned games do not count towards overall stats.)"
Buttons: Yes / No

@GiggliG
Copy link

GiggliG commented Jan 26, 2015

Will there be a way to move games played from one version to another? I just noticed you have fixed the games not showing up in 1.0 correctly, but the three games I played in version 1.1 are listed under v 1.0.

Also, unrelated (I'd report this as a general bug, but it might just be this version): When you clone a deck, and keep the game history , the Name column is empty in the new deck's history (and the * which shows there someone conceded is missing too).

@MarioZG
Copy link
Author

MarioZG commented Jan 27, 2015

There's bunch o nulll exceptions when you run application without deck selected - MarioZG@cdcca5f

@azeier
Copy link
Member

azeier commented Jan 28, 2015

I added you as a collaborator. You can now just push things directly to feature/DeckVersioning.

This way we don't need to to things twice :).

@MarioZG
Copy link
Author

MarioZG commented Jan 28, 2015

Thanks. That's pushed now.

azeier pushed a commit that referenced this pull request Jan 28, 2015
azeier pushed a commit that referenced this pull request Jan 28, 2015
@azeier
Copy link
Member

azeier commented Jan 28, 2015

  • Added "save as" and "clone selected"
  • Removed the "How do you wish to save the deck" popup
  • Clone dialog now reads:

Clone game history?
(Cloned games do not count towards class or overall stats.)
[clone history] [do not clone history]

  • also fixed cloning player, oppponent name and replay reference

I think, to get this working for now, moving decks between versions and deleting versions probably has to wait for the next release.

Anything else that's still missing?

azeier pushed a commit that referenced this pull request Jan 29, 2015
azeier pushed a commit that referenced this pull request Jan 29, 2015
azeier pushed a commit that referenced this pull request Jan 29, 2015
azeier pushed a commit that referenced this pull request Jan 29, 2015
azeier pushed a commit that referenced this pull request Jan 29, 2015
azeier pushed a commit that referenced this pull request Jan 29, 2015
azeier pushed a commit that referenced this pull request Jan 29, 2015
azeier pushed a commit that referenced this pull request Jan 29, 2015
azeier pushed a commit that referenced this pull request Jan 29, 2015
azeier pushed a commit that referenced this pull request Jan 29, 2015
azeier pushed a commit that referenced this pull request Jan 29, 2015
@azeier
Copy link
Member

azeier commented Jan 29, 2015

Git is still confusing the hell out of me sometimes...

I just wanted to rebase this on master after merging the missing cards feature, but that didn't quite work for some reason.

So I pushed the rebase (this) to master now, everything seems to be working fine. I hope...

@azeier
Copy link
Member

azeier commented Jan 29, 2015

I'm going to close this, as it's merged now. We can continue discussion here though.

@azeier azeier closed this Jan 29, 2015
@azeier
Copy link
Member

azeier commented Jan 29, 2015

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

Successfully merging this pull request may close these issues.

3 participants