Details

    • Type: Bug Bug
    • Status: Open (View Workflow)
    • Priority: Blocker Blocker
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: o.c.jsword.book
    • Labels:
      None
    • Environment:

      Windows, Android

      Description

      EsvBook.contains(Isaiah53) returns false in both And Bible and running junits on my Windows pc using a separate install. It is only the ESV that this problem occurs with.

      I don't get this problem in Xiphos so I think it is probably a JSword issue.

      I am using the latest version of JSword from github.

      Here is a junit I used:

      public void testESVIs53() throws Exception {
      	SwordBook book = (SwordBook)Books.installed().getBook("ESV");
      	assertNotNull("ESV not found", book);
      	
      	Version version = (Version)book.getBookMetaData().getProperty("Version");
      	assertEquals("You have a different version of ESV installed", "1.0.1", version.toString());
      
      	Verse isaiah52 = VerseFactory.fromString(book.getVersification(), "Is 52");
      	Verse isaiah53 = VerseFactory.fromString(book.getVersification(), "Is 53");
      	Verse isaiah54 = VerseFactory.fromString(book.getVersification(), "Is 54");
      	assertTrue("Is 52 not found", book.contains(isaiah52));
      	assertTrue("Is 54 not found", book.contains(isaiah54));
      	// fails on the next line
      	assertTrue("Is 53 not found", book.contains(isaiah53));
      }
      

        Activity

        Hide
        Chris Burrell added a comment -

        Yes, but i think the contains fails because the ESV module doesn't contain the whole key. (i.e. doesn't contain verse 0). I thought I put in a fix for that a while ago, but perhaps it got reverted during the versification work.

        Show
        Chris Burrell added a comment - Yes, but i think the contains fails because the ESV module doesn't contain the whole key. (i.e. doesn't contain verse 0). I thought I put in a fix for that a while ago, but perhaps it got reverted during the versification work.
        Hide
        DM Smith added a comment -

        I think you all have found the underlying problems. We should address these.

        The bigger question is what is wanted in asking whether a book contains a chapter? I'm thinking that this is fundamentally different than asking whether all the parts of a key is present in a book. Or is it that when a chapter is retrieved, it calls contains and comes back with an inappropriate answer?

        Show
        DM Smith added a comment - I think you all have found the underlying problems. We should address these. The bigger question is what is wanted in asking whether a book contains a chapter? I'm thinking that this is fundamentally different than asking whether all the parts of a key is present in a book. Or is it that when a chapter is retrieved, it calls contains and comes back with an inappropriate answer?
        Hide
        Martin Denham added a comment -

        So the current implementation of contains(key) is containsAllOf(key) but in this particular instance we are expecting containsAnyOf(key). Is that right?

        If so then we could either consider i) changing contains to be containsAnyOf or ii) adding a different function containsAnyOf or iii) changing contains to be containsAnyOf when the key is a chapter.

        From an And Bible perspective book.contains(key) is used to work out if there is anything that can be shown on the screen so contains(key) is always used in the sense of containsAnyOf so my initial preference would be to change the implementation of book.contains() to be containsAnyOf but there may be use cases others would raise that I haven't thought of.

        ----------
        I just remembered I emailed JSword-devel regarding a different issue related to VerseRange.contains() on 4th May so maybe contains() needs consideration in general. I need to raise a Jira for that because I don't think one was raised and I don't believe it has been fixed (I worked around it in AB). I'll raise a jira but hopefully nobody gets confused between the two different contains() problems.
        ----------

        Martin

        Show
        Martin Denham added a comment - So the current implementation of contains(key) is containsAllOf(key) but in this particular instance we are expecting containsAnyOf(key). Is that right? If so then we could either consider i) changing contains to be containsAnyOf or ii) adding a different function containsAnyOf or iii) changing contains to be containsAnyOf when the key is a chapter. From an And Bible perspective book.contains(key) is used to work out if there is anything that can be shown on the screen so contains(key) is always used in the sense of containsAnyOf so my initial preference would be to change the implementation of book.contains() to be containsAnyOf but there may be use cases others would raise that I haven't thought of. ---------- I just remembered I emailed JSword-devel regarding a different issue related to VerseRange.contains() on 4th May so maybe contains() needs consideration in general. I need to raise a Jira for that because I don't think one was raised and I don't believe it has been fixed (I worked around it in AB). I'll raise a jira but hopefully nobody gets confused between the two different contains() problems. ---------- Martin
        Hide
        DM Smith added a comment -

        I think I've figured this out.

        Before we added Verse 0, VerseFactory.fromString("Is 53") internally would have computed a verse range from Is 53:1 to the end of the chapter and then return the start of the verse range. Now it returns Is 53:0. In this case, verse 0 (a heading) is not in the chapter.

        If it were computed as a VerseRange, it would have stilled failed, as verse 0 was not there. I agree with the need to have containsAny and to clarify with containsAll.

        Show
        DM Smith added a comment - I think I've figured this out. Before we added Verse 0, VerseFactory.fromString("Is 53") internally would have computed a verse range from Is 53:1 to the end of the chapter and then return the start of the verse range. Now it returns Is 53:0. In this case, verse 0 (a heading) is not in the chapter. If it were computed as a VerseRange, it would have stilled failed, as verse 0 was not there. I agree with the need to have containsAny and to clarify with containsAll.
        Hide
        DM Smith added a comment -

        book.contains(key) determines whether something is in the module. It assumes that key is a Verse. If it is not it grabs the first verse in the key.

        When you give an incomplete specification to VerseFactory.fromString, it assumes values of 0 for missing chapter and verse. So if you specify:
        Is that will be Is.0.0 (the book introduction).
        Is 53 will be Is.53.0 (the chapter introduction).

        It will be these exact values looked for in the module.

        VerseFactory only returns a single Verse.

        If you had used VerseRangeFactory.fromString, then it would have assumed that
        Is is the entire book starting from Is.0.0
        Is.53 is the entire chapter starting from Is.53.0

        If you passed the VerseRange to the contains method, it would have used the first verse in the range and ignored the rest.

        Likewise for using a Passage, it only looks at the first verse.

        It does not mean containsAll, it means containsFirst.

        In the VerseKey classes contains(Key) means containsAll.

        Combine this with the change to allow chapter and verse 0. It explains why it would have worked before. The factory methods would have used 1 for missing chapter and verse. It is a reasonable heuristic to check for the first verse of a chapter.

        Show
        DM Smith added a comment - book.contains(key) determines whether something is in the module. It assumes that key is a Verse. If it is not it grabs the first verse in the key. When you give an incomplete specification to VerseFactory.fromString, it assumes values of 0 for missing chapter and verse. So if you specify: Is that will be Is.0.0 (the book introduction). Is 53 will be Is.53.0 (the chapter introduction). It will be these exact values looked for in the module. VerseFactory only returns a single Verse. If you had used VerseRangeFactory.fromString, then it would have assumed that Is is the entire book starting from Is.0.0 Is.53 is the entire chapter starting from Is.53.0 If you passed the VerseRange to the contains method, it would have used the first verse in the range and ignored the rest. Likewise for using a Passage, it only looks at the first verse. It does not mean containsAll, it means containsFirst. In the VerseKey classes contains(Key) means containsAll. Combine this with the change to allow chapter and verse 0. It explains why it would have worked before. The factory methods would have used 1 for missing chapter and verse. It is a reasonable heuristic to check for the first verse of a chapter.

          People

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

            Dates

            • Created:
              Updated: