<div class="gmail_quote">On Tue, Apr 7, 2009 at 5:27 PM, Troy A. Griffitts <span dir="ltr"><<a href="mailto:scribe@crosswire.org">scribe@crosswire.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
Ben,<br>
<br>
Do you have more specific information about the items you mention? More comments below:<br>
<br>
<br>
Ben Morgan wrote:<div class="im"><br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
On Mon, Apr 6, 2009 at 10:00 PM, Troy A. Griffitts <<a href="mailto:scribe@crosswire.org" target="_blank">scribe@crosswire.org</a> <mailto:<a href="mailto:scribe@crosswire.org" target="_blank">scribe@crosswire.org</a>>> wrote:<br>
<br>
Ben Morgan wrote:<br>
<br>
I didn't like it either. But there were two major reasons for this:<br>
a) Creating a new LocaleMgr when stringmgr was was very costly<br>
as BPBible keeps around a sword.conf file and then all the paths<br>
in this were automatically loaded.<br>
<br>
<br>
This should only happen on application startup and shouldn't be too<br>
costly. I believe loading locales should actually be quite fast,<br>
and if not, then we have other troubles we should fix.<br>
<br>
Well, it isn't always. I am often dealing with network drives, etc.<br>
And the sword.conf files I am using can have ~15-20 entries...<br>
</blockquote>
<br></div>
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.<div class="im">
</div></blockquote><div>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...<br>
<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div class="im">
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
b) more importantly, I discovered an issue where having 1.5.11<br>
locales and 1.5.12 locales around a the same time would break<br>
verse lookup.<br>
<br>
<br>
Yes, having 1.5.11 formatted locales will not work with the latest<br>
code, but I don't think I'm willing to support a configuration path<br>
which has both formats. I suppose we could add a quick check for a<br>
numeric value for the first book abbrev and not load the locale in<br>
1.6.x if so. Do you think it would be beneficial. I hesitate<br>
because 1.5.x will almost certainly break if it finds the new locale<br>
format files. It's almost better just to say both formats can't<br>
co-exist.<br>
<br>
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.<br>
</blockquote>
<br></div>
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.<div class="im">
</div></blockquote><div>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.<br><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div class="im"><br>
<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
BPBible provides its own locales, and it is important that these<br>
are loaded, not any others. This is why I need a custom LocaleMgr.<br>
<br>
<br>
<br>
:) I'm not gonna ask why BPBible needs its own locales.<br>
<br>
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.<br>
</blockquote>
<br></div>
BibleCS also has its own uilocales.d/ folder. We augment the base sword locales with anything we find in uilocales.d with this call:<br>
<br>
LocaleMgr::getSystemLocaleMgr()->loadConfigDir("./uilocales.d");<br>
<br>
We don't actually replace the system LocaleMgr.<div class="im"></div></blockquote><div>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) <br>
</div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div class="im"><br>
<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
const is important in public interface methods to advertise to the<br>
consumer what our contract is. For example, if we have a method like:<br>
<br>
doSomething(const SWModule *) {<br>
}<br>
<br>
we are guaranteeing to the caller that we will not modify the<br>
SWModule. Unless hasEntry is defined as const, then we will get a<br>
compile error inside doSomething if we try to call mod->hasEntry()<br>
<br>
We can cache. the mutable keyword is used in C++ to tell the<br>
compiler that we are allowed to change this class member even in a<br>
const method.<br>
<br>
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.<br>
</blockquote>
<br></div>
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?</blockquote>
<div>OK, the implementation you gave for SWModule was broken - it looked at the current entry. A replacement solution to fix this looks like this:<br><span style="font-family: courier new,monospace;">bool SWModule::hasEntry(const SWKey *k) const </span><br style="font-family: courier new,monospace;">
<span style="font-family: courier new,monospace;">{</span><br style="font-family: courier new,monospace;"><span style="font-family: courier new,monospace;"> SWKey *saveKey;</span><br style="font-family: courier new,monospace;">
<span style="font-family: courier new,monospace;"> bool retVal;</span><br style="font-family: courier new,monospace;"><br style="font-family: courier new,monospace;"><span style="font-family: courier new,monospace;"> if (!key->Persist()) {</span><br style="font-family: courier new,monospace;">
<span style="font-family: courier new,monospace;"> saveKey = CreateKey();</span><br style="font-family: courier new,monospace;"><span style="font-family: courier new,monospace;"> *saveKey = *key;</span><br style="font-family: courier new,monospace;">
<span style="font-family: courier new,monospace;"> }</span><br style="font-family: courier new,monospace;"><span style="font-family: courier new,monospace;"> else saveKey = key;</span><br style="font-family: courier new,monospace;">
<br style="font-family: courier new,monospace;"><span style="font-family: courier new,monospace;"> setKey(*tmpKey);</span><br style="font-family: courier new,monospace;"><br style="font-family: courier new,monospace;">
<span style="font-family: courier new,monospace;"> getRawEntry();</span><br style="font-family: courier new,monospace;"><span style="font-family: courier new,monospace;"> retVal = getEntrySize();</span><br style="font-family: courier new,monospace;">
<br style="font-family: courier new,monospace;"><span style="font-family: courier new,monospace;"> setKey(*saveKey);</span><br style="font-family: courier new,monospace;"><br style="font-family: courier new,monospace;">
<span style="font-family: courier new,monospace;"> if (!saveKey->Persist())</span><br style="font-family: courier new,monospace;"><span style="font-family: courier new,monospace;"> delete saveKey;</span><br style="font-family: courier new,monospace;">
<br style="font-family: courier new,monospace;"><span style="font-family: courier new,monospace;"> return retVal;</span><br style="font-family: courier new,monospace;"><span style="font-family: courier new,monospace;">}<br>
</span><br>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.<br>
<br><span style="font-family: courier new,monospace;"></span></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div class="im"><br>
<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
Have Chris' latest fixes also fixed your issues with these deltas?<br>
<br>
Sorry, I haven't tried them. I would expect they would, though.<br>
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).<br>
</blockquote>
<br></div>
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.<br>
</blockquote><div>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.<br>
If I had any test modules, I could test it, rather than guessing, though.<br><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>
Thanks for the work and the report.<br><font color="#888888">
<br>
-Troy.</font><div><div></div><div class="h5"><br>
<br>
<br>
<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
God Bless,<br>
Ben<br>
-------------------------------------------------------------------------------------------<br>
Multitudes, multitudes,<br>
in the valley of decision!<br>
For the day of the LORD is near<br>
in the valley of decision.<br>
<br>
Giôên 3:14 (ESV)<br>
<br>
</blockquote>
<br>
</div></div></blockquote></div><br>