Thanks to Jason Hood for the report. The crash is trivially reproduced
under Address Sanitizer if you set up the game id 15x15#12345 and then
use the Solve UI action.
The text format includes newline characters that weren't being
included in the buffer length calculation. Fix the calculation and
assert before returning that the string offset matches the calculated
length.
As far as I could tell, cur_x, cur_y, prev_cur_x, and prev_cur_y in
game_drawstate were never used. prev_cur_x and prev_cur_y in game_ui
were assigned but never referenced. So they may as well all go.
This adds an extra parameter to move_cursor() that's an optional pointer
to a bool indicating whether the cursor is visible. This allows for
centralising the common idiom of having the keyboard cursor become
visible when a cursor key is pressed. Consistently with the vast
majority of existing puzzles, the cursor moves even if it was invisible
before, and becomes visible even if it can't move.
The function now also returns one of the special constants that can be
returned by interpret_move(), so that the caller can correctly return
MOVE_UI_UPDATE or MOVE_NO_EFFECT without needing to carefully check for
changes itself.
Callers are updated only to the extent that they all pass NULL as the
new argument. Most of them could now be substantially simplified.
A test-build with a modern clang points out a number of 'set but not
used' variables, which clang seems to have got better at recently.
In cases where there's conditioned-out or commented-out code using the
variable, I've left it in and added a warning-suppressing cast to
void. Otherwise I've just deleted the variables.
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().
These are similar to the existing pair configure() and custom_params()
in that get_prefs() returns an array of config_item describing a set
of dialog-box controls to present to the user, and set_prefs()
receives the same array with answers filled in and implements the
answers. But where configure() and custom_params() operate on a
game_params structure, the new pair operate on a game_ui, and are
intended to permit GUI configuration of all the settings I just moved
into that structure.
However, nothing actually _calls_ these routines yet. All I've done in
this commit is to add them to 'struct game' and implement them for the
functions that need them.
Also, config_item has new fields, permitting each config option to
define a machine-readable identifying keyword as well as the
user-facing description. For options of type C_CHOICES, each choice
also has a keyword. These keyword fields are only defined at all by
the new get_prefs() function - they're left uninitialised in existing
uses of the dialog system. The idea is to use them when writing out
the user's preferences into a configuration file on disk, although I
haven't actually done any of that work in this commit.
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.
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.
This fixes a build failure introduced by commit 2e48ce132e011e8
yesterday.
When I saw that commit I expected the most likely problem would be in
the NestedVM build, which is currently the thing with the most most
out-of-date C implementation. And indeed the NestedVM toolchain
doesn't have <tgmath.h> - but much more surprisingly, our _Windows_
builds failed too, with a compile error inside <tgmath.h> itself!
I haven't looked closely into the problem yet. Our Windows builds are
done with clang, which comes with its own <tgmath.h> superseding the
standard Windows one. So you'd _hope_ that clang could make sense of
its own header! But perhaps the problem is that this is an unusual
compile mode and hasn't been tested.
My fix is to simply add a cmake check for <tgmath.h> - which doesn't
just check the file's existence, it actually tries compiling a file
that #includes it, so it will detect 'file exists but is mysteriously
broken' just as easily as 'not there at all'. So this makes the builds
start working again, precisely on Ben's theory of opportunistically
using <tgmath.h> where possible and falling back to <math.h>
otherwise.
It looks ugly, though! I'm half tempted to make a new header file
whose job is to include a standard set of system headers, just so that
that nasty #ifdef doesn't have to sit at the top of almost all the
source files. But for the moment this at least gets the build working
again.
C89 provided only double-precision mathematical functions (sin() etc),
and so despite using single-precision elsewhere, those are what Puzzles
has traditionally used. C99 introduced single-precision equivalents
(sinf() etc), and I hope it's been long enough that we can safely use
them. Maybe they'll even be faster.
Rather than directly use the single-precision functions, though, we use
the magic macros from <tgmath.h> that automatically choose the precision
of mathematical functions based on their arguments. This has the
advantage that we only need to change which header we include, and thus
that we can switch back again if some platform has trouble with the new
header.
If you define PUZZLES_INITIAL_CURSOR=y, puzzles that have a keyboard
cursor will default to making it visible rather than invisible at the
start of a new game. Behaviour is otherwise the same, so mouse actions
will cause the cursor to vanish and keyboard actions will cause it to
appear. It's just the default that has changed.
The purpose of this is for use on devices and platforms where the
primary or only means of interaction is keyboard-based. In those cases,
starting with the keyboard cursor invisible is weird and a bit
confusing.
Thanks to Larry Hastings for the patch (tweaked to include drag/release).
(cherry picked from Android port,
commit 377e61b144c518a3d9efba66be08bf00ff6596e8)
Mosaic's validate_desc() doesn't write to the description string, so
it has no need to make a copy of it. And if it doesn't copy it, it
can't leak the copy.
Using sfree() rather than free_game() in the error paths meant that
various arrays referenced from the game_state weren't properly freed.
Also one error path didn't free the game_state at all.
If can_configure is false, then the game's configure() and
custom_params() functions will never be called. If can_solve is false,
solve() will never be called. If can_format_as_text_ever is false,
can_format_as_text_now() and text_format() will never be called. If
can_print is false, print_size() and print() will never be called. If
is_timed is false, timing_state() will never be called.
In each case, almost all puzzles provided a function nonetheless. I
think this is because in Puzzles' early history there was no "game"
structure, so the functions had to be present for linking to work. But
now that everything indirects through the "game" structure, unused
functions can be left unimplemented and the corresponding pointers set
to NULL.
So now where the flags mentioned above are false, the corresponding
functions are omitted and the function pointers in the "game" structures
are NULL.
Only numbers and lower-case letters are allowed. Without this
restriction, a buffer overrun is possible.
To demonstrate the problem, load this save file into a build of Mosaic
with AddressSanitizer:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
VERSION :1:1
GAME :6:Mosaic
PARAMS :7:8x8a0h1
CPARAMS :7:8x8a0h1
DESC :41:b2c3b~~2a5c6e3a55c6a5a4244e0c3a64d4b4232b
NSTATES :1:1
STATEPOS:1: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.
Every call to draw_cell() was drawing a region including the whole
border of the cell, so that the calls overlapped. So if the cursor
moved left or up, then a COL_CURSOR outline would be drawn around the
new cell, and then a COL_GRID outline would be drawn around the old
cell, overwriting part of the cursor border.
I've fixed this in the rigorous way, by making draw_cell() calls cover
disjoint areas of the puzzle canvas, and using clip() to enforce that.
So now the single DRAWFLAG_CURSOR is replaced by a system of four
flags, indicating that the cell being drawn is the actual cursor
position, or the cell below it (hence containing the cursor's bottom
border), or to its right (needing the left border), or below _and_ to
the right (you still need the single pixel at the cursor's bottom
right corner!).
Also, to ensure the cursor edges are drawn even on the bottom or right
grid boundaries, draw_cell() is called for a set of virtual cells
beyond the actual grid bounds, with additional flags telling it not to
draw an actual puzzle cell there, just the relevant pieces of border.
The default setting for 'aggressiveness' is true. But the extra suffix
to specify it in the full version of the encoded game params was being
set if aggressiveness _was_ true, not if it was false. Result: round
trip encode/decode of a non-aggressive configuration fails, because
the encoded string has no aggressiveness suffix, and the decoder
interprets that as going back into aggressive mode.
Pulled out the default setting into a #define which is checked
consistently in both locations.
Negative numbers are used as a sentinel for an absent clue, so we have
to use a type that's guaranteed to have some negative numbers. char is
unsigned on some platforms. So now Mosaic runs apparently correctly on
Raspbian, for example.
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.
This is similar in concept to Minesweeper, in that each clue tells you
the number of things (in this case, just 'black squares') in the
surrounding 3x3 grid section.
But unlike Minesweeper, there's no separation between squares that can
contain clues, and squares that can contain the things you're looking
for - a clue square may or may not itself be coloured black, and if
so, its clue counts itself.
So there's also no hidden information: the clues can all be shown up
front, and the difficulty arises from the game generator choosing
which squares to provide clues for at all.
Contributed by a new author, Didi Kohen. Currently only has one
difficulty level, but harder ones would be possible to add later.
