We were using free_cfg(be_prefs) after we copied be_prefs to
all_prefs, but we actually want to use sfree(be_prefs) since we don't
want to free each element that has been copied. None of the games so
far use string preferences, which is why they haven't been affected by
this bug.
The change I made in c224416c76e41f284b318adc51f08c3ed11de8e2 was
incorrect: I accidentally removed a "return" statement and left in a
debugging printf() when I meant to keep the return and drop the
printf().
midend_process_key() has some generic code for converting MOD_CTRL along
with a printing character into a control character. This is derived
from the Emscripten front-end because browsers don't do this themselves.
Most other front ends seem to depend on the platform for this mapping.
The mapping was applied to all printable ASCII characters, but this
meant that Ctrl+-, which is commonly used by browsers to mean "zoom out"
got converted into CR and then CURSOR_SELECT. That was confusing to say
the least.
So now, the CTRL mapping is only applied to characters in the roughly
alphabetic range (0x40 to 0x7f), and MOD_CTRL applied to a character in
the range 0x20 to 0x3f gets a return of PKR_UNUSED instead. That means
that Ctrl+- in browsers now works properly.
I don't think this will affect other front-ends because they're
generally a lot less generous about passing MOD_CTRL to the mid-end.
I've tested the GTK port nonetheless and not found any problems.
This removed the "handled" pointer and instead extends the existing
boolean return value (quit or don't quit) into an enumeration. One of
the values still quits the program, but now there are different values
for keys that had an effect, had no effect, and are not used by the
puzzle at all. The mapping from interpret_move results to process_key
results is roughly:
move string -> PKR_SOME_EFFECT
MOVE_UI_UPDATE -> PKR_SOME_EFFECT
MOVE_NO_EFFECT -> PKR_NO_EFFECT
MOVE_UNUSED -> PKR_UNUSED
The mid-end can also generate results internally, and is the only place
that PKR_QUIT can arise.
For compatibility, PKR_QUIT is zero, so anything expecting a false
return value to mean quit will be unsurprised. The other values are
ordered so that lower values indicate a greater amount of handling of
the key.
These allow for distinguishing the case where a puzzle doesn't have a
use for a key from the case where it just happens to have no effect in
the current state of the puzzle. These were both represented by NULL,
but that now represents the case where a puzzle hasn't been updated to
make the distinction yet.
All the other constants named UI_* are special key names that can be
passed to midend_process_key(), but UI_UPDATE is a special return value
from the back-end interpret_move() function instead. This renaming
makes the distinction clear and provides a naming convention for future
special return values from interpret_move().
We're about to append one character to the buffer _and_ put a \0 after
it, so we need the buffer to be at least _two_ characters longer than
where the current position is.
I think this bug would have had a hard time showing up in normal use,
but I managed to trigger it by completely messing up a prototype
Emscripten preferences implementation, and a good thing too.
This must have been introduced during a last-minute rebase, or similar
- I'm sure it worked a couple of days ago! Because midend_set_config
passed a NULL game_ui to midend_set_prefs, the latter would make up a
temporary UI and apply the changes to that. As a result, the midend's
main UI would keep the original backend preferences, and those would
also be the ones saved to the config file.
With this option turned off (it's on by default), the single-letter
keyboard shortcuts like 'q' for quit and 'n' for new game don't
function any more. You can still access the same functions via more
complicated shortcuts like Ctrl-Q or Ctrl-N, and front ends can
provide any other UI they like for the same operations, but this way,
people aren't at risk of blowing away half an hour of puzzling with
one misaimed key.
This is a thing people have occasionally asked for, and I've generally
resisted on the grounds that I have sympathy for people playing
puzzles at work who need to be able to close the game quickly when an
unsympathetic boss wanders by. But now we have a preferences system,
we can cater to those people _and_ the ones who don't mind.
More immediately useful: adding _at least one_ universal preference in
the initial version of this system means that there will be no games
with no preference options at all, and therefore, no need to put
conditionals all through the participating frontends to deal with
whether the Preferences menu option should even be provided. That
would have been a waste of time because all those conditionals would
just have to be removed again as soon as the first universal
preference came along - so adding an easy one _now_ means we can save
the effort of putting in the temporary conditionals in the first
place.
This commit introduces a serialisation format for the user preferences
stored in game_ui, using the keyword identifiers that get_prefs is
required to write into its list of config_item. As a result, the
serialisation format looks enough like an ordinary config file that a
user could write one by hand.
The preferences for the game backend are kept in serialised form in
me->be_prefs. The typical use of this is to apply it to a just-created
game_ui by calling midend_apply_prefs(), which deserialises the prefs
buffer into a list of config_item and passes it to the backend's
set_prefs function, overwriting the preference fields (but no others)
of the game_ui.
This is duly done when creating a new game, when loading a game from a
save file, and also when printing a puzzle. To make the latter work,
document_add_puzzle now takes a game_ui (and keeps ownership of it
afterwards), and passes that to the backend's compute_size and print
functions.
The backend's own get_prefs and set_prefs functions are wrapped by
midend_get_prefs and midend_set_prefs. This is partly as a convenience
(it deals with optionally constructing a game_ui specially to call the
backend with), but mostly so that there will be a convenient place in
the midend to add standard preferences applying across all puzzles.
No cross-puzzle preferences are provided yet.
There are two external interfaces to all this, and in this commit,
neither one is yet called by any frontend:
A new pair of midend functions is exposed to the front end, called
midend_load_prefs and midend_save_prefs. These have a similar API to
midend_serialise and midend_deserialise, taking a read/write function
pointer and a context. So front ends that can already load/save a game
to a file on disk should find it easy to add a similar set of
functions loading/saving user preferences.
Secondly, a new value CFG_PREFS is added to the enumeration of
configuration dialog types, alongside the ones for the Custom game
type, entering a game description and entering a random seed. This
should make it easy for frontends to offer a Preferences dialog,
because it will operate almost exactly like three dialogs they already
handle.
These are currently only used for internal serialisation of midend
state in order to undo/redo across New Game, and are named
accordingly. But I'm about to reuse them for another set of
serialisation functions, so they'll want more general names, and also
need to be moved higher up in the source file so as to have been
defined where I'll want to use them.
I'm about to move some of the bodgy getenv-based options so that they
become fields in game_ui. So these functions, which could previously
access those options directly via getenv, will now need to be given a
game_ui where they can look them up.
Some games would like a way to check that the parameters in the encoded
UI string are consistent with the game parameters. Since this might
depend on the current state of the game (this being what changed_state()
is for), implement this by adding a game_state parameter to decode_ui().
Nothing currently uses it, though Guess usefully could.
The majority of back-ends define encode_ui() to return NULL and
decode_ui() to do nothing. This commit allows them to instead specify
the relevant function pointers as NULL, in which case the mid-end won't
try to call them.
I'm planning to add a parameter to decode_ui(), and if I'm going to have
to touch every back-end's version of decode_ui(), I may as well ensure
that most of them never need to be touched again. And obviously
encode_ui() should go the same way for symmetry.
In all front ends other than JavaScript, and in JavaScript prior to my
recent changes, MOD_NUM_KEYPAD can only be set on ASCII digits. For
now, enforce this in midend_process_key() so as to avoid inconsistency
between front ends. It might be useful to be able to distinguish
keypad versions of more keys, but that should be co-ordinated across
the code-base.
The two Return/Enter keys have always been treated the same in the
past, but a user complained today that Enter was no longer functioning
as CURSOR_SELECT in the web puzzles.
This happened in commit 9dbcfa765ba59a8, apparently because the web
front end is now translating the Enter key as MOD_NUM_KEYPAD | '\r'
instead of just '\r', and the new code in midend.c is only stripping
off MOD_NUM_KEYPAD for values >= 0x80.
Now it strips MOD_NUM_KEYPAD off C0 control characters as well, so
that only _printable_ ASCII characters can still have that modifier
when they get to the backend - i.e. you can tell numpad digits from
normal digits, and ditto +-* etc. But keypad Enter is now turned into
plain '\r' by the modifier removal code, and then into CURSOR_SELECT.
Other front ends still aren't even bothering to set MOD_NUM_KEYPAD on
the code sent by Enter. But that's fine, because now midend.c
officially doesn't care whether they do or not.
A save file that ended in the middle of a value before the "SAVEFILE"
field had been loaded would cause a read from uninitialised memory.
While technically undefined behaviour this was practically pretty
harmless. Fixed by handling unexpected EOF here the same an
unexpected EOF anywhere else.
This bug could be demonstrated by loading a truncated save file like
this in a build with MemorySanitizer enabled:
SAVEFILE:41:Simo
It now strips off modifier flags from keys that shouldn't have them and
maps printable characters with MOD_CTRL to the corresponding control
characters. It also catches Ctrl+Shift+Z because that obviously belongs
in the midend.
I've updated the JavaScript front-end to take advantage of these
changes. Other front ends are unchanged and should work just as they
did before.
Apart from "SEED" records, all values in save files generated by Puzzles
should be printable ASCII. This is enforced by assertion in the saving
code. However, if a save file with non-ASCII move strings (for
instance) manages to get loaded then these non-ASCII values can cause an
assertion failure on saving. Instead, the loading code now checks
values for ASCIIness.
This will not only avoid problems when re-saving files, but will also
defend the various internal parsers from at least some evil strings. It
shouldn't invalidate any save files actually generated by Puzzles, but
it will sadly invalidate some of my fuzzing corpus.
If we start seeing state records in a save file (MOVE, SOLVE, or
RESTART), we should already have seen STATEPOS, so emit an error if not.
This avoids the situation where we overrun the end of the state array
because we're continuing loading states in the hope a STATEPOS will come
along. I've also added an assertion that we're not overrunning the
state array for added paranoia.
An earlier version of this fix just removed the test for data.statepos
at the head of the loop, but that's wrong for a file that only has the
initial state.
This bug can be demonstrated by building Bridges with AddressSanitizer
and loading this save file:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
VERSION :1:1
GAME :7:Bridges
PARAMS :13:7x7i30e10m2d0
CPARAMS :13:7x7i30e10m2d0
DESC :24:a4b4a1g1a2a8a4a4m2b2b3e3
NSTATES :1:2
MOVE :10:L1,0,4,0,1
MOVE :10:L1,0,4,0,2
"1999999999999999999999999999999999999999999999999999" as a record
length should lead to an error, not a buffer overrun.
(fun fact that was less obvious to me than it should have been: very
large powers of ten are multiples of large powers of two, so that number
is -1 mod 2^32)
This bug can be demonstrated by building any puzzle with
AddressSanitizer and then loading this save file:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
VERSION :1999999999999999999999999999999999999999999999999999:1
This provides a way for the front end to ask how a particular key should
be labelled right now (specifically, for a given game_state and
game_ui). This is useful on feature phones where it's conventional to
put a small caption above each soft key indicating what it currently
does.
The function currently provides labels only for CURSOR_SELECT and
CURSOR_SELECT2. This is because these are the only keys that need
labelling on KaiOS.
The concept of labelling keys also turns up in the request_keys() call,
but there are quite a few differences. The labels returned by
current_key_label() are dynamic and likely to vary with each move, while
the labels provided by request_keys() are constant for a given
game_params. Also, the keys returned by request_keys() don't generally
include CURSOR_SELECT and CURSOR_SELECT2, because those aren't necessary
on platforms with pointing devices. It might be possible to provide a
unified API covering both of this, but I think it would be quite
difficult to work with.
Where a key is to be unlabelled, current_key_label() is expected to
return an empty string. This leaves open the possibility of NULL
indicating a fallback to button2label or the label specified by
request_keys() in the future.
It's tempting to try to implement current_key_label() by calling
interpret_move() and parsing its output. This doesn't work for two
reasons. One is that interpret_move() is entitled to modify the
game_ui, and there isn't really a practical way to back those changes
out. The other is that the information returned by interpret_move()
isn't sufficient to generate a label. For instance, in many puzzles it
generates moves that toggle the state of a square, but we want the label
to reflect which state the square will be toggled to. The result is
that I've generally ended up pulling bits of code from interpret_move()
and execute_move() together to implement current_key_label().
Alongside the back-end function, there's a midend_current_key_label()
that's a thin wrapper around the back-end function. It just adds an
assertion about which key's being requested and a default null
implementation so that back-ends can avoid defining the function if it
will do nothing useful.
This adds a new bool * argument, which can be NULL if front ends don't
care whether the keypress was handled. Currently they all do that.
Currently, "undo" and "redo" keys are treated as not handled if there's
no move to undo or redo. This may be a little too strict.
The device pixel ratio indicates how many physical pixels there are in
the platonic ideal of a pixel, at least approximately. In Web browsers,
the device pixel ratio is used to represent "retina" displays with
particularly high pixel densities, and also to reflect user-driven
zooming of the page to different text sizes.
The mid-end uses the device pixel ratio to adjust the tile size at
startup, and can also respond to changes in device pixel ratio by
adjusting the time size later. This is accomplished through a new
argument to midend_size() which can simply be passed as 1.0 in any front
end that doesn't care about this.
The developer documentation claims that save files are long ASCII
strings. This is mostly true, but there's nothing stopping a user
from entering non-ASCII characters as random seeds. The ASCII
property of save files is useful, so encode seeds in hex before
writing them unless they consist only of printable ASCII characters.
Hex-encoded seeds are written under a new key, HEXSEED, to distinguish
them from unencoded seeds. This means that old versions of the code
won't be able to load encoded seeds, but that's not a great loss:
seeds aren't generally portable between versions anyway.
That they are ASCII is implied by their inclusion in save files.
Nothing requires an absence of control characters, but it seems polite
to make them slightly readable.
Whenever the midend calls encode_params, it also checks that the
result is a printable ASCII string that doesn't contain '#' or ':'.
Parameter strings are embedded in save files, so they have to fit within
ASCII. They can't contain '#' or ':' because those delimit the
parameter section of a game ID. Nothing explicitly says they can't
contain control characters, but those would be a particularly egregious
violation of the recommendation that parameter strings be easy to type
into a shell.
statepos == 0 shouldn't ever occur in a save file because it indicates
an uninitialised midend. OTOH statepos == nstates is normal. Also
added an equivalent assertion when saving because Simon and I spent
some time discussing whether it could happen.
I just introduced the 'first_draw' flag in the midend, which should
force a screen clear whenever we draw a puzzle with a fresh drawstate.
But in fact there were several places where the midend replaces the
drawstate and I hadn't set that flag to true.
In particular, a user just reported that when you press 'n' for a new
game in an existing Magnets window, the new puzzle's clues are drawn,
but any old clues in places where the new puzzle doesn't have one is
not _un_drawn. (Because Magnets has no code to undraw a single clue -
it never needs to!)
Added a set of first_draw wherever we call new_drawstate, which should
make this reliable again.
I don't know how I've never thought of this before! Pretty much every
game in this collection has to have a mechanism for noticing when
game_redraw is called for the first time on a new drawstate, and if
so, start by covering the whole window with a filled rectangle of the
background colour. This is a pain for implementers, and also awkward
because the drawstate often has to _work out_ its own pixel size (or
else remember it from when its size method was called).
The backends all do that so that the frontends don't have to guarantee
anything about the initial window contents. But that's a silly
tradeoff to begin with (there are way more backends than frontends, so
this _adds_ work rather than saving it), and also, in this code base
there's a standard way to handle things you don't want to have to do
in every backend _or_ every frontend: do them just once in the midend!
So now that rectangle-drawing operation happens in midend_redraw, and
I've been able to remove it from almost every puzzle. (A couple of
puzzles have other approaches: Slant didn't have a rectangle-draw
because it handles even the game borders using its per-tile redraw
function, and Untangle clears the whole window on every redraw
_anyway_ because it would just be too confusing not to.)
In some cases I've also been able to remove the 'started' flag from
the drawstate. But in many cases that has to stay because it also
triggers drawing of static display furniture other than the
background.
The Rockbox frontend allows games to be displayed in a "zoomed-in"
state targets with small displays. Currently we use a modal interface
-- a "viewing" mode in which the cursor keys are used to pan around
the rendered bitmap; and an "interaction" mode that actually sends
keys to the game.
This commit adds a midend_get_cursor_location() function to allow the
frontend to retrieve the backend's cursor location or other "region of
interest" -- such as the player location in Cube or Inertia.
With this information, the Rockbox frontend can now intelligently
follow the cursor around in the zoomed-in state, eliminating the need
for a modal interface.
This is the main bulk of this boolification work, but although it's
making the largest actual change, it should also be the least
disruptive to anyone interacting with this code base downstream of me,
because it doesn't modify any interface between modules: all the
inter-module APIs were updated one by one in the previous commits.
This just cleans up the code within each individual source file to use
bool in place of int where I think that makes things clearer.
This commit removes the old #defines of TRUE and FALSE from puzzles.h,
and does a mechanical search-and-replace throughout the code to
replace them with the C99 standard lowercase spellings.
This changes parameters of midend_size and midend_print_puzzle, the
return types of midend_process_key, midend_wants_statusbar,
midend_can_format_as_text_now and midend_can_{undo,redo}, the 'bval'
field in struct config_item, and finally the return type of the
function pointer passed to midend_deserialise and identify_game.
The last of those changes requires a corresponding fix in clients of
midend_deserialise and identify_game, so in this commit I've also
updated all the in-tree front ends to match. I expect downstream front
ends will need to do the same when they merge this change.
The read function used by midend_deserialise and friends is expected
never to perform a partial read (the main deserialisation code always
knows how many bytes it can expect to see), so it's specified to
return simply TRUE or FALSE for success/failure, rather than the
number of bytes read.
This probably wasn't breaking anything, since in the case of
deserialising from an internal memory buffer a short read could only
arise due to an outright bug constructing the buffer. But now I've
spotted it, I should fix it.
This function gives the front end a way to find out what keys the back
end requires; and as such it is mostly useful for ports without a
keyboard. It is based on changes originally found in Chris Boyle's
Android port, though some modifications were needed to make it more
flexible.
This is another situation in which the midend's state, at the time
midend_new_game tries to save it, is not such as to generate a viable
serialisation. In this case, it's because after the user enters a game
id into the Game > Specific or Game > Random Seed dialog box,
midend_set_config will have _already_ started overwriting important
fields of the midend such as me->desc and me->seed, before
midend_new_game is even entered.
In fact this caused an assertion failure (thanks to Lennard Sprong for
reporting it), because one of those fields was set to NULL, so the
resulting serialisation buffer was incomplete, leading to a
deserialisation error later. But I was lucky: if I hadn't been, the
serialisation might have been complete, and valid according to the
deserialisation code, but would have contained a mixture of the new
game's description and the old one's move chain, which would surely
have had far stranger results.
For the moment, I'm fixing this by simply not storing a serialisation
in that situation. Perhaps a nicer approach might be to store one
before starting to overwrite midend fields, and then have
midend_new_game swap it in if it's already been generated. That way
you _could_ still undo past an action like this. But preventing the
assertion failure is a good start.
