Add "When space is pressed, switch direction" control

[undergroundmonorail]

Hi, I'm not really familiar with this codebase at all but I did my best :P

This commit adds a new option in File > Preferences to toggle the behaviour of the space key. By default, the existing behaviour is used. The new option allows the user to press space to toggle direction (i.e. focus the word that intersects the current word at this point).

It's a fairly simple change and I tested it as well as I could. However, I'm not familiar with any of the variant crossword types the app seems to support so I don't know how well it will behave in, say, a crossword with diagonal words. I don't anticipate any real issues, though; it's just checking if the current focused direction is puz::ACROSS and setting it to puz::DOWN, and otherwise setting it to puz::ACROSS.

This PR addresses issue #161.

[mrichards42]

Hi @undergroundmonorail thanks! I'll take a look a this in the next couple days. FYI that failing build is actually coming from this part:

/Users/appveyor/projects/xword/src/dialogs/PreferencesPanel.cpp:219:9: error: use of undeclared identifier 'm_switchDirectionsOnSpace'
        m_switchDirectionsOnSpace->SetValue(1);
        ^
/Users/appveyor/projects/xword/src/dialogs/PreferencesPanel.cpp:221:9: error: use of undeclared identifier 'm_insertBlankOnSpace'
        m_insertBlankOnSpace->SetValue(1);
        ^
/Users/appveyor/projects/xword/src/dialogs/PreferencesPanel.cpp:260:9: error: use of undeclared identifier 'm_switchDirectionsOnSpace'
    if (m_switchDirectionsOnSpace->GetValue())
        ^
/Users/appveyor/projects/xword/src/dialogs/PreferencesPanel.cpp:293:22: error: use of undeclared identifier 'm_insertBlankOnSpace'
    BindChangedEvent(m_insertBlankOnSpace);
                     ^
/Users/appveyor/projects/xword/src/dialogs/PreferencesPanel.cpp:294:22: error: use of undeclared identifier 'm_switchDirectionsOnSpace'
    BindChangedEvent(m_switchDirectionsOnSpace);
                     ^
5 errors generated.

Reason being that macos uses a different preferences panel since the ui guidelines are quite different. You'll also want to add those new controls in https://github.com/mrichards42/xword/blob/master/src/dialogs/wxFB_PreferencesPanelsOSX.fbp

[undergroundmonorail]

Oh, of course, thank you! I thought it couldn't find the identifiers because it was looking in files that it couldn't find, being case sensitive. It didn't even occur to me that it was looking in a completely different place.

I'll give that a shot :) I don't have a Mac to test on unfortunately but I assume if I add the correct controls with the same names and everything into the ...OSX.fbp, that will solve the problem?

[jpd236]

There's some code at https://github.com/mrichards42/xword/blob/master/src/XGridCtrl.cpp#L647-L663 to change directions which is a bit more flexible / checks for existence of a word in that direction. I'm not sure if the logic is perfect, but it might be good to use the same logic here to cover some additional cases for variety puzzles and hopefully reduce the number of changes we'd need to fix #164.

[undergroundmonorail]

Don't mind me, realized I messed up some formatting stuff :)

[undergroundmonorail]

Modified the code to reuse the logic from SetFocusedSquare as @jpd236 suggested. It's slightly redundant now, since it gets called twice when space is pressed (once to find the new direction and once during the execution of SetFocusedSquare), but at least it's cleaner?

I just put the function right above where it's first needed and let my IDE generate the corresponding line in the .hpp file. Let me know if there's anything that should be changed; I basically don't know C++ at all :P

[mrichards42]

Sorry this took so long to get to! The main thing I'm concerned about is that extracting OppositeFocusDirection might be more complicated than it first appears. I like the idea of centralizing the focus direction code, although I expect that existing code that deals with setting the grid direction and square ends up going through SetFocusedSquare anyways, so that might already be central enough.

I think I'd go with one of two things here:

1. The simple route: basically just your first two commits (although you might be able to simplify the first one to just this). Since SetFocusDirection already "clamps" the direction to something that should have a word, I think you can get by with giving it a square plus whatever direction you want (and no word) and it should do something sensible.
2. The refactor route: remove the word argument from that function, and then like @jpd236 suggested, we'd want to use this new function anywhere this "swap direction" logic is used (e.g. like the link above, which is triggered on double-click).

[mrichards42]

This could probably use a refactor now that we're treating SPACE differently in both contexts (which letter to set, and where to move).

Perhaps something like this (pseudocode):

if letter == SPACE:
    if SWAP_ON_SPACE:
        # just swap, no letter
    else:
        # set blank and move to next square, if any
else:
    # enter letter and do the normal move-to-next behavior

[mrichards42]

While I like the idea of this refactor, I think this part of the code does too much to really be able to pull it out of its original context.

OppositeDirection(square, direction) makes a lot of sense to me, but the word argument shouldn't come along for the ride. It looks like in the original context this block of code was concerned with (a) finding a word if the caller didn't supply one, and (b) figuring out what the direction should be.

If we want to keep this refactor, I think I'd recommend removing the word argument, and then where it's used in SetFocusedSquare, you'd want to figure out the new word based on the new direction, so

direction = OppositeDirection(square, direction);
word = m_puz->FindWord(direction);