On Fri, 2012-04-27 at 19:59 +0200, Martin Erik Werner wrote:
> > On Tue, 2012-04-24 at 10:14 +0800, Paul Wise wrote:
> > > On Tue, Apr 24, 2012 at 5:26 AM, Martin Erik Werner wrote:
> > >
> > > > I am looking for a sponsor for my new package "lierolibre"
> > >
> > > A review, since you are upstream too, I'm including some advice
> > > related to that too.
> (...)
> > > I note many files don't have copyright/license headers:
> > >
> > >
> > http://tieguy.org/blog/2012/03/17/on-the-importance-of-per-file-license-information/
> > >
> > I'm aware, I have taken to practice adding copyright headers to all
> > *header* files whose related code I poke in, but since I have not made
> > changes to all bits, many remain unchanged.
>
> I've now added copyright headers globally throughout the source files.
>
>
> > > I would suggest moving the C++ code to a src/ (or similar) subdir.
> > >
> > Yeah, that's probably a good idea, I'll look into that.
>
> done
>
> > > I would suggest deleting sdl.m4 from your VCS and just letting
> > > autotools copy in the one from SDL.
> > Via autoreconf you mean? I'll look into that.
>
> done
>
> > > gvl is both an embedded code copy and a nest of embedded code
> > copies,
> > > some of which are even in Debian (zlib, libpcl1-dev, libtut-dev). My
> > > eyes!
> > Well the zlib is unused, (and now deleted). I was not aware of libpcl
> > and libtut, I'll have a look at ripping those out as well.
> >
>
> All three removed, unused anyways.
>
> (...)
> > > The change weapon button doesn't seem to work.
> > >
> > Do you mean the one for Player2? Yeah, that's the default case for me
> > as well, though I suspect that that's due to me having Alt Gr instead
> > of LAlt, and I'm not sure if it's possible to support both, I'll have
> > to look into that..
>
> This is unchanged for now.
>
> > > Please use dh --parallel or mention in debian/rules why build
> > > parallelism isn't possible to use.
> > >
> > Ah, ok, I'll do that.
> >
> > > I would suggest depending on dh-autoreconf and running dh --with
> > > autoreconf to ensure that the configure script and Makefiles are
> > > always buildable on Debian. This is useful since Debian does rebuild
> > > testing for newer versions of autotools.
> > >
> > Ah, I haven't looked at autotools from a Debian perspective enough it
> > seems, thanks for the hint.
>
> Both added, along with hardening build flags and a debug package.
>
> > > cppcheck warnings:
> > >
> > > [gvl/gvl_test/_build/deque.cpp:51]: (error) Throwing exception in
> > destructor
> > > [gvl/crypt/curve25519.cpp:318]: (error) Memory leak: temp
> > > [gvl/containers/tests/deque.cpp:48]: (error) Throwing exception in
> > destructor
> > > [gvl/crypt/curve25519.cpp:690]: (error) Memory leak: temp1
> > > [gvl/uthread/uthread.cpp:100]: (error) Throwing exception in
> > destructor
> > > [viewport.cpp:132]: (error) Possible null pointer dereference: worm
> > -
> > > otherwise it is redundant to check if worm is null at line 111
> >
> > I'll have to do some digging here :)
>
> All these should be addressed now.
>
> I've made a new upstream release, as per NEWS:
>
> Version 0.2 - April 2012, by Martin Erik Werner
>
> * New dependency on boost_program_options
> * Reworked command-line handling completely
> + Generalised options
> - -f --file : input file
> - -d --dir : input dir
> - -w --write : output file
> - -h --help
> - -v --sdlvideo
> - arg1 is auto-assigned to input file
> - arg2 is auto-assigned to input dir
> - arg3 is auto-assigned to output file
> * Fixed a bug where giving a directory path without the ending "/" would
> set
> the path to the parent directory instead.
> * Defaults to reading data/liero.cfg instead of ./liero.cfg
> * Removal of non-free dtoa piece in gvl
> * Better resolution handling (only for 'Nearest' filter)
> + Game window now defaults to double the size (640x400)
> + Fullscreen now defaults to highest supported screen resolution
> - Game area will grab max multiple of 320x200, rest becomes border
> + Added F7 and F8 keys for x3 and x4 scaling respecively
> * Added ability to quit game via desktop events (window close, alt+F4,
> etc.)
> * Made configure checks for SDL_mixer not depend on pkg-config
> * Error messages from handling config files are now much more verbose
> + If possible gives error type, file, line number, setting path, etc.
> + This is only enabled with libconfig++ version 1.4 and above
> - (libconfig++9 in Debian/Ubuntu)
>
> >
> > IIRC with X11 there are hints you can send to the WM to prevent users
> > from being able to resize to particular sizes, that might be useful to
> > get rid of the bordering.
> The problem here is actually that only integer multiples of 320x200
> works for the currently used Nearest filter, I've used this with the
> scaling keys and with fullscreen, so it's at least reasonable.
>
> Arbitrary window drag-resizing still doesn't work, it appears that
> somehow, SDL stops supplying SDL_RESIZEVIDEO events after a tenth of a
> second of resizing or so, I've not figured out how to solve this...
>
> > Also, I encountered a couple of segfaults when resizing, one in
> > scale2x mode when resizing the window less than
> > 320x200. The other was random while scaling in scale2x mode.
> Yeah, the scale2x is something I've tried and failed at poking at, for
> now it's stuck between 320x200 and 640x400, and frankly I personally
> think that it's a disgrace to the classic pixly feel of Liero, so yeah,
> it will probably not get that much love from me :)
I've made a new 0.3 release which have fixed a few extra things:
* Made it possible to exit from fullscreen via Alt+F4
* Fixed a bug in the launch script that made the liero.cfg file in the
home
directory not be prioritized.
* Improved window/fullscreen resolution behaviour
+ Game grabs maximum possible window size on launch
+ Fullscreen uses host desktop resolution
- Should fix issues with fullscreen not using the best available
resolution
+ Fullscreen now remembers and restores previous window size
+ Drag-resizing the window now works
- Jumps up by integer multiples when window is large enough
* Incomplete support for scale2X on sizes above 640x400
+ Currently just creates black space between pixels
* Binary now defaults to writing the CFG file to the $HOME/settings
directory
(not just the launcher script)
New version is up on mentors:
http://mentors.debian.net/package/lierolibre
dget -x
http://mentors.debian.net/debian/pool/main/l/lierolibre/lierolibre_0.2-1.dsc
And in git:
http://anonscm.debian.org/gitweb/?p=pkg-games/lierolibre.git;a=summary
git://anonscm.debian.org/pkg-games/lierolibre.git
I'd be happy is someone could re-review the package and sponsor it if it
looks ok.
Thanks.
--
Martin Erik Werner <martinerikwerner@gmail.com>
Attachment:
signature.asc
Description: This is a digitally signed message part