Working Projects

A COMMENT OVERALL

It is very helpful for us reviewing the code if you check these in in small pieces and not one massive bulk update. That is one reason why I suggest incremental changes.

Token Rebuilds to Interface

What is the situation today?

Our loading tokens are using a form of commitment process (evaluate then commit or rollback) much like a SQL database.

This process today uses things like AbstractObjectContext (which inside it has an ObjectCommitStrategy) in order to do this level commitment process.

Why do we do this?

1. Primarily, if there is a token that has a bad syntax, we don't want a partial data structure to get into the core code. This causes a tremendous amount of "what if" scenarios in our code that means folks (a) write a ton of error checking code (b) catch and silently consume errors
2. We want to catch LST errors at load and report that ASAP to the data team. If they have to actually USE a token in order to make sure it is valid, that is a HUGE testing burden on the data team.

This commit process has been VERY valuable (we want to keep it!), but also challenging. There are a few limitations we want to get rid of:

1. This forces us to only do things that ObjectCommitStrategy knows about. There is a huge centralized code dependency (high workload, fragile)
2. This forces us to remember never to write to a specific object, but always through the Context (annoying, fragile)
3. This forces a design where "everything is a CDOMObject", which creates a form of demi-god object (PlayerCharacter is still our "God Object" ;)

Where do I see this stuff?

1. The tokens are in plugin.lsttokens
2. The contexts are in pcgen.rules.context
3. Some abstract items are in pcgen.rules.persistence.token

An actual example of how the token loading works is in pcgen.rules.persistence.TokenSupport.processClassTokens(...)... that will return success or failure and capture any messages in case of failure into a set of parsed messages.

The actual commit/rollback process can be seen in pcgen.persistence.lst.LstUtils.processToken(...)

What is the solution?

The best way to get around this is to "properly" use reflection, which was really not a thing comfortable for many developers in about 2006 when we started down the token path. Thankfully this can be abstracted into a library, so the reflection is well hidden from most users/developers.

The reflection code is StagingInfo, and it exits in our Base PCGen library (pcgen-base repository) in pcgen.base.proxy.

Is an example in the code today?

Yes. This staging is used for a limited number of tokens and you can find the process in pcgen.rules.persistence.TokenSupport.processInterfaceToken(...)

What is scope of work?

At this point, some folks may be thinking things like "Hey - I can have a getAlignment and putAlignment method on Deity.class!". That may be literally true, but there are a few considerations to that:

1. One step at a time
2. There are things outside the scope of this description that are also changing and make the first step as described here valuable and a dramatic step to completely eliminate the abstraction in CDOMObject to be potentially undoing other things we value

The scope of work here is to change indirect calls through an object context, e.g.: context.getObjectContext().put(deity, ObjectKey.ALIGNMENT, al); ...into direct calls on the object, e.g.: deity.put(ObjectKey.ALIGNMENT, al);

This directly meets requirements #1 and #2 above.

In order to do this a few things need to happen:

• Read and write interfaces need to be defined, so they can be passed to a StagingInfo

It is my belief that the interface would work best right now if the scope was a single behavior. So the put and get just for ObjectKey.* would be processed separately.

• The tokens that are using ObjectContext to use those methods need to call items directly on the object
• A method needs to be defined that will appropriately limit usage of the object

  This may require a bit of explanation. Let's use plugin.lsttokens.deity.AlignToken. Today, this keys directly off the object, Deity, using CDOMPrimaryToken as the interface (the parent interface CDOMToken actually defines how TokenSupport will process that token).

  If we create interfaces ReadObjectKey and WriteObjectKey, those can be reused across all of the same types of methods, but it places us in a situation where we have lost the ability to enforce ALIGN:x only works on a Deity. So there is a balancing act here:

  We probably don't want to have interfaces directly for each type of object (readDeity and writeDeity), as that creates interface proliferation (and we want to get away from that level of specific behavior anyway)

  We probably want to think about how we can have tokens actually work on Dynamic objects - this relates to (A) above... the dynamic objects are all the same class, and thus a different validation method is necessary than one based on class or the interfaces implemented by that class. (see pcgen.cdom.inst.Dynamic)

• It is likely that a new interface beyond CDOMPrimaryToken / CDOMInterfaceToken needs to be defined. It is possible CDOMInterfaceToken could be enhanced with a default method that always passes, but could be overridden with a method that would check for legality from #3 above in cases where it is necessary.

What is one possible path?

WARNING: Any mixed tokens that still use the context probably don't want to be converted, so two of these actions will be reverted at the end

1. Take the import for ObjectKey out of ObjectCommitStrategy and find the errors
2. Delete all the methods that are then errors from those methods being removed from the interface
3. A whole bunch of tokens now have errors. Those need to be checked. If they are just a simple use of the interface then:
   1. changed to direct calls rather than through the objectcontext
   2. converted to the new load method (not CDOMPrimaryToken)
   3. Have the limit from #3 (limit where it is legal) added
   4. If they are complicated (FavoredClass), then I'd leave as-is for the moment.
4. Revert the changes made in #1 and #2

Lather, rinse, repeat for ObjectKey, IntegerKey, ListKey, StringKey, FactKey, FactSetKey

You can skip VariableKey, FormulaKey, MapKey for now.

Eliminate Facades

What is the situation today?

There are a series of Facades (e.g. RaceFacade) that provide an interface on the core object for use by the UI.

Why do we do this?

1. Primarily, the facades are isolation of behavior between the core and the UI.

This design has a few limitations that we want to get rid of:

1. This forces the UI awareness back into the core, forcing the core to have methods from the Facade Interfaces it doesn't really "want" (and are only there to serve the UI)
2. This is very design-specific. Every behavior (Race has Hands) is a game-dependent piece of information that makes PCGen less flexible. While we primarily handle d20, it still is a tight tie that makes it hard to get the UI dynamic to the data.
3. This is removing a level of indirection that is necessary preparation for later work that will completely change how we access information in the UI (see below)

Where do I see this stuff?

1. The facade interfaces are in pcgen.facade.core

What is the solution?

For now, this step is to delete the interfaces and directly use the objects. The strategic direction is to use channels. See pcgen.output.channel

Is this shown today?

Yes and more. PC Alignment is now controlled through a channel... but that setup is more than this step is asking at the moment.

What is scope of work?

1. Delete the WhateverFacade and directly use the object to get information.

Good places to start: RaceFacade, DeityFacade, HandsFacade, GenderFacade

Create Channels/Delete Facets

What is the situation today?

There are a series of Facets that store the primary information for the model storing the PC. These are in pcgen.cdom.facet

Why do we do this?

1. The Facets are small containers for specific sets of information about a PlayerCharacter. These were part of the process of reducing the "god object" behavior of PlayerCharacter.

   In a larger picture, Facets are going away. There are different types of facets, but in general, the new formula system can take over most of the behaviors of facets. (There are exceptions we will eventually address).

This design has a few limitations that we want to get rid of:

1. The facets are still operating in a model where the UI has to go order the core to do things. This is *really* challenging for things that have interacting behaviors. Gender, for example, is usually PC controlled, but there are some (cursed) magic items that can override it. This really means our solving system (the formula system) needs to be UI- AND object-aware, and we want it to do the resolution of what the current situation is on a PC.
2. We want to pass control of ALL of these items that touch the new formula system over to the data. Eventually, the data should know the variable is "PCsAlignment" and then the HTML (or whatever) that displays the PC should be told it's "PCsAlignment". In this way, the variable name from the data can match the HTML and NO code modifications are necessary to change what or how it is displayed. This completes a significant portion of our "separation of duties" to reduce the amount of code change necessary for minor items in game systems, and allows us to focus on larger issues.

Where do I see this stuff?

1. The facets are in pcgen.cdom.facet, more specifically the ones relevant here are mostly in pcgen.cdom.facet.model

What is the solution?

The strategic direction is to use channels. See pcgen.output.channel

Is this shown today?

Yes. PC Alignment is now controlled through a channel. You can see some compatibility work (aka temporary code) in pcgen.output.channel.ChannelCompatibility

What is scope of work?

Step 1: Move from using the facet to using the new formula system, while Enabling Data to control the channel/variable. This actually takes some interacting work with data.

1. We need to set up a system that tells us whether we (code) control the variable name for an item or whether data has asserted IT controls the name. These are called "CodeControls" (see pcgen.cdom.util.CControl for the names, such as ALIGNMENTINPUT)
2. If we control the name, we need to create the channel. See SourceFileLoader.defineBuiltinVariables for how alignment works.
3. Depending on the complexity, we may need ChannelCompatibility items. Arguably, the Alignment work is not complete, but it is at least functional.
4. Delete the facet. There may be other facets using the one you deleted. If so, you will need something like: In PlayerCharacter.doFormulaSetup, you will need soemthing like: VariableChannelFactoryInst.watchChannel(id, "DeityInput", activeSpellsFacet);

If there is a priority [a number] when the listener is set up, you will need to use that as well - please see the other watchChannel method in VariableChannelFactoryInst

1. If there is a new format involved (things like DEITY), then you probably need to work with the data team to have that variable defined in all data modes OR you need some method for figuring out where it is/is not legal.

Step 2: Unwind most of CharacterFacade (a bit of a concentrated set of behavior), and reduce the breadth of usage of the pcgen.facade.core.CharacterFacede object within the UI. (.getCharID() usage is good, others to be eliminated)

Note: Step 2 in some cases may be VERY hard. Alignment is an example, where changing Alignment can have side effects. We don't currently have a great design for that scenario, in that checking for qualification of the alignment, checking EX_CLASS, etc. are not behaviors we yet have a design to handle in the formula system.

Move UI to Dependency Injection model

What is the situation today?

Currently, there are a set of models in the UI that are swapped out for each character. These are done passively on swap, and force many items to have knowledge of CharacterFacade.

Why do we do this?

1. We only want to build UI items once, and swap the model underneath

This design has a few limitations that we want to get rid of:

1. We want to move toward a "dependency injection" model
2. We want to lower dependencies on runtime calls up and down a stack (there are classes in a tangle that DI can eliminate)

Where do I see this stuff?

1. pcgen.gui2.tabs for the most part

What is the solution?

Convert these items to dependency injection and eliminate the model swapping effects in the UI.

Is an example shown today?

No.

What is scope of work?

Note: Strictly the new method in Step 1 of Phase 1 is not necessary, although it may help you identify places to look. Longer term it may make sense either to not do the new method and place things directly into the initComponents method, or to do the two phases below and then inline those initializeStaticModel methods at the end. Either way, the target here is the createModels, restoreModels and storeModels methods.

Phase 1:

1. Start with code/src/java/pcgen/gui2/tabs/CharacterInfoTab.java ... Add a method: public void initializeStaticModel(Supplier<CharacterFacade> supplier);
2. Add an empty version of the method above to all classes that implement CharacterInfoTab
3. pcgen.gui2.tabs.InfoTabbedPane needs to be modified. It:
   1. Owns the Supplier
   2. Must call the method above in initComponent()
   3. May need to initialize the Supplier object and the value of the Supplier in its constructor (depending on how the Supplier is written)
   4. Note: Null must be a legal return value for the supplier, and it should be initialized to null.

Phase 2:

1. For each of the classes where initializeStaticModel was added, look at the following method: public ModelMap createModels(CharacterFacade character)
2. For each of the values of a put statement adding to the ModelMap, do the following:
   1. Evaluate if that subclass needs the CharacterFacade. If so, provide that subclass the supplier in the constructor to that object and alter the class to use the Supplier.
   2. Delete the put, rather taking the construction of that object (which now uses the supplier) and place it into initializeStaticModel.
   3. Delete the "install" in restoreModels(...)
   4. Delete the "uninstall" out of saveModels(...)
   5. Delete the "uninstall()" method on modified subclass
   6. Take the items from the "install()" method on the modified subclass and add those to initializeStaticModel (correcting for the difference in location)
3. It is almost certain you will run into side effects where the Supplier will need to be passed into a constructor at some point - this is the idea, and part of DI.

WARNING: For your own sake, do these in small pieces, one target at a time. You will rapidly find a few (QualifiedTreeCellRenderer comes to mind) that will have a wide ranging impact.

UI Consolidation

What is the situation today?

QualifiedFilterHandler is (to within generics) identical between Race, Template, Domain, and potentially other tabs.

The same is true of the XInfoHandler methods in various tabs.

The same is true of availPanel and selPanel, built in the initializers of the classes for various tabs.

Where do I see this stuff?

pcgen.gui2.tabs.* QualifiedFilterHandler is, today, an inside class

What is the solution?

Extract one of them into its own class, make them all shared, slightly expanding generics if necessary. For the XInfoHandler classes, you may have to construct it with a few things, but still consolidation is possible For the Avail/Sel panel items, create two new classes, the boxes, glue, addAction and lots of other stuff can be isolated from each of the tabs.