Given squares in the middle of a word cause assertion failures when navigating through them

[jpd236]

(Inspiration: https://spyscape.com/crosswords/puzzle-127-neptunes-trident)

Using this test file (unzip first):

test.zip

The center square is configured as a given/clue square, so users shouldn't be able to mutate it; however, it's part of a word. If you try to use the arrow keys to navigate into the square, you get an assertion failure asserting that the square is white or the puzzle is diagramless.

I think since the square should be considered non-focusable (you can't click on it), it should be treated like a black square with keyboard navigation and skipped over.

[mrichards42]

Hmm, I'd be inclined to treat this as a revealed white square (which can't be edited) rather than a black square, since we make a lot of assumptions about black squares. Even better would be to mark it as a checked-and-marked-correct square, but that wouldn't be portable -- apparently there are a few solving states I've stuck in as extra attributes that crossword solver ignores:

xword/puz/formats/jpz/save_jpz.cpp

Lines 194 to 195 in 3f18eeb

	if (square->HasFlag(FLAG_CORRECT))
	cell.append_attribute("correct") = "true";

But coming from a format like jpz with explicit words, you'd have to intentionally mark that square as part of a word, so I'd buy that it should work. Out of curiosity (a) what does crossword solver do with this puzzle, and (b) what does a release build of XWord do (since the assert should be silenced)? In a debug build if I tell it to ignore the assert failure it highlights the black square anyways, as if it were a diagramless puzzle. EDIT: wow, and also lets me type in a letter, that's just bonkers.

[jpd236]

Yeah, it's pretty broken :)

#147 is a targeted fix for this that seems reasonable to me - the call to SetFocusedSquare it prevents is guaranteed to trigger an assertion failure based on the condition I added, so it seems to me like it would always be better in that case to move forward with the rest of the method (in this case, finding the next white square in that direction).

We could maybe go beyond that and start treating these "annotation" squares as white squares. I introduced the concept of an annotation square (which is what is being used here - in JPZ format, a square with type="clue") for the Acrostic support in #112. Changing them to act like a revealed white square would maybe work, but I'm a little worried about side effects that might have on acrostics / other methods which change focus.

[jpd236]

Crossword solver doesn't let you focus on the square, but it also doesn't show the contents - it shows it as blank white.

[mrichards42]

Oh right, annotation squares, I forgot about those :)

Ok, so an annotation square with a white background perhaps? I'd still probably convert that to a white square that's already been revealed if I were interpreting that neptune's triednt puzzle, but if we get a puzzle with an annotation in the middle of the word (which is also a totally valid way to represent that), I like that fix.

[jpd236]

As a separate change, changing annotation squares to use white backgrounds by default seems reasonable, and would match what the Crossword Nexus solver does... I'm trying to remember if there was some reason that was a non-trivial change. Will take a look.

[jpd236]

BTW, re: the alternative of a pre-revealed white square (which I saw you did in 79fa394) - there are a few drawbacks there that I can think of:

• It counts under completion status as a filled square
• "Erase Grid" erases it
• Has the little indicator that it's a verified correct square

If I were saving as a .puz, that's what I'd do, but for a .jpz I personally lean towards the annotation square, especially if we can fix the BG color.

[jpd236]

In testing this further I found a couple more broken behaviors:

• It's also possible to trigger the same failed assertion by hitting backspace after the annotation square, or space before it. Also, if your movement options say to go to the next square after entering a letter rather than the next blank square, or if every other letter in the word is filled, you can also trigger it. So it looks like we have some more to do here.
• Shift+Arrow navigation skips over the word containing the annotation square, for some reason.

[mrichards42]

Hmm, so I'm fairly certain what we're running up against is that the default puz::Word "find" function is FIND_ANY_SQUARE:

xword/puz/Word.hpp

Lines 145 to 153 in d3b8c24

	Square * FindNextSquare(Square * start)
	{
	return FindNextSquare(start, FIND_ANY_SQUARE, NEXT);
	}
	Square * FindPrevSquare(Square * start)
	{
	return FindPrevSquare(start, FIND_ANY_SQUARE);
	}

e.g. in XGridCtrl::MoveAfterLetter

xword/src/XGridCtrl.cpp

Line 1581 in d3b8c24

newSquare = m_focusedWord->FindNextSquare(m_focusedSquare);

FIND_ANY_SQUARE is perfectly fine if you assume that words can only contain white squares :) But maybe that should default to FIND_WHITE_SQUARE instead, or perhaps safer is to audit for any usages of FindNextSquare and FindPrevSquare in XGridCtrl and make sure they use FIND_WHITE_SQUARE.

[jpd236]

Yep, that's right for the first issue. Sent #152 - I couldn't find any other usage beyond the three cases I mentioned running into above.

For the issue with Shift->Arrow, I'm a little less sure how to move forward, if at all. The core problem is that when navigating in this mode, rather than looking for the next word in that direction, we look for the next square that could be a word:

xword/src/XGridCtrl.cpp

Lines 1662 to 1668 in cd5d08c

	// Move to the next white square in the arrow direction that
	// *could* have a word.
	for (newSquare = m_focusedSquare;
	newSquare;
	newSquare = newSquare->Next(arrowDirection))
	{
	if (newSquare->HasWord(focusedDirection)

In the case of this Spyscape puzzle, you have a word with one white square, an annotation square, and another white square. Each of those two white squares isn't considered part of a potential word in this system, because a word has to have > 1 character. So they're skipped, even though a word is defined.

I see this logic has changed a fair bit over time for different scenarios, so I'm a bit more hesitant about changing it. But maybe it would be safe to navigate to the next square that is definitely part of a defined word, or could be a word per this algorithm?

[mrichards42]

But maybe it would be safe to navigate to the next square that is definitely part of a defined word, or could be a word per this algorithm?

I think that's fine -- left a comment on that PR, but we definitely should use defined words in all cases when we have them.

Really the logic should always use defined words (I'm pretty sure we always have words, since we generate them for formats that don't support them), except that can't work for diagramless puzzles. It would probably have been better to define separate "movement" interfaces, and have XGridCtrl use a different implementation depending on the puzzle type, but here we are.