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.
Without this, the "Undo" button ends up greyed even though it actually
works. I'm not sure about whether updating the permalinks is necessary:
maybe we don't need that in new_game() either.
Load/save has been in the JavaScript backend for a while, as have
prettier controls. And JavaScript-capable touchscreens are all around
us, if still poorly supported by Puzzles.
In their normal state, most of the top-level menu items are a verb and
an object, like "Undo move". This is admirably clear, but on a small
screen the menu can take a lot of space. In each case the verb alone
is sufficient to know what the button does, so use a media query to
suppress the noun is the viewport is very narrow. "Very narrow" here
is roughly where the menus would overflow onto four lines in my
browser.
In the old design, when they wrapped onto multiple lines, various bad
things happened. The lines overlapped one another, the lines got
broken within buttons but not between buttons, and if they had got
broken between buttons the left button on each line would have lacked
a left border.
I've made two major changes to fix this. First, I've switched from
flow layout to flex layout. This has much better default behaviour,
breaking lines in the right places, not overlapping lines, and even
arranging line-wrapping within a button when the viewport gets really
narrow.
Second, I've given each button a border on all four sides and then
used negative margins to overlap them. This required changing the
borders from transparent black to opaque grey to make them display
correctly when overlapping.
The result is not quite identical to the old version on a wide
viewport, but I think it's as close as I can get while keeping the new
CSS pleasant.
Ideally, the separator would vanish when it was adjacent to a line
break, but I've not worked out how to do that yet.
- Generate HTML pages from the manual, and install them
- Add "Contents" and "Help on <name>" menu items that will open the
appropriate page in a web browser
Ben Hutchings points out that when I 'harmlessly' changed 'dark_theme'
from a gboolean to a bool, it wasn't harmless, because its address is
passed to g_object_get, which expects a pointer to gboolean. (And of
course it's a variadic function, so it can't type-check that.)
It's got a bit out of date over the years, with some changes to the
code not fully reflected in it (e.g. not all the int -> bool type
changes were documented, and TRUE and FALSE were still mentioned),
and quite a lot of new functions not added. (In particular, the dsf
API was not documented, and it certainly should have been, if only so
that people can find out what it even stands for!)
As well as correcting for factual accuracy, two content changes in the
advice chapter:
I've reworded the definition of 'fairness' to explicitly mention that
requiring the player to use Undo is cheating. That's always how I
_intended_ the definition, but I didn't say it clearly enough.
And I've added an entire new section describing the normal sensible
way to implement redraw(), via a loop of the form 'work out what this
cell should look like, check it against an array in game_drawstate of
the last state we drew it in, and if they're different, call a redraw
function'. That was mentioned in passing in two other sections, but I
know at least one developer didn't find it, so now it's less well
hidden.
Reordered the statements in the fixed Unruly blank_state so that there
doesn't need to be a double if statement (and I think it's more
sensible in any case to put each memset of a freshly allocated array
immediately after the alloc).
In GTK set_window_background, the nest of #ifdefs is now complicated
enough to deserve a few comments on the #else and #endif lines. And
while I was there I switched the gboolean to a bool, on my general
principle that platform-specific boolean types are only worth using
when you're passing them to a platform API function (and perhaps not
even then, if it's not passed by reference).
Commit cc7f550 "Migrate to a CMake-based build system." reversed the
order of xpm_icons, so the largest icon (96x96) is now first and
the smallest (16x16) is now last.
The About dialog now shows the smallest icon, and the window icon is
now the largest icon.
Change the array indexing so that the same size icons are used as
before.
Fixes: cc7f5503dc8f ("Migrate to a CMake-based build system.")
When the user chooses a global dark theme, the window background will
be dark and the default text colour light. Overriding the window
background to be light grey can make the menu and status bars
unreadable. In case a dark theme is used, only set the background
colour for the content area.
References: https://bugs.debian.org/944237
WITHIN() used to treat the min and max as inclusive bounds but was
changed to treat the max as exclusive, apparently accidentally.
Fixed: 5f5b284c0bdd ("Use C99 bool within source modules.")
The common structure is ref-counted and dup_game() bumps the reference
count rather than copying it. However, blank_state() always allocates
a new instance. Add a parameter to control whether blank_state()
allocates it.
Fixes: 47cec547e59a ("Unruly, Group: reference-count the 'immutable' array.")
gcc 6 warns about statements that are indented as if they were meant
to be part of a preceding conditional block. In this case I don't
think that was intended, so shift it left.
References: https://bugs.debian.org/811577
Josh Triplett reported:
> If I ask pearl to generate a 5x5 tricky puzzle, it runs forever.
I find that 5x6 or 6x5 works, so set the minimum accordingly.
References: https://bugs.debian.org/667963
interpret_ui_drag is now called from update_ui_drag, so it makes more
sense to have the former appear first in the file, together with its
comment explaining the expected usage.
A user reported recently that they were trying this as an extra
challenge (solve the whole puzzle mentally and then draw it in
finished form in one UI action). But the backtracking behaviour of
Pearl's dragging mode meant that the loop erased itself as soon as the
drag came back to a revisited position.
In this commit I fix that by making the exception that you can
unconditionally return to the start point of the drag, _provided_ that
in doing so you don't create a grid cell of degree > 2.
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 purpose of check_window_size is to be run after we try to resize
the puzzle window, decide whether the window size needs a further
update, and if so, make it. But the SetWindowPos call that actually
made the update (triggered by the subroutine check_window_resize
returning true to indicate that a change was needed) had mysteriously
gone missing.
An example case where this goes wrong: start up a puzzle at a game
size large enough to need a tile size smaller than default. Then
change the setting to one so small that the menu bar is now the
limiting factor on how small the window can be. (For example, changing
Mosaic from its 50x50 preset to 3x3, if your monitor isn't so huge
that the former fits.) The window comes out the wrong size, and with
this SetWindowPos reinstated, now it gets corrected.
What seems to have happened was that the SetWindowPos was originally
under #ifndef _WIN32_WCE, i.e. we only want to do it on desktop
Windows, not on CE. Then commit 39d299f579da3e9 (introducing manual
window resizing in the Windows front end) moved the call into a
different function, and in the process, accidentally reversed the
sense of the #ifdef. And then commit ff3e762fd007883 (removing the
bit-rotted Windows CE support completely) removed it, along with
everything else under #ifndef _WIN32_WCE!
It was originally supposed to be _enabled_ on desktop Windows, not
disabled. So I've put it back now.
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.
I've had a lot of Puzzles nightly builds fail recently in the NestedVM
stage, with a 'jar' command producing a message along these lines:
java.util.zip.ZipException: attempt to write past end of STORED entry
at java.base/java.util.zip.ZipOutputStream.write(ZipOutputStream.java:337)
at jdk.jartool/sun.tools.jar.Main.copy(Main.java:1250)
at jdk.jartool/sun.tools.jar.Main.copy(Main.java:1263)
at jdk.jartool/sun.tools.jar.Main.addFile(Main.java:1211)
at jdk.jartool/sun.tools.jar.Main.create(Main.java:879)
at jdk.jartool/sun.tools.jar.Main.run(Main.java:319)
at jdk.jartool/sun.tools.jar.Main.main(Main.java:1681)
Suppressed: java.util.zip.ZipException: invalid entry size (expected 0 but got 786 bytes)
at java.base/java.util.zip.ZipOutputStream.closeEntry(ZipOutputStream.java:288)
at java.base/java.util.zip.ZipOutputStream.finish(ZipOutputStream.java:361)
at java.base/java.util.zip.DeflaterOutputStream.close(DeflaterOutputStream.java:238)
at java.base/java.util.zip.ZipOutputStream.close(ZipOutputStream.java:378)
at jdk.jartool/sun.tools.jar.Main.create(Main.java:854)
... 2 more
It's hard to work out exactly what this error dump means, and
web-searching for the error message isn't much help because the same
exception can occur in application code using java.util.zip, and most
mentions on the web are about that, and not about what I want to know,
which is why it might happen in the 'jar' program in particular.
However, the clues visible in that message suggest that 'jar' had
somehow got confused about the size of one of the files it was adding
to the jar archive, in that it initially decided it was 0 bytes long
and later found it was longer. That suggests a problem of excessive
parallelism between the build steps, perhaps due to a missing
dependency in the makefile, which might plausibly cause the 'jar' step
to be running already while some file it needs to read is still being
written. (Which would also explain why it doesn't happen every time.)
An eyeball review of cmake/platforms/nestedvm.cmake didn't find any
obvious missing dependencies. But I vaguely remembered that in some
other context I'd had trouble with cmake 'add_custom_command'. So in
this commit I replace all those custom commands with custom _targets_,
listing the previous OUTPUT files as BYPRODUCTS. And then the
dependencies are written using the target names, instead of the file
names.
I don't fully understand why this should make a difference. But it
seems more reliable in a soak test, and still builds the right things,
so I'll commit it and see if it makes the flakiness in the actual
nightly builds stop happening.
Apparently, some compilers can't work out that the pattern
if (!t->root) { special-case handler followed by early return }
n = t->root;
while (n) { ... }
will execute the while loop at least once, on the grounds that the
_first_ test for n being non-NULL must pass, because we initialised n
from t->root which can't be NULL on any code path where we didn't take
the early return. So they might give an uninitialised-variable warning
for the variable 'ki', which is initialised inside the while loop.
Compilers, eh. But it's easy enough to turn the while into a do-while,
so that even the least alert compiler will know it runs at least once.
Apparently some compilers can't work out that new_window() will always
write to its error-message parameter if it returns failure, so they
complain at the call site that 'error' might be used uninitialised.
Fix by explicitly initialising it. (To NULL, which really _shouldn't_
stop the compiler from warning, because surely that's just as bad if
it reaches the following printf!)
Also, while I'm at it, move it into the block where it's used, so it
doesn't look as if it might pervade the whole of main().
I don't expect this to actually come up in any circumstance, but it
prevents a warning in some versions of gcc that would otherwise arise
from the use of 'int' to compute the input size: if gcc isn't
confident that the int is positive, then it complains that possible
inputs to malloc might be in the region of 2^64 - (small multiple of a
negative 32-bit int).
I would hope malloc would fail in any case on such an input, so
failing a couple of lines earlier makes no important difference.
Annoyingly, stdint.h is missing in my NestedVM build setup (though it
has stdbool.h - it's not _totally_ C90). So I have to check that at
cmake time.
Also, removed the #defines for smalloc and friends from the tree234
test mode. These were needed in the old build system, when
tree234-test was built ad-hoc without being linked against malloc.c.
But now tree234-test links against the same utils library as
everything else, and can use the real smalloc - and doing so prevents
another of these warnings when compiling with -flto.
My standard 'abort on failure' wrappers around malloc and friends look
more or less the same in most of my C software. In this case, they
were so much the same that there was even a comment betraying that I
copy-pasted them from Halibut. And nobody has noticed in the whole
lifetime of this code base :-)
Previously, the typedef 'clue' was just 'char', but it was used in the
expectation that it would be signed. So on platforms that default to
unsigned char, such as 32-bit Arm, Palisade would completely fail to
function correctly.
Every time we append to the string 'ret', we check first that there's
enough space, and realloc it larger if it's getting close to full.
Except that I missed one case at the join between the two parts of the
encoding.
(Spotted because apparently on someone's build platform this led to a
compiler warning that 'ret' might be null. I think _that's_ not a
serious worry, but the missing resize was definitely unintentional.)
Most games store a string in 'aux' during new_game_desc() that saves
solve_game() during gameplay from having to reconstruct the solution
from scratch. Galaxies had all the facilities available to do that,
but apparently just forgot to use them.
(Reindents existing code: diff best viewed with whitespace ignored.)
In the very first stage of game generation, we're supposed to pick 1/5
of the grid squares to put tents in, in a uniformly random manner. It
failed to be uniform, because I had the wrong limit in random_upto -
but it was too small rather than too large, so it never overran the
buffer in a way that something like ASan or valgrind would have caught.
This integer parameter appears in all of the game's anim_length,
flash_length and redraw methods, but the documentation only described
it properly in the section for anim_length.
The section for flash_length refers you to anim_length for the
description of all the parameters, but unhelpfully, it did so without
a conveniently clickable (in HTML) cross-reference.
And the section for game_redraw missed out the description of 'int
dir' completely, which is particularly unhelpful since that's the
function most likely to actually need to care about it! (Even if
forward and reversed move animations look different, they _probably_
still have the same duration, so it's more likely that anim_length
would ignore 'dir' and redraw would use it than vice versa.)
They disappeared in commit c0da615a933a667: I removed all the
per-puzzle code that erased the whole game window on first draw, and
accidentally also took out the code that drew the Tents grid.
Each individual grid tile includes its left and top grid lines, so
most of the grid ended up being drawn anyway by draw_tile(). But the
right and bottom borders aren't within any tile, so they weren't.
It relied on reading gamedesc.txt to find a list of puzzle binaries to
run. But gamedesc.txt is now specific to the Windows build (since it
contains Windows executable names), and isn't available in the Unix
cmake build directory.
Fixed by making a simpler gamelist.txt available on all platforms.
A user recently mentioned having found even 'Easy' to be harder than
they'd like. That difficulty level generates puzzles that can be
solved by filling in any square for which the other colour would
immediately generate a run of 3, and spotting rows that have the
correct total number of one colour and filling in all remaining
squares in the other colour.
Initially I thought there wasn't much I could do to make the solution
techniques easier than that. But after a bit of thought, I decided the
second criterion can be weakened a bit. The new 'Trivial' level
replaces it with a special case: when a row or column only has _one_
remaining unfilled square, the remaining square is filled in by
counting.
That version of the rule doesn't require the player to do any counting
in order to _spot_ possible applications of it: you can see at a
glance that a row or column has only one remaining grey square, even
if having seen it you then have to count to work out which colour to
fill it in. So it makes a gentler introduction to the game.
Patch due to Suller Andras, who noticed that the COL_HIGHLIGHT lines
at the top of the left clue bar and the left of the top one (i.e. the
ones next to the green 'done' button when it appears) were missing,
because the 'done' button was occupying a pixel too much space.
This seems like a generally helpful design for game editors in
general: if we're going to have a helper program that can construct an
instance of a game, then one obvious thing you'd want as output from
it would be the descriptive game id, suitable for pasting back into
the playing UI.
So, in the just-re-enabled helper program 'galaxieseditor', I've
rewritten game_text_format so that it generates exactly that. Now you
can place dots until you have a puzzle you like, then use the 'Copy'
menu item to make it into a game id usable in 'galaxies' proper.
This doesn't set a precedent that I'm planning to _write_ editors for
all the other games, or even any of them (right now). For some, it
wouldn't be too hard (especially games where the solution and clues
are the same kind of thing, like default-mode Solo); for others, it
would be a huge UI nightmare.
Most of these aren't especially useful, but if we're going to have
them in the code base at all, we should at least ensure they compile:
bit-rotted conditioned-out code is of no value.
One of the new programs is 'galaxieseditor', which borrows most of the
Galaxies code but changes the UI so that you can create and remove
_dots_ instead of edges, and then run the solver to see whether it can
solve the puzzle you've designed. Unlike the rest, this is a GUI
helper tool, using the 'guiprogram' cmake function introduced in the
previous commit.
The programs are:
- 'combi', a test program for the utility module that generates all
combinations of n things
- 'divvy', a test program for the module that divides a rectangle at
random into equally-sized polyominoes
- 'penrose-test', a test program for the Penrose-tiling generator
used in Loopy, which outputs an SVG of a piece of tiling
- 'penrose-vector', a much smaller test program for the vector
arithmetic subroutines in that code
- 'sort-test', a test of this code base's local array sorting routine
- 'tree234-test', the exhaustive test code that's been in tree234.c
all along.
Not all of them compiled first time. Most of the fixes were the usual
kind of thing: fixing compiler warnings by removing unused
variables/functions, bringing uses of internal APIs up to date. A
notable one was that galaxieseditor's interpret_move() modified the
input game state, which was an error all along and is now detected by
me having made it a const pointer; I had to replace that with an extra
wrinkle in the move-string format, so that now execute_move() makes
the modification.
The one I'm _least_ proud of is squelching a huge number of
format-string warnings in tree234-test by interposing a variadic
function without __attribute__((printf)).
These look like puzzles, in that they link against a frontend and
provide the usual 'struct game', but they don't count as a puzzle for
purposes of shipping, or even having to have descriptions and icons.
There's one of these buried in the code already under an ifdef, which
I'll re-enable in the next commit.
I just found out, in GTK3 under X11, that if you use the 'Copy' menu
item to copy a text representation of the game to the clipboard,
afterwards the puzzle window is no longer redrawn.
That was pretty mysterious, and I still don't _fully_ understand it.
But I think the main point is that when you set the GtkDrawingArea to
be the owner of an X11 selection, that requires it to have an X11
window of its own, where previously it was managing fine as a logical
subrectangle of its containing GtkWindow. And apparently switching
strategies half way through the run is confusing to GTK, and causes
redraws to silently stop working.
The easy workaround is to make fe->window rather than fe->area the
thing that owns GTK selections and receives the followup events. That
must already have an X window, so nothing has to be changed when we
make it a selection owner half way through the run.
In this platform's set_platform_puzzle_target_properties, I referred
to ${EXENAME} twice, which is not one of the function parameters. It
worked anyway, because CMake has dynamic scope, and that variable was
defined - to the right value - within the local-variable scope of the
calling function. But that wasn't at all what I meant to do!
Renamed it to ${TARGET}, which is the actual parameter name we get
passed.
The previous fix worked OK, but it was conceptually wrong. Puzzles
save files are better regarded as binary, not text: the length fields
are measured in bytes, so translating the file into a different
multibyte character encoding would invalidate them.
So it was wrong to fetch a C byte string containing the exactly right
binary data, then translate it into a Javascript string as if decoding
from UTF-8, then retranslate to a representation of a bytewise
encoding via encodeURIComponent, and then label the result as
application/octet-stream.
This probably wouldn't have caused any problems in practice, because I
don't remember any situation in which my save files use characters
outside printable ASCII (plus newline). But it's not actually
forbidden, so a save file might choose to do that some day, so that
UTF-8 decode/reencode hidden in the JS was a latent bug.
Now the URI-encoding is done on the C side, while we still know
exactly what the binary data ought to look like and can be sure we're
translating it byte for byte into the output encoding for the data:
URI. By the time the JS receives a string pointer from get_save_file,
it's already URI-encoded, which _guarantees_ that it's in ASCII and
won't be messed about with by Emscripten's UTF8ToString.
