On Mon, 29 Oct 2012 02:11:10 AM Saagar Sethi wrote: > So we have fixed one of the bugs we explicitly said we were going to fix, > which was the hammers carrying-over.
OK, I looked at the patch, and there are several issues, the last of which is
a show stopper.
- There is no need to add a GUI to BuildQueuePanel. It is a FreeColPanel, so
it can get the GUI by calling getGUI().
- Correct me if I am wrong, but does this patch only handle the case of
removing the first item in the build queue? If so, it can be subverted by
inserting a new item at position zero. Should this be allowed?
- The arguments to showConfirmDialog need to be references to messages defined
in FreeColMessages.properties (see FreeColClient.askToQuit() for a simple
example).
- In BuildQueuePanel.java there is a comment that it would be better not to
have to hard code the reference to model.goods.hammers. I agree. The way I
did this in the AI planning code was to select goods types such that
isBuildingMaterial is true and isStorable is false.
- Please do not use tabs.
> We have added an option
> "model.option.clearHammersOnConstructionSwitch" (feel free to rename) in
> FreeColServer.fixGameOptions. When this is true, if you remove the first
> buildable in the buildQueue, it will pop up a dialog if you are sure, and
> if you are, it will remove the buildable and reset your hammers to 0. We're
> a bit unsure how to get this as an option in the specifications XML file.
- Look at similar options like saveProductionOverflow. IMHO clearHammers*
belongs right there with it, in the game options colony group. Note that
clearHammers* should be off by default in the classic rules.
- You should use the CLEAR_HAMMERS_ON... constant you define in
GameOptions.java when evaluating that option.
> You mentioned that this was a minor fix, but it actually took us a decent
> amount of time (I guess just to learn the code base wellenough).
Well said, and you have bumped into some code that is not that obvious...
> What we
> did was in BuildQueuePanel.java, in the BuildQueueMouseAdapter class's
> mousePressed, we check if we are aboutto remove the currentlyBuilding
> object.
- This works, but only for the mouse clicking method of deleting the current
buildable. It does not handle the keyboard delete method, or dragging the
buildable back onto the Buildings list.
> If so, we prompt with the dialog, and if they hit ok, we set the
> colony's hammers to 0. Interestinglywe have to do this again in ColonyWas,
> for the change to take effect once we exit the buildQueuePanel.
- This not a good idea. The purpose of ColonyWas is to fire client property
changes when a colony changes state, not to change the state itself. Please
do not do this.
> Do you know why this is?
- FreeCol is a client-server application. In BuildQueuePanel and ColonyWas
you are manipulating the client. Real changes only happen in the server.
With your patch, if I bring up a colony with non-zero hammers, and remove the
current buildable, the hammers appear to be zero, but this is an illusion:-).
Do something that causes the colony to be updated from the server (like moving
a unit to a different building) and the hammers count goes back to what it was
originally. It is never changed in the server.
The line that is critical here in BuildQueuePanel.java is this:
getController().setBuildQueue(colony, buildables);
When that fires, the client InGameController sends a SetBuildQueue message to
the server, and receives an update back that modifies the client values. On
the server side, SetBuildQueue messages are ultimately handled by
setBuildQueue() in the server InGameController. That is where you need to put
the logic to clear the hammers.
What you are currently doing is clearing the hammers once on the client side
in the mouse handler, but this is reverted when you quit the panel and the
server is called to update the build queue, however hooking into ColonyWas
allows you in turn to undo the effect of the server update. Temporarily at
least.
Cheers,
Mike Pope
signature.asc
Description: This is a digitally signed message part.
------------------------------------------------------------------------------ Everyone hates slow websites. So do we. Make your web apps faster with AppDynamics Download AppDynamics Lite for free today: http://p.sf.net/sfu/appdyn_sfd2d_oct
_______________________________________________ Freecol-developers mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/freecol-developers
