RFC - edit nick names dialog added to gtk-gui.c

Lubomir I. Ivanov neolit123 at gmail.com
Fri Jan 11 17:33:26 PST 2013


On 11 January 2013 18:02, Amit Chaudhuri <amit.k.chaudhuri at gmail.com> wrote:
> Hi.
>
> Attached is a diff which partially implements the edit nick names
> functionality.  It leans heavily on the stand alone prototype Lubomir worked
> up and a bunch of discussions with Dirk.  On advice, I've resisted the urge
> to move this to separate files.
>
> I believe it works for editing but for delete it only printf's my intended
> hooks to complete.
>
> The recent introduction of device_info cut across the previous code
> slightly.  With the move to separate files for the newly named struct I had
> to add a function to device.[ch] to return the head of list.  Is this a
> complete no-no?
>
> All comments etc. gratefully received.  As it is my first contribution
> direct to list, please shout if things are wrong / awkward.  I think I'm ok
> on whitespace and sign-offs...
>

hello amit,
i haven't compiled this, but here are some quick points to make.

instead of head_of_device_info_list(), i would just expose the linked
list pointer to be accessible via the device.h header to the rest of
the application.
a function that doesn't do much other than returning an address of a
static C declaration may be redundant...this may remain a "branch",
depending on how smart the compiler treats it.

i'm saying again that the nickname dialog and future dialogs should go
in a separate files and that code has to be further removed from
gtk-gui.c,
instead of more being added to it. then again, that's just an opinion.

if you wish to preserve your debugging code (printf), you can branch
it in pre-compiler macro checks such as #ifdef NICKNAME_DEBUG, but
honestly it shouldn't be present much in the final version. you can
take a look at divelist.c for examples of how its done app wide.

isn't device.c:remove_device_info() sufficient for removal of a
nickname entry? a bit ignorant on my end i must admit...
but you really should remove a nickname entry quite easily...
(there are a couple of things that remain to be cleaned in said
function as well actually)

while i approve of the idea of using sprintf() to populate a hex
string of a device id for the UI once per dialog allocation.
i don't see the need to use sscanf() each time a device id is matched.
instead of sscanf(deviceid_string, "0x%x8", &deviceid);
you can simply store the 32bit value along the gtk tree (i.e. as a
hidden column), which shouldn't be _that_ fragmented, heap wise
unless the implementation specific mapper decides to spaz out and
store it in mordor.
sscanf() is slow, but not so much on a modern, monster CPU...
but i just don't see the need to do string -> uint conversations on
each matching request.

for the final patch try using only /* */ ANSI C styled comments.
also variable declarations are recommended near function entry point.

^ some of these are with "pedantic mode engaged", so don't mind me
that much. :-)

lubomir
--
p.s:
greg k-h, recently made a post about patch reviews not being such a
bad idea, so here that goes.


More information about the subsurface mailing list