[sword-devel] Sword for BPBible 1.5.12

Tue Apr 7 17:06:59 MST 2009

On Tue, Apr 7, 2009 at 5:27 PM, Troy A. Griffitts <scribe at crosswire.org>wrote:

> Ben,
>
> Do you have more specific information about the items you mention?  More
> comments below:
>
>
> Ben Morgan wrote:
>
>> On Mon, Apr 6, 2009 at 10:00 PM, Troy A. Griffitts <scribe at crosswire.org<mailto:
>> scribe at crosswire.org>> wrote:
>>
>>    Ben Morgan wrote:
>>
>>        I didn't like it either. But there were two major reasons for this:
>>        a) Creating a new LocaleMgr when stringmgr was was very costly
>>        as BPBible keeps around a sword.conf file and then all the paths
>>        in this were automatically loaded.
>>
>>
>>    This should only happen on application startup and shouldn't be too
>>    costly.  I believe loading locales should actually be quite fast,
>>    and if not, then we have other troubles we should fix.
>>
>> Well, it isn't always. I am often dealing with network drives, etc.
>> And the sword.conf files I am using can have ~15-20 entries...
>>
>
> AugmentPath entries or what?  I still don't believe loading locales should
> take any noticeable time, and if it does, I would like to profile the code
> to make it faster.  We could load on demand the locale from the SWConfig,
> just as a first thought.
>
Yes, this is AugmentPath entries. I am just going from experience on how
long it takes - I seem to remember it was taking ~0.7 seconds one time I
tried it. And it is pointless to look it up here, anyway, as I don't want
those locales...

        b) more importantly, I discovered an issue where having 1.5.11
>>        locales and 1.5.12 locales around a the same time would break
>>        verse lookup.
>>
>>
>>    Yes, having 1.5.11 formatted locales will not work with the latest
>>    code, but I don't think I'm willing to support a configuration path
>>    which has both formats.  I suppose we could add a quick check for a
>>    numeric value for the first book abbrev and not load the locale in
>>    1.6.x if so.  Do you think it would be beneficial.  I hesitate
>>    because 1.5.x will almost certainly break if it finds the new locale
>>    format files.  It's almost better just to say both formats can't
>>    co-exist.
>>
>> Yes, but they do. You can't just say they don't. I'm talking a real case
>> that I was trying that was breaking; trying to use the modules on a BibleCS
>> installation on another computer.
>>
>
> The fact remains that old locale files and new locale files cannot operate
> on the same sword configuration.  Both 1.5.x and 1.6.x will fail if this is
> the case.  We can make 1.6.x ignore the 1.5.x locales, but we can't do
> anything about the 1.5.x software already on the computer.  It will cease to
> work.  The correct thing to do is to not mix locale formats on the same
> sword configuration.  I can't see how passing hard coded paths is a better
> solution.
>
That's why I need to pass hard coded paths to make sure I don't touch 1.5.11
style locales. Otherwise locales.d folders in other module-containing places
will break it.

>
>         BPBible provides its own locales, and it is important that these
>>        are loaded, not any others. This is why I need a custom LocaleMgr.
>>
>>
>>
>>    :) I'm not gonna ask why BPBible needs its own locales.
>>
>> BPBible has extra things in the locales. It has stuff in them to support
>> dashes in booknames, and also the different filter options. BPBible needs to
>> be able to completely manage its own locales, without input from anywhere
>> else - that way we know that the locale will work properly.
>>
>
> BibleCS also has its own uilocales.d/ folder.  We augment the base sword
> locales with anything we find in uilocales.d with this call:
>
> LocaleMgr::getSystemLocaleMgr()->loadConfigDir("./uilocales.d");
>
> We don't actually replace the system LocaleMgr.
>
Well, I used to do this - until I found it was breaking. I've thought about
it since, and due to the way augmenting works (i.e. it augments the actual
locales if it finds two the same in different locations, rather than
completely replacing it, which is what I want)

>
>
>     const is important in public interface methods to advertise to the
>>    consumer what our contract is.  For example, if we have a method like:
>>
>>    doSomething(const SWModule *) {
>>    }
>>
>>    we are guaranteeing to the caller that we will not modify the
>>    SWModule.  Unless hasEntry is defined as const, then we will get a
>>    compile error inside doSomething if we try to call mod->hasEntry()
>>
>>    We can cache.  the mutable keyword is used in C++ to tell the
>>    compiler that we are allowed to change this class member even in a
>>    const method.
>>
>> OK, I didn't realise that. I tried setting it to const, and it didn't
>> work, so I gave up with it. If you can get it working, that's good. But the
>> commit that you did for linkEntry broke for at least Matthew and me - it may
>> behave differently for different compilers.
>>
>
> Nothing I changed to make members const should have modified the behaviour
> of the code.  If it doesn't work, it should have nothing to do with whether
> the methods are declared const.  Maybe I missed a missed a signature
> somewhere causing the method to not override the base class. Not sure.  Can
> you give an example of the failure, or do you see the problem?

OK, the implementation you gave for SWModule was broken - it looked at the
current entry. A replacement solution to fix this looks like this:
bool SWModule::hasEntry(const SWKey *k) const
{
    SWKey *saveKey;
    bool retVal;

    if (!key->Persist()) {
        saveKey = CreateKey();
        *saveKey = *key;
    }
    else    saveKey = key;

    setKey(*tmpKey);

    getRawEntry();
    retVal = getEntrySize();

    setKey(*saveKey);

    if (!saveKey->Persist())
        delete saveKey;

    return retVal;
}

Only one problem here - getRawEntry isn't const. I can't see how you can get
round this generically (unless getRawEntry is made const...) - this was why
I had it return -1 if it wasn't supported.

>
>     Have Chris' latest fixes also fixed your issues with these deltas?
>>
>> Sorry, I haven't tried them. I would expect they would, though.
>> I will probably disable r2287 as a matter of principal - I doubt any
>> frontend would work properly with multiple v11ns at the moment (I'm pretty
>> sure BPBible and Xiphos at least won't).
>>
>
> I believe they should come close.  Disabling them from the API won't make
> anything operate any better if your users install modules with alternate
> versification.  You'll have a much better shot at supporting them if you
> leave the registration of other versification enabled.
>
These, however, I'm pretty sure, will break BPBible. BPBible does much more
error checking of verse references, and expects to be able to move these
between modules. This will no longer work. It is much better not to support
it (like 1.5.11 won't) than to break it.
If I had any test modules, I could test it, rather than guessing, though.

> Thanks for the work and the report.
>
>        -Troy.
>
>
>
>
>
>> God Bless,
>> Ben
>>
>> -------------------------------------------------------------------------------------------
>> Multitudes, multitudes,
>>   in the valley of decision!
>> For the day of the LORD is near
>>   in the valley of decision.
>>
>> Giôên 3:14 (ESV)
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.crosswire.org/pipermail/sword-devel/attachments/20090408/f826c4fd/attachment-0001.html>