TrackableType: cleanup default value, reduce need of extra classes

[Hanmac]

This cleanup reduce the need to define extra classes just for getDefaultValue
(java makes them anyway under the hood)

• add new TrackableValueType where you can give its default value, mostly variant of 0 or Empty View
• TrackableType now supports set default value
• TrackableObjectType has null as default
• TrackableCollectionType has an empty collection (that isn't created each time)

[tool4ever]

looks bad:

[tool4ever]

Imo it'd break (at least here) now, modifying the default value:

forge/forge-game/src/main/java/forge/game/card/CardView.java

Lines 1764 to 1770 in 908c095

	TrackableCollection<CardView> views = get(key);
	if (views == null) {
	views = new TrackableCollection<>();
	views.add(cardToAdd.getView());
	set(key, views);
	}
	else if (views.add(cardToAdd.getView())) {

[Hanmac]

wait, my fix doesn't work, need to do it another time

Edit:
I was thinking computeIfAbsent + getDefaultValue, but that doesn't work either 🤔

[Hanmac]

@tool4ever your opinion on using:
computeIfAbsent(key, (t) -> new TrackableCollection<CardView>()) ?

[tool4ever]

Hmn, I think it's still just asking for trouble but not enough benefit:
this could easily introduce subtle bugs by new code since it'd be reasonable to expect that modifying the returned collection is safe :/

Besides what's the point of switching the default anyway? We know we'll still want to work with null values so the Maps can stay small...

[Hanmac]

Besides what's the point of switching the default anyway? We know we'll still want to work with null values so the Maps can stay small...

Because it's stupid that we need to differ between (empty) collection and null values.

(Even if we store empty collection as null behind)

[Hanmac]

@tool4ever i can't really overwrite FCollection.getEmpty might need to remove that
(see can't override static methods)

but i would be okay if:

public FCollectionView<CardView> getChosenCards() {
        return get(TrackableProperty.ChosenCards);
}

Would return empty Collection if able?
Then I don't need to mess with the default values,
while still be able to just iterate them in the GUI?

hm The GUI part doesn't need to know that it's a Trackable Collection inside 🤔

[Hanmac]

Maybe related, but i noticed that the CardView helper functions that work with CardCollection stuff,

forge/forge-game/src/main/java/forge/game/card/CardView.java

Lines 1736 to 1857 in 50dfcde

	Card setCard(Card oldCard, Card newCard, TrackableProperty key) {
	if (newCard != oldCard) {
	set(key, CardView.get(newCard));
	}
	return newCard;
	}
	CardCollection setCards(CardCollection oldCards, CardCollection newCards, TrackableProperty key) {
	if (newCards == null || newCards.isEmpty()) { //avoid storing empty collections
	set(key, null);
	return null;
	}
	set(key, CardView.getCollection(newCards)); //TODO prevent overwriting list if not necessary
	return newCards;
	}
	CardCollection setCards(CardCollection oldCards, Iterable<Card> newCards, TrackableProperty key) {
	if (newCards == null) {
	set(key, null);
	return null;
	}
	return setCards(oldCards, new CardCollection(newCards), key);
	}
	CardCollection addCard(CardCollection oldCards, Card cardToAdd, TrackableProperty key) {
	if (cardToAdd == null) { return oldCards; }
	if (oldCards == null) {
	oldCards = new CardCollection();
	}
	if (oldCards.add(cardToAdd)) {
	TrackableCollection<CardView> views = get(key);
	if (views == null) {
	views = new TrackableCollection<>();
	views.add(cardToAdd.getView());
	set(key, views);
	}
	else if (views.add(cardToAdd.getView())) {
	flagAsChanged(key);
	}
	}
	return oldCards;
	}
	CardCollection addCards(CardCollection oldCards, Iterable<Card> cardsToAdd, TrackableProperty key) {
	if (cardsToAdd == null) { return oldCards; }
	TrackableCollection<CardView> views = get(key);
	if (oldCards == null) {
	oldCards = new CardCollection();
	}
	boolean needFlagAsChanged = false;
	for (Card c : cardsToAdd) {
	if (c != null && oldCards.add(c)) {
	if (views == null) {
	views = new TrackableCollection<>();
	views.add(c.getView());
	set(key, views);
	}
	else if (views.add(c.getView())) {
	needFlagAsChanged = true;
	}
	}
	}
	if (needFlagAsChanged) {
	flagAsChanged(key);
	}
	return oldCards;
	}
	CardCollection removeCard(CardCollection oldCards, Card cardToRemove, TrackableProperty key) {
	if (cardToRemove == null || oldCards == null) { return oldCards; }
	if (oldCards.remove(cardToRemove)) {
	TrackableCollection<CardView> views = get(key);
	if (views == null) {
	set(key, null);
	} else if (views.remove(cardToRemove.getView())) {
	if (views.isEmpty()) {
	set(key, null); //avoid keeping around an empty collection
	}
	else {
	flagAsChanged(key);
	}
	}
	if (oldCards.isEmpty()) {
	oldCards = null; //avoid keeping around an empty collection
	}
	}
	return oldCards;
	}
	CardCollection removeCards(CardCollection oldCards, Iterable<Card> cardsToRemove, TrackableProperty key) {
	if (cardsToRemove == null || oldCards == null) { return oldCards; }
	TrackableCollection<CardView> views = get(key);
	boolean needFlagAsChanged = false;
	for (Card c : cardsToRemove) {
	if (oldCards.remove(c)) {
	if (views == null) {
	set(key, null);
	} else if (views.remove(c.getView())) {
	if (views.isEmpty()) {
	views = null;
	set(key, null); //avoid keeping around an empty collection
	needFlagAsChanged = false; //doesn't need to be flagged a second time
	}
	else {
	needFlagAsChanged = true;
	}
	}
	if (oldCards.isEmpty()) {
	oldCards = null; //avoid keeping around an empty collection
	break;
	}
	}
	}
	if (needFlagAsChanged) {
	flagAsChanged(key);
	}
	return oldCards;
	}
	CardCollection clearCards(CardCollection oldCards, TrackableProperty key) {
	if (oldCards != null) {
	set(key, null);
	}
	return null;
	}

should be moved to GameEntityView to be used with AttachedCards
or maybe even directly into TrackableObject

[tool4ever]

hmn, type safety can get a bit messy with these generics 🤔
maybe it should only present itself as Iterable to callers?
though I'm not certain that wouldn't just make it more messy somewhere else...

[Hanmac]

maybe it should only present itself as Iterable to callers?

yeah that what i was thinking too, the functions doesn't need to know that they are TrackableCollection on the inside, so i might not need a TrackableCollection.Empty variant

i don't have a problem with Tracker Properties to be null inside the Tracker,
but this functions setting the Game Properties to null when empty can be annoying,
i will try to debug how much it affects this

hmn, type safety can get a bit messy with these generics 🤔

You mean for the addCard/removeCard ones?
i want to keep them that way for now, no need to make them Type generic.

i will try to make bit of the collection cleanup in another MR

[Hanmac]

@tool4ever found the piece again i have been looking for:

forge/forge-game/src/main/java/forge/game/player/PlayerView.java

Lines 98 to 100 in 89af34e

	public FCollectionView<PlayerView> getOpponents() {
	return Objects.requireNonNullElse(this.<FCollectionView<PlayerView>>get(TrackableProperty.Opponents), new FCollection<>());
	}

Something like that on the View classes for the get methods, to return an empty List?
(I need to check if it needs to be an FCollection or if a normal Collection is enough)

[Hanmac]

@tool4ever with #10801 I probably don't need to touch the default value for collection