[FreeCiv-Java] Re: Upgrading to 1.10 and restructuring Client.java a bit

[Brian Duff <bduff@xxxxxxxxxxxxx>]

Sorry about the long time it took me to respond; my mail program filtered your
message into a different folder for other java work I'm involved in, and I
only just found it.

Artur Biesiadowski wrote:

> Only part of client that I'm not ashamed of is org.freeciv.tile package.
> Of course it could be done better, but I think it is quite extensible,
> object oriented etc. This was the only part of code I wanted to use
> outside the project and thus I spend some time thinking about
> design/interface.

That's about the only part of the code I've not touched so far :).

>
> Rest of code is basically one great hack to stick freeciv client on top
> of map code. Reason for Client being so large was to have a place where
> all things meet - gui, networking, resources etc. I generally hate
> static variables and just use Client as one big pseudo-global object to
> keep all things that can come handy - thus you will see a lot places
> where a reference to client is passed as argument.

I've split a lot of the functionality out into manager classes; the client
holds on to a reference to a manager instance which deals with functionality
in a specific area e.g. there is a DialogManager for invoking dialogs in a way
that nicely hides the implementation from the client, there is a
RulesetManager for dealing with rulesets in general.


> There is too much code in it - for example now I would move tile
> processing away from it (it will have to be done anyway to read new
> tilespec - more on that later). One thing that I would like to defend is
> keeping networking inside Client class - there is really no reason to
> put it into other class, as all network traffic needs to be dispatched
> to various places of system and only Client knows about them all.
> One thing that comes to mind is placing small handling routines inside
> net.Pkg* classes. This might be good way (Client reference would have to
> be passed anyway, but there would be less code in one file), but there
> is a lot of cases where same packet format is used for different
> purposes, just by differenting type of packet. Having case statement
> inside PkgGenericInteger is worse that current way and creating separate
> Pkg* class for each type of request is a bit overkill. There is a way to
> do it without code replication - having all Pkg classes using
> PkgGenericInteger inherit from it and just reimplement handle(Client
> client) method. I'm not convinced, but if you are really worried about
> Client.java size you might consider it. Just remember that there would
> be a big switch statement in Client anyway to create different types of
> Packets. Only bnus would be moving away actual handle*() methods from
> Client to Pkg class (allowing maybe to make a lot of Pkg fields
> private).
>

My feeling was that it would be more extensible to have Handlers for each
incoming packet type; there's an AbstractHandler base class with an abstract
handle() method from which all the Handlers inherit, and a dispacher which
dynamically retrieves the handler for a specific packet type and calls its
handle() method. The dispacher also takes care of instantiating the correct
packet type. I've created an abstract base class for packets which has send()
and recieve() methods on it. The dispacher has an internal array of instances
of handlers; the array indices correspond to the packet types, so this is
probably almost as fast as the original select statement, java's runtime array
checking aside.

The listen() network loop is still inside Client.java, but it delegates most
of its work to the dispacher. Also, I've moved the implementation of this loop
onto a separate thread, because some of the incoming packets have to
manipulate modal dialogs (e.g. when the select nation dialog is on screen,
various nations become disabled as other users choose them. This happens via
an incoming PktSelectNation). The dialog manager helps with this, making sure
that calls involving dialogs are always handled on the AWT event thread so
Swing doesn't get upset.

> I'm not longer actively developing the client, so if you really think
> about working on it, I could handle it to you. For now 'official'
> distribution site for source/data files is cvs server at www.gjt.org.
> There is no problems in updating the source there. I've actually started
> change to 1.9.0 some time ago, but I haven't finished it and thus it is
> not in cvs (as I preferred to have something that works with _some_
> server out there).

Well as I say, I won't check in until I've at least achieved the same level of
functionality that was there before I ripped it to shreds. Which may or may
not take an inordinate amount of time :).

> > Are there any automated tools for generating the java source?, e.g. I
> > noticed that Constants.java has a "do not modify, machine generated"
> > comment at the top, since I need to upgrade this for 1.10.0, it'd be
> > nice not to have to do it by hand (I've been modifying it manually when
> > I need to up till now).
> Code for it is inside org.freeciv.util.EnumParser. It should be written
> in perl, but I don't know perl, so it ended up being in java. It is
> extreme hack, I don't suppose it can parse various enums very well, but
> it kind-of-worked for freeciv source. After generating there is some
> manual work - there are few enums missing, some problems with terminator
> in few places etc, so it is not fire-and-forget thing. I wanted
> something to bootstrap Constants.java file and later I just edited it by
> hand. I suppose that it is still faster/better to edit it by hand,
> especially as you will need only few changed for each thing you
> implement.
>

I seem to have manually got it just about right. I sort of changed the way the
enums are defined in java using a static enum mechanism, but this means that
the constants are no longer strictly compile time constants (e.g. they can't
be used in switch statements), but are still final and static. This does have
the advantage of being less fiddly and easier to maintain, at the loss of some
runtime efficiency.

>
> > This sort of goes for the packet stuff to; in
> > theory it might be possible to "parse" packets.h and/or packets.c and
> > generate the java code from it...
>
> No, I don't think so. In best case you will be able to just generate
> fields from .h definition - and I just use copy/paste for it (editing
> char* to String etc). Most work is inside net routine - reading from
> stream and it is very specific. You have to carefully read C source, to
> detect all traps - some fields are packed, there are various lengths
> fields, existence of some fields depend of values of other etc. I know
> that it is unpleasant work, but I think that doing it by hand (with
> possible occasional copy/paste from C) is only safe way to do this.

Yes, I see what you mean about the C source. Having spent several hours I
think I've got the packet handling stuff mostly right now, but I agree now
that it's probably not possible to automate this without a very sophisticated
parser.

> <tile stuff>

I've not looked into the tile/image stuff yet, although I had noticed that it
would require some triaging because of tilespec. Thanks for the suggestions;
I'll spend some time ploughing through the c code to get an idea of how this
stuff works.

> Another thing to consider is enabling applet support. It is impossible
> with current way of handling images, but if cache could be set up on
> server, it would be possible. Anyway, it would be very network intensive
> - all images and classes would have to be downloaded each time -
> possibly 2-3MB even with compression ?

Yes, I think this would be quite seriously heavy. It would also probably
require the Java plugin, because of the 1.2 dependencies, but that's pretty
much to be expected with applets these days, from what I gather. I've mostly
been working on Java applications for the last few years, and haven't really
played with applets since the early days of Java. Still, applet support would
be a big plus feature wise.

> To reassume, it would be great if you could continue to work on it - I
> think that it has the potential, and for me both city/settler overlays
> and nuke animation just beats gtk client by few lengths :) Tileset
> scaling/change is also a bonus, but except that, there is not much for
> now to compete with gtk version.

I think it's nice that you don't require an X server under Win32 to run the
client :). Although I'm mostly developing under Linux at home, so I don't
suppose that matters to me personally. I've not seen the nuke animation yet
:), but the overlays are a nice feature.

> If you have any questions concerning why I've done something this way,
> or how code works or is supposed to work :) feel free to ask. This list
> is otherwise dead for a long time, so I think that we can discuss
> everything on the forum - just in case somebody else would like to jump
> at some point by reading archives.

I will do; I'm sort of involved in about three open source java things at the
moment, and my enthusiasm for each one waxes and wanes depending on the phase
of the moon and the current price of potatoes. But I'll try to post regular
updates on what I'm doing to this list and any questions I have.

Thanks for the info,

Brian

bduff.vcf
Description: Card for Brian Duff