[Sugar-devel] [PATCH Maze 4/4] Save and restore state of the game

[Sascha Silbe]

Excerpts from Manuel Kaufmann's message of 2012-03-21 02:52:54 +0100:


First of all, thanks for the patch! Your help is very welcome.

Disclaimer: I'm neither the maintainer of Maze nor a regular
contributor to it. My opinions and advice may differ from what you
need to do to get your patch accepted by the Maze maintainer.


[patch description]
> The data is saved (as JSON, with simplejson module) in the 'event.filename'
> path using the 'sugar.datastore' API.
>
> A filename example is:
> 
>  ~/.sugar/default/datastore/09/0940b7eb-1cb7-4687-aa7a-ac2c174dcfd7/data

The file name is an implementation detail of the current version of
sugar-datastore and shouldn't usually be mentioned in the commit
message of an unrelated component.


[game.py]
> @@ -23,13 +23,23 @@
>  
>  import sys
>  import time
> -
> +import simplejson
[...]

At least in Glucose [1], we're trying to get rid of simplejson. If you
want Maze to continue to run on systems that don't have Python 2.6
yet, you can do the following to use the standard json module on
"modern" systems and fall back to simplejson on older ones:

try:
    import json
except ImportError:
    import simplejson as json


> +import tempfile
> +import shutil
>  import pygame
> +import olpcgames
> +
> +import logging
> +log = logging.getLogger('Maze')
> +log.setLevel(logging.DEBUG)
> +logging.basicConfig()

sugar-activity will already initialise the logging subsystem for you,
so no need for the setLevel() and basicConfig() calls.

To set the log level, you can set the environment variable
SUGAR_LOGGER_LEVEL. There are generally three ways to do that:

1. In ~/.sugar/debug (contains commented-out examples).

   Easiest to do, affects all new Sugar sessions, works even if Sugar
   is started directly by the display manager.

2. Setting it in your shell before starting Sugar.

   Only affects the about-to-be-started Sugar session, which may be
   exactly what you want (in some cases).

3. Setting it in the Terminal (activity) shell before starting an
   activity session manually using sugar-activity.

   Only affects the about-to-be-started activity session, not Glucose
   or other activities.


[...]
[MazeGame.__init__()]
> -        # start with a small maze using a seed that will be different
> -        # each time you play
> -        self.maze = Maze(int(time.time()), int(9 * self.aspectRatio), 9)
> -        self.reset()
> +        # no data is known yet (maybe the user is resuming a state or
> +        # launching a new instance)
> +        self.data = None
> +

See below for comments regarding this change.


> @@ -283,8 +293,42 @@ class MazeGame:
>                      (event, sys.exc_info())
>              else:
>                  print "Message from unknown buddy?"
> +
> +        elif event.type == pygame.USEREVENT:
> +            # process fil save / restore events

typo: file


> +            if event.code == olpcgames.FILE_READ_REQUEST:
> +                log.debug(event)

The first argument to the log.<level> family of functions is a format
string. If you wish to log the value of some variable, you should use
%s or %r in the format string and pass the variable as second+ parameter:

                log.debug('event=%r', event)

Depending on what exactly you're logging, this mistake may even be
exploitable [2,3], similar to printf() exploits with C.


> +                log.debug('Loading the state of the game...')
> +                self.data = simplejson.loads(open(event.filename, 'r').read())
> +                log.debug('Loaded data: %s' % self.data)
> +                return True

Why do you store the intermediate data, rather than passing it to a
Maze instance (that has previously been instantiated with default
values) or creating a new instance right away?


> +            elif event.code == olpcgames.FILE_WRITE_REQUEST:
> +                log.debug(event)
> +                log.debug('Saving the state of the game...')
> +                data = {'seed': self.maze.seed,
> +                        'width': self.maze.width,
> +                        'height': self.maze.height,
> +                    }
> +                log.debug('Saving data: %s' % data)

As explained above, log.<level> can do the formatting for you:

                log.debug('Saving data: %s', data)

The advantage of this is that the formatting is only do if this line
actually gets logged, providing a minor performance benefit on systems
that don't have debug logging enabled.


> +                file_dsobject = datastore.create()
> +                file_dsobject.metadata = event.metadata

You're creating a new Journal entry every time, rather than updating
an existing entry. This will result in Journal spam as activities are
expected to save their state whenever you switch away from them. It
also won't match the behaviour outlined in the Human Interface
Guidelines (HIG [4]) and implemented by other Sugar activities.

sugar.activity.activity.Activity already implements all of the logic
you need and AFAICT it should work similarly with olpcgames. Just
write your JSON data to the file (path) you were given, optionally
updating metadata as well. sugar.activity.activity.Activity.save()
will take care of creating or updating the Journal entry.


> +                temp_filename = tempfile.mktemp()

There's no need to use another temporary file. The file (name) you get
passed down from write_file() already is a temporary one. Ownership of
that file will pass through save() to the data store, which will
move it into place (no copying involved on usual systems).


> @@ -357,9 +401,28 @@ class MazeGame:
>      def run(self):
>          """Run the main loop of the game."""
>          # lets draw once before we enter the event loop
> +
> +        # We have to update the clock to get the first event because
> +        # the game could be called from the Journal and we need to
> +        # catch the olpcgames.FILE_READ_REQUEST event to load the last
> +        # state of the game
> +        clock = pygame.time.Clock()
> +        clock.tick(25)
> +        for event in pausescreen.get_events(sleep_timeout=30):
> +            self.processEvent(event)

I don't know olpcgames, so I can't say whether this is a reasonable
approach. From my experience with other systems I'd guess there's at
least a better way to do it.


> +        if self.data == None:
> +            # start the game with default values (easy)
> +            self.data = {'seed': int(time.time()),
> +                         'width': int(9 * self.aspectRatio),
> +                         'height': 9}
> +
> +        log.debug('Starting the game with: %s' % self.data)
> +        self.maze = Maze(**self.data)
> +        self.reset()

As mentioned in passing above, it should be easier to continue
instantiating Maze in the constructor using default values (for the
case of a new activity instance) and replacing it with a new instance
from within read_file() (resp. the FILE_READ_REQUEST event handler)
for the case of continuing an activity session from the Journal. The
Collaboration code in Maze works similar (check out the handler for
the "maze:" message in game.MazeGame.handleMessage()), so I don't
expect any blocker on this path.

Sascha

[1] https://wiki.sugarlabs.org/go/Taxonomy#Glucose:_The_base_Sugar_environment
[2] https://en.wikipedia.org/wiki/Uncontrolled_format_string
[3] http://drdobbs.com/security/197002914?pgno=3
[4] https://wiki.sugarlabs.org/go/Human_Interface_Guidelines
-- 
http://sascha.silbe.org/
http://www.infra-silbe.de/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://lists.sugarlabs.org/archive/sugar-devel/attachments/20120326/34bbfab4/attachment-0001.pgp>