I was looking for a way to remove unused images from a module. Having missed the final posts in vassalengine.org/forums/viewtopic.php?t=814, I wrote a small Java utility that deletes any image file that is not found in buildfile.
Up to now, it searches for the image filename without extension in the buildfile. If it finds it, it marks it for deletion. I mainly used the ArchiveWriter class to perform the reading, uncaching and saving of the module, and it seems to work, so far. For searching buildfile, I basically copied the buildstring() method found in GameModule.
Right now it is standalone, but if the maintainers were interested, I could add it to (or rather, try to code it into) the module editor.
I have been looking at the vassal code. At first I thought about looking for the strings “icon” or “image” in the property names of the AbstractConfigurable children of a module, since I thought it could be simply implemented, then I realised that basic pieces don’t extend AbstractConfigurable. Is there a proper way to fetch a list of images used by a game piece, including its properties, like layers?
Related: I have been looking to the image types supported by vassal, and I have come out with the list:
image/png
image/jpeg
image/x-png
image/vnd.wap.wbmp
image/gif
image/bmp
Svg is handled as well by the SVG utils, so any reference can be solved. I was wondering if this list was complete.
Since all the previous image types are single files with no external references (except svg, whose references can be solved), is it not safe to assume that any image not appearing in the module structure is unused?
No, not at present. The only way to do this properly is if every component
is modified to report on what files it depends. (There’s a discussion here
about that from about a year ago, where I wrote code to do what your cleaner
does and reached the same conclusion as you have.)
Since adding this facility to AbstractConfigurable (actually, it should
be part of AbstractBuildable) would break all custom code, I’ve been holding
off on doing it until we have the new module library on the new site ready,
since at that point we’ll be in a better position to notify module maintaners
of these kinds of changes.
How’d you come up with this list?
No. It’s possible for a custom component to have a hard-coded image filename.
It’s also possible to construct image filenames from the values of variables.
This approach cannot work reliably.
What about checking the build string of each component against every file in the image/ directory? This is basically the rough method I used for my own needs, and it seemed to work with the images that are stored in buildfile, and that are not result of variables.
I used javax.imageio.ImageIO.getImageReadersByMIMEType() which returns all available readers (at least, I think).
I see your point. However, if this tool was to provide a list of unused images to the module designer, providing the option to delete unwanted images, wouldn’t it be more reliable? What I had in mind was a window opened from the Editor that listed the “unused” images; the designer could then select which ones to delete. I was not thinking about a fully automated (and dangerous) function.
I finished writing the code for a primitive version of a cleaner integrated in the editor. Basically, it is just two classes: the cleaner and its interface. So far it does not control the file type before deletion, so it does not support svn, but that is easily fixed.
If anybody is interested I can upload it to svn, but I need to be given permission to do so in sourceforge. Whom shall I ask to?
Actually, I used the 3.1 branch on svn as a start, since I was unable to get the trunk to run. If I am not mistaken there were some remarks on this forum that 3.1 is a better starting place to tinker with.
I had a very brief look at your code just now. Five things which jump out
at me:
Please use List instead of Vector. There’s no need to pay the
synchronization cost imposed by Vector when everything is happening on
one thread. Anyway, Vector is semi-deprecated; if you need a synchronized
list, you should use Collections.synchronizedList() to wrap an ArrayList.
You have a bunch of hard-coded strings in the GUI part of your code.
These need to be I18N’d so that our translators have a chance to translate
them. (Use Resources.getString() and add keys to VASSAL.properties.)
We’re trying to move our GUI to use MigLayout in order to get the correct
spacing around and between widgets. You’re using BorderLayout in your dialog.
I don’t want to merge new code which does layout the old way, as that just
digs us a deeper hole. (I can show you what needs to be done here, if that
would help.)
ModuleCleaner should probably do its work on a background thread, so as
not to block the GUI. Again, this is one of those issues for which we’re
in transition right now—we have lots of code which presently runs on the
EDT, but shouldn’t, and I don’t want to add new code which does the wrong
thing. (And again, I can show you what needs to be done here.)
You don’t write many comments. We like comments. This isn’t terribly
complex code, but a few comments can help our (and your) comprehension of
what’s happening. Also, putting in the Javadoc would be nice.
Done. I used Vector<> because I am mainly a matlab programmer , so I thought: “vectors must be good”. Changed to ArrayList.
Done.
Done. I kind of like the current look, but I am open to suggestion
Actually, I locked the GUI on purpose, it was to avoid the desynching of the list of unused images to the actual contents of buildfile.
Done, to some extent: I added javadoc to all public methods and all classes. Sorry about the lack of comments, but I hoped that the names of methods and classes as well as the program flow were clear enough. I must admit, however, that they are not. Speaking of comments, are the beginning comment, LGPL and copyright notices to be added manually?
Thanks .
Right now I have trouble with the interaction between ImagePicker and ModuleCleaner: when ModuleCleaner.deleteImages() removes the images, they are stored in ArchiveWriter.removedImages. However, ImagePicker calls DataArchive.getImageNames() to retrieve the names of the images to display in the picker. As far as I know this is the only call to getImageNames() and getImageNameSet() in DataArchive.
Pointing to ArchiveWriter could allow ImagePicker either to avoid displaying the removed images or to restore them.
I’m happy to give feedback to people who volunteer to write code. We
badly need more coders. (In case Ben is reading this: Yes, I will
get to those patches of yours…)
Hashtable is in the same boat, BTW. Use HashMap instead.
Good. Thanks.
I’ll try to take a look at this tonight.
The problem with doing it this way it that it also blocks repaints, not
just user input—and so the app will appear to have frozen, despite that
it’s just doing some processing.
A better way would be to provide a progress bar which is updated by a
SwingWorker.
Thanks.
Yes.
I’ll have to think about this. ArchiveWriter and DataArchive are a Big
Damn Mess. (I’m about halfway done with writing code to replace them
for 3.2. If you’d like to help with that, let me know…)
I am looking into it right now. Besides, to what version of jre shall my code be compliant? I have 1.6, but so far I have been using 1.5 so far (1.5.0_17). Is that correct for 3.1? What about 3.2? I am asking because I have seen that SwingWorker is included in JSE6, while Vassal has a swing-worker-1.2.jar in lib/ (I found the API at swingworker.dev.java.net/nonav/ … index.html)
Well, I am willing to help. Keep in mind that you have seen more or less a quarter of all the java code I have written. I am more of an “old” kind of programmer (function oriented). While I did some forays into OOB, I programmed mostly in C (no ++) and Matlab. In which branch are you working on this?
Our minimum requirement for the JRE is 1.5. The reason for this is that
we have a lot of users right now on versions of Mac OS X for which there
is no 1.6 JRE. Someday, I expect that enough of those machines will be
replaced that we’ll be able to move the minium to 1.6 (and I look forward
very much to that day) but it’s still some ways in the future. We’ll still
be supporting Java 1.5 for VASSAL 3.2.
All of the code we release is built with the ‘-target 5 -source 5’ flags,
which ensures that we produce 1.5-compatible class files. I build releases
using an OpenJDK 1.6 compiler. I recommend doing all of your work with a
1.6 JDK, if for no other reason than it will have fewer bugs and have
better performance.
Regarding SwingWorker: The swing-worker-1.2.jar is a backport of SwingWorker
from 1.6 to 1.5. The API is the same, so the only thing we’ll need to do
when we finally move to 1.6 is to change a bunch of imports to point to
javax.swing.SwingWorker instead of org.jdesktop.swingworker.SwingWorker.
Understanding how to program in the abstract is most of the job anyhow;
the rest is just syntax.
In a branch were exactly zero of what I’m talking about is committed right
now. I’ll try to get this stuff into the repo soonish, so that we can
collaborate in it.
Here’s a brief summary of what I’m doing, but first, some background on
why:
We’ve historically struggled with high memory consumption from large map
images. A few weeks ago, I found what looks like a solution to the problem:
If we load map images by slicing them into tiles the first time a module is
loaded, and then caching on disk the raw image data, we can rapidly load
large images without using much memory. This part is coded already and works.
The problem is that in order to maintain the disk cache, we need some
information which DataArchive isn’t set up to provide. E.g., suppose someone
is using the Editor on a module and changes one of the map images. Then we
need to be able to tell the next time the module is loaded that the chached
tiles for that image are stale and need to be rebuild. To detect this
condition, we need to know the size of the image file, its modification time,
etc. But DataArchive gives us now way to get at that information.
Now, there’s a very nice API for file systems being introduced in Java 7.
It solves all of the problems with DataArchive quite nicely. It’s not such
a huge API that we can’t create our own implementation of it, and that’s
just what I’ve started to do.
The home page for the NIO API I’m talking about is here:
The classes we need are all under java.nio.file (with a small amount
of stuff under java.nio.channels).
I think this would make what you’re trying to do easier, as well. The
overall point of this is to give us uniform access to files, no matter
whether they’re in ZIP archives, on disk, or wherever.
So, I’ll get this into SVN this evening, and then we can talk about
what still needs to be done.
SwingWorker has proven itself a tougher customer than I suspected. I am yet unable to pass information to a ChangeListener. Still working on it. I think I might be close to the solution, though. I am building with 1.6, and the code compliance for the editor is set to 1.6.
Ok, I am still trying to completely understand how subclipse works, since it does not let me commentate on merges from 3.1 to my own.
There are a lot of examples of SwingWorker being used in the codebase;
try grepping for them. I’m guessing that you’ll find an example of the
kind of thing you’re trying to do in VASSAL.build.module.map.ImageSaver,
starting from about line 235.
Also, you should set code compliance to 1.5, if you can.
I didn’t make it quite that far last night. Possibly I’ll get to this
tonight or tomorrow.
I use SVN from the command-line so I’m only guessing about what you’re
seeing, but: A merge only gets you the changes. You still have to commit
them, which is when you have the opportunity to add notes to the log.
I managed to get it to work. getImageNameSet() returns a first empty string (“”) that my code was unable to deal with. I fixed ModuleCleaner, but maybe I should change getImageNameSet() to avoid empty strings.
So basically, the flow is something like:
Create branch xyz
commit changes to branch xyz (as many times as needed)
merge from trunk (or whatever is used as trunk)
check for collisions (update from xyz)
commit to branch xyz
Is that correct?
That’s probably a bug, getting an empty string from getImageNameSet().
I’d like to know where that’s comming from.
Something else about DataArchive which occurred to me just now will cause
you problems: There’s no guarantee that any of the image names you get
from getImageNameSet() are in the base module—they might be in an
extension instead, but DataArchive won’t tell you that… Yet another
reason why we need to get rid of DataArchive.
ArchiveWriter.getImageNameSet() calls DataArchive.getImageNameSet() which calls getImageNamesRecursively() which calls the culprit, getLocalImageNames:
if (archive != null) {
for (ZipEntry entry : iterate(archive.entries())) {
if (entry.getName().startsWith(imageDir)) {
s.add(entry.getName().substring(imageDir.length()));
}
}
}
Considering the results, and since the empty string is the very first returned, then the first entry in the archive that has images/ as a prefix is images/ itself. It can be easily fixed.
How come? As far as I know the base module cannot be aware of the contents of its extensions, in the sense that no extension is loaded when editing a module. I think it is the other way around. If images in the base module are called by the extension, they will be marked for deletion if they are not used in the module as well. But that is a hard limitation of my code, since it cannot look outside the module (and cannot know the whole set of its existing extensions).
, so I thought: “vectors must be good”. Changed to ArrayList.