[jsword-devel] Patch for misc improvements

[Joe Walker]

Applying, thanks - notes inline:

DM Smith wrote:

> I am attaching a patch that does the following:
> 1) I removed getKeyList from KeyUtil as it merely cast a VerseBase to 
> a Key, which it already was. I then used the VerseBase directly.

I'd left it like this because I thought it was a useful marker in case 
we wanted to stop VerseBase being a Key. But I think you are right in 
doing this.

> 2) I made PassageKeyFactory a singleton. It already had a static 
> instance of itself. I added the default constructor as private, and 
> added an instance() method for getting the singleton.

This makes sense, but see note on 5

> 3) I made all variables that stored a PassageKeyFactory have the base 
> class type of KeyFactory and only in the few places where 
> PassageKeyFactory extensions were needed did I cast it.

Good, I've been trying to get rid of PKF. See note on 5.

> 4) I removed cleanInvalidCharacters from XMLUtil and the places 
> (filters) that used it as this behavior happens upon reading the text 
> before it is handled to the filters.
>
> 5) I changed:
>         Key key = index.findWord(null);
> to:
>         Key key = PassageKeyFactory.instance().createEmptyKeyList();
> The former would throw a NPE if index were null. Which was the case in 
> the test code. It turns out that this was merely a way to create an 
> empty key list. So the change makes the code clearer.

It's not just PKF that I'm not keen on. I see Keys as depending on the 
Book that created them. So really we don't need a KeyFactory - we just 
need a Book (which inherits from KF anyway).
The advantage of the index.findWord(null); style is that it gets the 
empty Key from the book and does not make the assumption that the Keys 
are Passages. I think a search engine indexing a non-Bible could break 
as a result of this. If you don't mind I'm not going to commit this 
until we've debated it a bit more.

But the index.findWord(null) idiom is very poor as you note. I guess we 
need to add a method to index to create an empty Key. So I'll add a bug.

An on a similar vein, I wonder how many times KeyList crops up in 
methods that ought to be Key.

> 6) I added COPYRIGHT to ConfigEntry. It is documented on crosswire as
> being a valid key in a conf file.
>
> 7) I fixed a bug in GZIPBackend where changing from Genesis to Matthew
> would result in Genesis being shown as the the text for Matthew. The 
> problem was that both used the same block number (i.e. chapter 0 in 
> their respective testaments.) Testament needed to be considered along 
> side of the block number. In addition, I saw that there were some 
> questions as to the value of the indexes and there were some 
> misleading comments about them. I studied the code, the indexes and 
> added significant comments to the code. I also changed variable names 
> to agree with the actual purpose of the variables.

Good one. That's been one of the bugs that's bitten me every now and 
again and I've not seen the pattern.

> 8) I added a routine to filter RTF data in the conf files that shows 
> up in the about field. The result is that the display is significantly 
> better. I also fixed a bug that was stripping blank lines that should 
> be kept.
>
> 9) I changed some logging levels to be better aligned with what was 
> being logged. This allows me to selectively turn off some logging and 
> see information that is appropriate. I think debug should show 
> extended data, warn should show recoverable problems, error should 
> show non-recoverable problems. I don't know what the distinction 
> between info and debug should be. So I didn't mess with these.

Thanks. On the split between debug and into, I see the log file mostly 
as a fault diagnosis tool, so users don't generally need to see it. Some 
logging could be useful in live (the info level) and some logging you 
want turned off in live (the debug level)

> 10) I commented out of config.xml those items that are not used by the 
> code. It was referencing stuff in limbo and stuff that bibledesktop 
> does not use.

Thanks, I'd forgotten about this.

> 11) In core.xml I removed limbo code from the built jars. This 
> requires the stuff that should not be in limbo to be moved.

I think I did exactly the same thing, I'll check.

> 12) I removed the concatenation of an empty string to a variable, when 
> that variable could be used directly. There are still more places that 
> this can be done. I do it when I bump into it.

I used to be lazy and see ""+obj as a faster way of typing 
obj.toString(). Oops.

> 13) I changed some arguments to be more specific. E.g. in 
> AbstractPassage, toVerseArray took an Object when it always was passed 
> a VerseBase. I changed it to VerseBase.

Good one. I'd not noticed that. toVerseArray() probably preceeded the 
VerseBase concept and copied the toArray() idiom.

> 14) I updated bugs.txt to include some other things I found. Please 
> reclassify if I placed the todo in the wrong place of the roadmap.

Thanks,

> 15) I was getting tired of seeing warnings about code in limbo, so I 
> change the code to get rid of the warnings. Mostly it was adding 
> //$NON-NLS-1$ comments.


Thanks - a cool list of updates.

Joe.