[Freeciv-Dev] [patch #1296] restructure basic notify functions

[pepeto]

Update of patch #1296 (project freeciv):

  Status:None => Done   
 Open/Closed:Open => Closed 


___

Reply to this item at:

  

___
  Message posté via/par Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [patch #1296] restructure basic notify functions

[pepeto]

Follow-up Comment #9, patch #1296 (project freeciv):

> One remark: if the packet should be used for another function,
> it should not be changed at all. Does this mean a 'const'
> keyword would be needed? This would again require to make a copy
> of the packet to change the coordinates.

Ok, I took your idea.


(file #6834)
___

Additional Item Attachment:

File name: trunk_S2_2_restructure_notify_conn3.diff Size:7 KB


___

Reply to this item at:

  

___
  Message posté via/par Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [patch #1296] restructure basic notify functions

[Matthias Pfafferodt]

Follow-up Comment #8, patch #1296 (project freeciv):

>> I think the reset of the coordinates (x,y) within the packet
>> is needed, as the same packet struct could also be used for 
>> offline events. 

One remark: if the packet should be used for another function, it should not
be changed at all. Does this mean a 'const' keyword would be needed? This
would again require to make a copy of the packet to change the coordinates.

> I do the same. I am working on Freeciv code for 3 years, and I 
> still ignore some things. 

Nice to know that I'm not alone ;-)

___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [patch #1296] restructure basic notify functions

[pepeto]

Follow-up Comment #7, patch #1296 (project freeciv):

>> if (ptile) {
>> /* Is it useful? */
>> packet->x = ptile->x;
>> packet->y = ptile->y;
>> }
>
>
> I think the reset of the coordinates (x,y) within the packet is
> needed, as the same packet struct could also be used for offline
> events.

Changed this comment to 'Set the coordinates back, to make the packet still
usable.'

>> Why is this not simple strncpy()?
>
>
> I do most of my coding as learning-by-doing. Sorry if I did
> not choose the right function or if there are bad code
> sections. Here I did select the first function which did that
> I want. You did delete the block completely and I think its a
> better solution.

I do the same.  I am working on Freeciv code for 3 years, and I still ignore
some things.

I am sorry if sometimes, the comments looks a bit too affirmative, but
actually, there are advices.  Don't worry if people comment your patch, it
marks at least they have interest on it.

Actually the usage of strncpy() is not the best in my opinion in this case. 
sz_strlcpy() (defined in utility/support.h) alias auto-sized mystrlcpy()
should be faster because it stops when the string ends and unsure it finish
by a nul character.

In your previous patch, you should also have used only: 'packet_send =
*packet;', then all fields, including the string would have been copied.


(file #6829)
___

Additional Item Attachment:

File name: trunk_S2_2_restructure_notify_conn2.diff Size:7 KB


___

Reply to this item at:

  

___
  Message posté via/par Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [patch #1296] restructure basic notify functions

[Matthias Pfafferodt]

Follow-up Comment #6, patch #1296 (project freeciv):

>  if (ptile) {
>/* Is it useful? */
>packet->x = ptile->x;
>packet->y = ptile->y;
>   }

I think the reset of the coordinates (x,y) within the packet is needed, as
the same packet struct could also be used for offline events.

> Why is this not simple strncpy()? 

I do most of my coding as learning-by-doing. Sorry if I did not choose the
right function or if there are bad code sections. Here I did select the first
function which did that I want. You did delete the block completely and I
think its a better solution.

___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [patch #1296] restructure basic notify functions

[pepeto]

Follow-up Comment #5, patch #1296 (project freeciv):

Using only 1 packet structure in notify_conn_packet().


(file #6823)
___

Additional Item Attachment:

File name: trunk_S2_2_restructure_notify_conn.diff Size:7 KB


___

Reply to this item at:

  

___
  Message posté via/par Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [patch #1296] restructure basic notify functions

[pepeto]

Update of patch #1296 (project freeciv):

 Assigned to:None => pepeto 


___

Reply to this item at:

  

___
  Message posté via/par Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [patch #1296] restructure basic notify functions

[pepeto]

Follow-up Comment #4, patch #1296 (project freeciv):

>> + my_snprintf(packet_send.message, sizeof(packet_send.message), "%s",
>> + packet->message);
>
>
> Why is this not simple strncpy()?

Or sz_strlcpy(packet_send.message, packet->message)?

___

Reply to this item at:

  

___
  Message posté via/par Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [patch #1296] restructure basic notify functions

[Marko Lindqvist]

Update of patch #1296 (project freeciv):

 Planned Release:   2.3.0 => 2.2.0  

___

Follow-up Comment #3:

> +  my_snprintf(packet_send.message, sizeof(packet_send.message), "%s",
> +  packet->message);

Why is this not simple strncpy()?


___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [patch #1296] restructure basic notify functions

[Matthias Pfafferodt]

Follow-up Comment #2, patch #1296 (project freeciv):

updated patch; I included the proposed changes:

* create a new struct packet_chat_msg and set the coordinates for each
connection
* code cleanup

(file #6680)
___

Additional Item Attachment:

File name: 0001-restructure-basic-notify-functions.diff Size:8 KB


___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [patch #1296] restructure basic notify functions

[pepeto]

Follow-up Comment #1, patch #1296 (project freeciv):

It looks a good patch.  But I noticed an error in notify_conn_packet(): if
one connection is not allowed to receive the tile coordinates, then the
coordinates in the packets are set to -1 (which is right), but then, if the
next connection of the list is allowed to see that tile, the packet
coordinates are not computed back.

Also, I'm not sure about:
  if (packet->x != -1 && packet->y != -1) {
ptile = map_pos_to_tile(packet->x, packet->y);
  } else {
assert(!is_normal_map_pos(-1, -1));
ptile = NULL;
  }

I would have done:
  if (is_normal_map_pos(packet->x, packet->y)) {
ptile = map_pos_to_tile(packet->x, packet->y);
  } else {
ptile = NULL;
  }
like in client side.


___

Reply to this item at:

  

___
  Message posté via/par Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [patch #1296] restructure basic notify functions

[Matthias Pfafferodt]

URL:
  

 Summary: restructure basic notify functions
 Project: Freeciv
Submitted by: syntron
Submitted on: Sonntag 13.09.2009 um 18:09
Category: general
Priority: 5 - Normal
  Status: None
 Privacy: Public
 Assigned to: None
Originator Email: 
 Open/Closed: Open
 Discussion Lock: Any
 Planned Release: 2.3.0

___

Details:

this patch includes a cleanup of the notify_* functions.

* the new function fill_packet_chat_msg() is used to generate the struct
packet_chat_msg
* vnotify_conn() is replaced by notify_conn_packet()

As all the notify_* functions now have the message as struct packet_chat_msg
this can directly be used for offline events (patch #1280)



___

File Attachments:


---
Date: Sonntag 13.09.2009 um 18:09  Name:
0001-restructure-basic-notify-functions.diff  Size: 8kB   By: syntron



___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev