Wireshark · Ethereal-dev: [ethereal-dev] Re: Wishlist #32 (and 30) (oh and some possible UI enhancements)

["Paul D. Walker" <pdwalker@xxxxxxxxxxxx>]

I have been analyzing why the previous sort patch did not work (as
pointed out by Gilbert) and I have found the following problem:

When the packet data is loaded, there is a structure called the
frame_data structure (defined in packet.h) that appears to hold the
packet information.  This structure has a field called "row" which is
defined as the "row number for this packet in the display"

In file.c, the function add_packet_to_packet_list is filling in the
frame_data->row structure with the current row of the display.  If I
add_packet_to_packet_list, and the packet is the 3rd packet, it gets a
row value of 2 (we are counting from zero).

	packet	clist row	frame_data->row
	1		0		0
	2		1		1
	3		2		2

The problem occurs when we sort the list (ignoring the problem with
the textual sort for a moment).  If I sort the data, the following
happens to our information

	packet	clist row	frame_data->row
	3		0		2
	2		1		1
	1		2		0

We can see that the row number for the sorted list will be different
from the row number stored in the frame_data->row field.

When we select a row (packet_list_select_cb in main.c) we end up
calling select_packet(packet_list, row).  select_packet  searches
through the packet list until row matches the value stored in
frame_data->row.  If we selected the first row, row 0, expecting to
get the details of packet 3, we find out that instead, we get the
value of packet 1.  oops.

This problem will also be a consideration when we try to delete
packets from the packet list window.  Are we going to delete the
packet that is selected by the selection bar (intuitive), or are we
going to delete the packet identified by the position of the selection
bar (unintuitive)?

The question I have to ask (and hopefully someone can answer me) is,
why is the row number being stored in the frame_data->row field?  Is
this just used for sequencing the packets as the come in?  If you are
using this value as the sequence number, then what is the use of the
packet "No." column data?  Shouldn't we be using that instead of
storing it redundantly in the frame_data->row field?  (I haven't
dereferenced the structure to find out where that data is actually
stored yet)

The solution in my mind would be to
	- when selecting the current row,
        retrieve the packet number from the "No." column

	- take this packet number and
        pass it as an argument to the select_packet
        (instead of the clist row)

Therefore, even after sorting the data by whatever column, the
protocol tree and hex view will display the packet that is actually
selected.  (and the delete function would follow)


Ok, assuming that I have not overlooking something glaringly obvious
to those who have spend more time in the code than my two days, this
implementation leads to another set of design problems.

What happens if
	- the "No." column is moved to a different location?
	- the "No." column is removed entirely?

How do we retrieve the packet number?  (was this problem the original
reason why the row value was stored independantly in the frame_data
structure?)

One "problem" with the clist (again, this is my first time using the
gtk library so my lack of knowledge is great) widget is that there is
no way to return data from a row based on the name of the column.
Therefore we would have to loop throught the columns looking for the
"No." column name and then remember the column # where the "No."
column is currently stored.

This is possible.

However, since the packet_list is defined at startup, any changes to
the order of the columns or in the columns that are hidden/revealed
will not take place until the program is restarted.  If the "No."
column is removed, the next time we start the program, our protocol
tree and hex view become broken because there is no "No." column to
identify the packet.

We can get around this by placing a restriction on the "No." column
that prevents it from being removed (or even moved if you like) and
that would solve this problem.  However that solution is not clean and
does not follow the principle of least surprise.

So how about we make all the columns of the packet_list available at
startup?  All that the column preferences would do would be to say
what order the columns appear and what order they appear in.  Any
columns that we are not interested in would be hidden.

This design change would:
	- allow us to display/hide/move columns dynamically
	- take up more memory for the packet list
	  (you HAVE 1GB of ram, don't you?)
	- solve the problem of the disappearing "No." column
	  (as it would be just hidden)
	- allow for a clean implementation of sorting and deleting packet
rows

Summary

- the sorting fails because of the dependance on the frame_data->row
field which no longer has any relevance to the packet information
after a sort

- this problem would affect any delete implementations

- the solution is to rely on using the packet number from the "No."
column

- reliance on the "No." column to identify the packet for displaying
could fail because the "No." column could be moved (and we can search
for it) or taken away entirely (and break our ability to identify and
display the packet in the hex and tree views).

- to solve this problem, we should have all the columns defined in
memory and only display the columns that we are interested in.

- this design change would allow us to move the columns or hide/reveal
the data columns in the clist dynamically without restarting the
program.


Any comments?  Am I barking up the wrong tree or have I misunderstood
the code completely?  I thought I would bounce this off you guys first
before going ahead and doing it (and breaking something else that I am
not aware of completely)

- Paul
pdwalker@xxxxxxxxxxxx