11861 - Editing PropertySheet trait shouldn't wipe its state

[Cattlesquat]

Fixes #11861

@BrentEaston See what you think -- a short version of what's described in the attached issue is that when you edit the PropertySheet trait in the piece definer, it wipes all the contents you've put into the state (the values of the properties themselves), because the way only way to preload values into the properties is to use the edit-the-preview-piece power in the PieceDefiner, and normally PieceDefiner effectively wipes the state of any individual trait that gets edited.

My proposed fix is just to exempt PropertySheet from that wiping behavior -- an edited PropertySheet trait will just keep its current state. I don't think this can cause any particular harm, and even if maybe the results aren't perfect if you delete a property at least this improves the most common case?

(EDIT: See #11861 for discussion of what happened on testing. Not sure where to go with it...)

[Cattlesquat]

See #11861 for discussion of what happened on testing. Not sure where to go with it...

[Cattlesquat]

@BrentEaston I think this one is notionally waiting for you to take a look at other options.

[BrentEaston]

There is a problem with this approach. The question is, why does this work for all other traits? The specific problem I found is that if you remove a row from the property sheet, then all of the values get shifter up. Try creating 3 rows x, y and z and give them values 1, 2 and 3. Test. Then edit and remove line y. Then test again and z now has the value of 2 instead of 3.

I think the problem is in the Propertysheet, not the PieceDefiner. The 'shape' of the ProprtySheet trait is dynamic, it depends on the number and order of the defined rows. I think the bug is in PropertySheet.Ed.getState() which is blasting away the existing state and returning a default set of null values. This should be returning the actual state recorded in the pre-edit piece, less deleted rows + added rows.

[geezer09]

This problem is actually not limited to the property sheet trait. I think the problem is more generalized and that the piece editor does not handle the state of various properties very well. In this particular instance, why is it not possible to set the state of the various properties directly within the property sheet properties windows, the one where the list of properties is generated? Seems like that would make sense instead of having to close that window and then clicking on the piece to do it "manually". In my opinion, it should not be possible to change the state of the gamepiece via the gamepiece itself. This leads to many issues and inconsistencies. This is somewhat related to my issue #13665

In any case the problematic code for this one particular instance is here propertysheet.java

@Override
public String getState() {
  final StringBuilder buf = new StringBuilder();
  buf.append(
          String.valueOf(STATE_DELIMITOR).repeat(Math.max(0, propertyTable.getRowCount())));

  return buf.toString();
}

This method is not named properly and called by the piece editor to get the state of the piece. The method just creates an empty state instead of actually getting the state. In a way this is a much safer thing to do but leads to the issue at hand.

Easy fix)

Pass the state to the private static class Ed within propertysheet.java. If no properties were added or modified, return this state in getState(). If any changes were made just return a blank state as is currently being done.

Proper fix)

Allow user to set those states directly in the window where properties are added and removed. Then we can modify getState() to always return the correct state.

see #13691 for implementation suggested