Details

    • Type: Bug Bug
    • Status: Closed (View Workflow)
    • Priority: Critical Critical
    • Resolution: Duplicate
    • Affects Version/s: 1.6
    • Fix Version/s: None
    • Component/s: o.c.jsword.book.sword
    • Labels:
      None

      Description

      I received an And Bible bug report for this issue. I can reproduce it in Bible Desktop and And Bible but not in Xiphos. Therefore I suspect a problem with JSword, or possibly with both BD and AB.

      To view an instance of the problem open TurNTB (New Turkish Bible) in Bible Desktop and display Eph 2:4-5. Verses 4 and 5 should be merged but unfortunately verses 4 and 5 are duplicated. Also Heb 2:2-3 has the same problem and many other verses in TurNTB.

      I am not familiar with C++ Sword code but it might be good to check how Sword handles merged verses because Xiphos does not have this problem.

      A possible fix might be to check if a verse is identical to the previous, and if so ignore it. I can see how I might implement this in AB code but you may like to implement a fix in JSword, however I can't think how that would be done.

        Issue Links

          Activity

          Hide
          Martin Denham added a comment -

          Troy commented that C++ Sword uses
          virtual void SWModule::setSkipConsecutiveLinks(bool val);
          but can't find the Sword source.

          I have been stepping through JSword code and it appears, at least in TurNTB blockNum, blockStart, and blockSize are the same for merged verses e.g. Eph 2:4, Eph 2:5 so logic could be added to ZVerseBackend.getRawText to skip duplicate verses.

          The next level up would be to check osisContent.content in AbstractPassageBook.getOsisIterator

          The next level up would be BookData.getOsisContent

          If we could add a setSkipDuplicates(bool) that affects ZVerseBackend it can be done by comparing blockStart etc. Other than that it would seem to require xml or string comparisons to find duplicates.

          I would like to somehow apply a simple patch to old JSword code that could also be used with new code. Am I going in the right direction?

          Show
          Martin Denham added a comment - Troy commented that C++ Sword uses virtual void SWModule::setSkipConsecutiveLinks(bool val); but can't find the Sword source. I have been stepping through JSword code and it appears, at least in TurNTB blockNum, blockStart, and blockSize are the same for merged verses e.g. Eph 2:4, Eph 2:5 so logic could be added to ZVerseBackend.getRawText to skip duplicate verses. The next level up would be to check osisContent.content in AbstractPassageBook.getOsisIterator The next level up would be BookData.getOsisContent If we could add a setSkipDuplicates(bool) that affects ZVerseBackend it can be done by comparing blockStart etc. Other than that it would seem to require xml or string comparisons to find duplicates. I would like to somehow apply a simple patch to old JSword code that could also be used with new code. Am I going in the right direction?
          Hide
          Martin Denham added a comment -

          Okay, here is an experimental patch to ZVerseBackend. It relies on the fact that blockNum, verseStart, and verseSize are the same for merged verses. Any comments?

          Index: ZVerseBackend.java
          ===================================================================
          — ZVerseBackend.java (revision 2195)
          +++ ZVerseBackend.java (working copy)
          @@ -34,6 +34,7 @@
          import org.crosswire.common.util.Logger;
          import org.crosswire.common.util.NetUtil;
          import org.crosswire.jsword.JSMsg;
          +import org.crosswire.jsword.book.BookCategory;
          import org.crosswire.jsword.book.BookException;
          import org.crosswire.jsword.book.DataPolice;
          import org.crosswire.jsword.passage.Key;
          @@ -117,6 +118,8 @@
          public ZVerseBackend(SwordBookMetaData sbmd, BlockType blockType)

          { super(sbmd); this.blockType = blockType; + // Some commentaries like MHC show the same content for several adjacent verses, but merged verses should not be duplicated + this.isPreventDuplicateVerseContent = BookCategory.BIBLE.equals(sbmd.getBookCategory()); }

          /*
          @@ -322,6 +325,17 @@
          int verseStart = SwordUtil.decodeLittleEndian32(temp, 4);
          int verseSize = SwordUtil.decodeLittleEndian16(temp, 8);

          + //MJD start
          + // do not return duplicate text for merged verses
          + if (isPreventDuplicateVerseContent && index==lastIndex+1 && blockNum==lastBlockNum && verseStart==lastVerseStart && verseSize==lastVerseSize)

          { + lastIndex = index; + return ""; + }

          + lastIndex = index;
          + lastVerseStart = verseStart;
          + lastVerseSize = verseSize;
          + //MJD end
          +
          // Can we get the data from the cache
          byte[] uncompressed = null;
          if (blockNum == lastBlockNum && testament == lastTestament) {
          @@ -400,6 +414,11 @@
          */
          private int lastTestament = -1;

          + private boolean isPreventDuplicateVerseContent;
          +
          + private long lastIndex = -1;
          + private int lastVerseStart = -1;
          + private int lastVerseSize = -1;
          /**
          *
          */

          Martin

          Show
          Martin Denham added a comment - Okay, here is an experimental patch to ZVerseBackend. It relies on the fact that blockNum, verseStart, and verseSize are the same for merged verses. Any comments? Index: ZVerseBackend.java =================================================================== — ZVerseBackend.java (revision 2195) +++ ZVerseBackend.java (working copy) @@ -34,6 +34,7 @@ import org.crosswire.common.util.Logger; import org.crosswire.common.util.NetUtil; import org.crosswire.jsword.JSMsg; +import org.crosswire.jsword.book.BookCategory; import org.crosswire.jsword.book.BookException; import org.crosswire.jsword.book.DataPolice; import org.crosswire.jsword.passage.Key; @@ -117,6 +118,8 @@ public ZVerseBackend(SwordBookMetaData sbmd, BlockType blockType) { super(sbmd); this.blockType = blockType; + // Some commentaries like MHC show the same content for several adjacent verses, but merged verses should not be duplicated + this.isPreventDuplicateVerseContent = BookCategory.BIBLE.equals(sbmd.getBookCategory()); } /* @@ -322,6 +325,17 @@ int verseStart = SwordUtil.decodeLittleEndian32(temp, 4); int verseSize = SwordUtil.decodeLittleEndian16(temp, 8); + //MJD start + // do not return duplicate text for merged verses + if (isPreventDuplicateVerseContent && index==lastIndex+1 && blockNum==lastBlockNum && verseStart==lastVerseStart && verseSize==lastVerseSize) { + lastIndex = index; + return ""; + } + lastIndex = index; + lastVerseStart = verseStart; + lastVerseSize = verseSize; + //MJD end + // Can we get the data from the cache byte[] uncompressed = null; if (blockNum == lastBlockNum && testament == lastTestament) { @@ -400,6 +414,11 @@ */ private int lastTestament = -1; + private boolean isPreventDuplicateVerseContent; + + private long lastIndex = -1; + private int lastVerseStart = -1; + private int lastVerseSize = -1; /** * */ Martin
          Hide
          DM Smith added a comment -

          I'll give it a go. I think it is definitely on the right track.

          Show
          DM Smith added a comment - I'll give it a go. I think it is definitely on the right track.
          Hide
          DM Smith added a comment -

          This issue has better content than JS-66. I'll keep it both open until solved.

          Show
          DM Smith added a comment - This issue has better content than JS-66 . I'll keep it both open until solved.
          Hide
          DM Smith added a comment -

          I've put in a note in JS-66 to refer to the solution provided here.

          Show
          DM Smith added a comment - I've put in a note in JS-66 to refer to the solution provided here.
          Hide
          Martin Denham added a comment -

          Nic also had an issue with this in PS: http://www.crosswire.org/tracker/browse/PS-57.
          It might be worth asking how he solved it, although I didn't think Sword had a problem with this.

          Show
          Martin Denham added a comment - Nic also had an issue with this in PS: http://www.crosswire.org/tracker/browse/PS-57 . It might be worth asking how he solved it, although I didn't think Sword had a problem with this.
          Hide
          Martin Denham added a comment - - edited

          I have been looking at this again and come up with another possible solution if you didn't like the previous - this is slightly less 'hacky'. It was tricky to find a place that would affect all bibles. I thought of creating a DeDupeVersesFilter but the Filters (like OSISFilter, GBFFiltr,..) don't seem to allow embedded re-use i.e. Filters on Filters. If filters could be embedded then that might possibly be more elegant. The following suggestion is at a slightly higher level than before and I have to add similar code to And Bible for the optimised Bible loading process but it will fall back to this for badly formatted Bibles and I think this code is used by most other front ends. It just checks to see if the verse text is the same as the previous verse:

          AbstractPassageBook:

                  RawTextToXmlProcessor processor = new RawTextToXmlProcessor() {
                  // track previous text to exclude duplicates caused by merged verses
                  private String previousVerseText = "";
                      
                  public void preRange(VerseRange range, List<Content> partialDom) {
                      if (showTitles) {
                          Element title = OSISUtil.factory().createGeneratedTitle();
                          title.addContent(range.getName());
                          partialDom.add(title);
                      }
                  }
          
                  public void postVerse(Key verse, List<Content> partialDom, String rawText) {
                      // If the verse is empty then we shouldn't add the verse tag *** FOLLOWING LINE CONTAINS THE CHECK FOR DUPLICATE VERSE ***
                      if ((allowEmpty || rawText.length() > 0 ) && !previousVerseText.equals(rawText)) {
                          List<Content> osisContent = filter.toOSIS(AbstractPassageBook.this, verse, rawText);
                          addOSIS(verse, partialDom, osisContent);
                      }
                      previousVerseText = rawText;
                  }
          
          Show
          Martin Denham added a comment - - edited I have been looking at this again and come up with another possible solution if you didn't like the previous - this is slightly less 'hacky'. It was tricky to find a place that would affect all bibles. I thought of creating a DeDupeVersesFilter but the Filters (like OSISFilter, GBFFiltr,..) don't seem to allow embedded re-use i.e. Filters on Filters. If filters could be embedded then that might possibly be more elegant. The following suggestion is at a slightly higher level than before and I have to add similar code to And Bible for the optimised Bible loading process but it will fall back to this for badly formatted Bibles and I think this code is used by most other front ends. It just checks to see if the verse text is the same as the previous verse: AbstractPassageBook: RawTextToXmlProcessor processor = new RawTextToXmlProcessor() { // track previous text to exclude duplicates caused by merged verses private String previousVerseText = ""; public void preRange(VerseRange range, List<Content> partialDom) { if (showTitles) { Element title = OSISUtil.factory().createGeneratedTitle(); title.addContent(range.getName()); partialDom.add(title); } } public void postVerse(Key verse, List<Content> partialDom, String rawText) { // If the verse is empty then we shouldn't add the verse tag *** FOLLOWING LINE CONTAINS THE CHECK FOR DUPLICATE VERSE *** if ((allowEmpty || rawText.length() > 0 ) && !previousVerseText.equals(rawText)) { List<Content> osisContent = filter.toOSIS(AbstractPassageBook.this, verse, rawText); addOSIS(verse, partialDom, osisContent); } previousVerseText = rawText; }
          Hide
          Martin Denham added a comment - - edited

          sorry, just found the noformat tag

          Show
          Martin Denham added a comment - - edited sorry, just found the noformat tag
          Hide
          Martin Denham added a comment -

          Patch for the change suggested above.

          Show
          Martin Denham added a comment - Patch for the change suggested above.

            People

            • Assignee:
              DM Smith
              Reporter:
              Martin Denham
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: