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
        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.
        Hide
        DM Smith added a comment -

        Is this still an issue?

        Show
        DM Smith added a comment - Is this still an issue?
        Hide
        Martin Denham added a comment -

        Yes, the above junit test fails on my current build of JSword which is not right up to date and also fails on JSword tip. I had to work around it in And Bible.

        Running the above junit the text "Is 53 not found" is displayed.

        Good to see you are back again!

        Martin

        Show
        Martin Denham added a comment - Yes, the above junit test fails on my current build of JSword which is not right up to date and also fails on JSword tip. I had to work around it in And Bible. Running the above junit the text "Is 53 not found" is displayed. Good to see you are back again! Martin
        Hide
        Martin Denham added a comment -

        I just found the work around in And Bible:

            /**
             * When checking a book contains a chapter SwordBook returns false if verse 0 is not in the chapter so this method compensates for that
             * 
             * This can be removed if SwordBook.contains is converted to be containsAnyOf as discussed in JS-273
             */
            private boolean bookContainsAnyOf(Book book, Key key) {
                if (book.contains(key)) {
                    return true;
                }
                
                Iterator<Key> iter = key.iterator();
                while (iter.hasNext()) {
                    if (book.contains(iter.next())) {
                        return true;
                    }
                }
                
                return false;
            }
        
        Show
        Martin Denham added a comment - I just found the work around in And Bible: /** * When checking a book contains a chapter SwordBook returns false if verse 0 is not in the chapter so this method compensates for that * * This can be removed if SwordBook.contains is converted to be containsAnyOf as discussed in JS-273 */ private boolean bookContainsAnyOf(Book book, Key key) { if (book.contains(key)) { return true ; } Iterator<Key> iter = key.iterator(); while (iter.hasNext()) { if (book.contains(iter.next())) { return true ; } } return false ; }

          People

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

            Dates

            • Created:
              Updated: