Someone asked me how to remove unused images from modules again, which
I usually offer to help module designers who aren’t using UNIX to do.
It occurred to me that if, instead of doing that, I’d added the purge
feature to VASSAL the first time someone requested it, not only would
we have the purge feature, but it would have taken me less time overall.
So, since I’m waiting for replies from people right now and can’t do
non-Linux troubleshooting myself until June, I decided to do it.
Quite a while ago, Brent said on this topic:
What I realized after a few hours of frustration is that this can’t
work, becuase not every Buildable is an AbstractBuildable, and so
it’s impossible to retrieve any file-use information from any subtree
of the build tree which has a non-AbstractBuildable as its root. Case
in point: BoardPicker is not an AbstractBuildable, and it tends to have
some important image-using children.
I’m uncertain how to proceed. The alternatives I’ve thought of are:
Add a getImageNames() method to Buildable.
Create a tool which operates on the XML rather than via the build
tree.
I’m not fond of either solution. (1) would break all custom code which
defines Buildables which don’t inherit from an existing Buildable, and
Buildable is a very spare interface—I’m not sure such a specific method
belongs there. (2) is not really possible right now, because of the way
GamePieces are stored in the buildFile. A GamePiece knows how to decode
that mess, but unfortunately only a GamePiece knows how to decode that
mess. For all other build elements, all we’d need to do is specify which
attributes contain filenames (but this should be done inside the Buildable,
right, becasue that’s where the attributes have semantic meaning? So then
we’re back to the problem with (1) again). If GamePieces were stored like
the other build elements instead as a block of data, then maybe just
parsing the XML would have a chance of working…
Are there really many cases of this that are significant? I had similiar issues when I did the i18n work and converted a few of them to AbstractConfigurable.
GlobalMap is another funny, it implements AutoConfigurable.
The number of these is very limited and I can’t see us creating more. You could code around these as special cases. BoardPicker doesn’t have any images itself, so Map can just skip it and go directly to the boards. GlobalMap can also be coded for specially in Map.
I still think this will be the cleanest way to do it if you can code around these ‘funnies’.
An XML parser that’s external to the component classes won’t work, as you say. Rather than adding the method to the Buildable interface you can define a new ImageContainer interface. AbstractBuildable can implement it, along with any other classes like BoardPicker. This is pretty much what Brent did for the i18n work, and it works pretty well.
I don’t see how the proposed solution addresses the problem, which is that
subtrees rooted at Buildables which are not AbstractBuildables are wholly
opaque from above.
If I’m walking the tree to find ImageContainers, as soon as I hit a
non-AbstractBuildable, I can’t find out what it’s children are. If
I can’t do that, then I can’t be guaranteed of having a complete list
of required images. But that means, for the images I haven’t found
this way, I can’t tell whether anything uses them, and hence they
shouldn’t be removed on pain of breaking the module. As a result,
the sole case in which we can remove images for the the archive is
if the build tree contains only AbstractBuildables, which is
impossible for a functioning module.
Maybe I haven’t understood what you’re suggesting?
I thought about this some more at supper, and had two ideas:
This should not be restricted to images. We should be registering
all files in use (e.g., audio, HTML, etc.), it just happens that
the most common file type to end up unused is an image.
This problem could be solved from the outside by adding two methods
to Buildable, getChildren() and getParent(), in addition to adding a
RequiresFiles interface to any Buildable which requires files from the
archive. That way, it would be possible to walk the tree from the
outside and ask each node whether it RequiresFiles. (Is this what you’re
suggesting above? If so, I don’t see how it can be done without having
a way to walk the tree.)
This problem is going to crop up any time we need to find a property
of (a subtree of) the build tree rather than just a property of some
individual node to which we have a reference. Adding these two
methods would be an easy and general way to address the problem.
True, but any component should know whether it can contain components that use images (e.g. BoardPicker). It’ll be easy for us to write that implementation for any classes in the core engine library. We can accept the downside that custom-written classes might fail to implement the interface. The reason it’s better to create a new interface is that if we add a method to the Buildable interface, then custom classes written to implement the old Buildable interface will break when run under the new version. That’s much worse than simply failing to expose their images for clean-up.
I’ve often been tempted to add getParent() and getChildren() to the Buildable interface, but I think it’s best to resist, again for the reason that it would break all the custom-written classes that implement the interface directly.
You’ve hit on exactly why I want to add those two methods: I think it’s
better to break all of the custom Buildables once, in a predictable way
where we can say exactly how to fix them, rather than having this problem
happen over and over and crop up in unpredictable ways.
There’s no way for BoardPicker to know whether someone has subclassed
one of its children in a way which alters its file requirements. So
we’ll end up with one of two situations: (a) the module designer will
also be stuck subclassing BoardPicker just to get it to pick up that
information, or (b) the module designer won’t realize that he needs
to, and will end up with unexpected results when purging images,
results which he’s unlikely to connect with the BoardPicker problem.
This is exactly the kind of thing I’d like to avoid, and it’s going
to keep happening this way witout a way to traverse the tree.
What we are in effect doing here, is building a garbage collection
algorithm. And one of the fundamental issues that needs to be done is
to have a reliable way of telling what items are still in use. If the
images aren’t exposed, then there is no way to notice that they are
being used. So the failure mode will be to remove images that are
still needed. That will also break the module.
If the resulting problem was that there were unused images that were
mistakenly left in the module, it wouldn’t be as bad a failing. That
results in some unnecessary bloat, but doesn’t break anything.
Instead, we would end up purging images that are being used, because
we can’t find out how is using them.
Yes, this is precisely what bothers me. We can’t reliably purge a file
unless we know that nothing uses it. If there is any component from
which we can’t get an answer about what files it requres, then ther
is no file which we can purge, as that component could potentially
be using all hitherto-unused files.
If the image purger removes files which should be in the module, then
that’s a problem, which, as they say, keeps on giving. It will haunt
us until we fix it. Adding two methods to Buildable will cause breakage
now, but then the problem will be solved for good.
Of these, KeyBufferrer, MenuDisplayer, and StackExpander subclass
MouseAdapter; Chatter, HidePiecesButton, and PrivateChat subclass
JPanel; and the reamining ones subclass Object.
For the ones which subclass Object, I suppose they could be changed
to subclass AbstractBuildable. I don’t see how that could screw up
anything, as they’ll still continue to be Objects.
For the ones which subclass MouseAdapter: MouseAdapter is just a
convenience class, all of its methods are empty. Nothing should
be relying on these to be MouseAdapters, but rather to implement
the various mouse interfaces which they do. So probably these
could also become AbstractBuildables without breaking anything.
The ones which extend JPanel are problematic. I’m inclined to say
that this was a mistake—that the JPanel should be held internally
instead. It’s unlikely that anyone has subclassed Chatter or
PrivateChatter, but someone well might have subclassed HidePiecesButton.
Of course, if we switch these all to be AbstractBuildables, then
the following question arises: What’s the point of having the Buildable
interface at all? It wouldn’t ever be correct for it to be instantiated
by a non-AbstractBuildable, so…?
There are two concepts that we should keep separate:
The Buildable interface is about serializing a Java object to/from XML. The other concept we’re discussing is traversing the Module hierarchy in order to get information about it. I18n info is one such set of information; images used is another; another example is the Validation logic (the stuff that throws up a warning if you create a Map without any Boards, for example). All of these concepts are represented by their own interfaces. That makes it possible for people writing their own classes to opt-in to these features or not. Maybe a commercial developer is making a module that is never intended to be loaded into the module editor, so purging images is a non-issue. Maybe some day we’ll want to replace the XML serialization with one of the more modern XML-to-Java mappings. Having a bare Buildable interface that assumes nothing is what will enable that possibility. The more we clutter that interface up with methods that make assumptions about the object hierarchy or functionality of the classes underneath, the harder it will be.
If we really want to generalize the ability to traverse through the module and collect information, maybe we can introduce an IterableComponent interface (or something) that has getParent() and getChildren() methods. Again, let AbstractBuildable implement it for convenience reasons.
My point is precisely that global build tree information cannot be
gathered reliably if the interface it uses is opt-in. I don’t have
a problem with the Buildable interface per se, I’ve now realized;
see my last comment below.
That doesn’t address the problem, by itself. There would still be no way
from the root to reach IterableComponents which have non-IterableComponents
as ancestors.
Now, if every IterableComponent threw an exception on the attempt to
assign it a non-IterableComponent as a child, that would address the
problem, and still leave designers free (in a Henry-Ford-Model-T-color
sort of way) to create non-IterableComponent Buildables.