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

[Lubomir I. Ivanov]

On 13 January 2013 11:17, Amit Chaudhuri <amit.k.chaudhuri at gmail.com> wrote:
> Hi Lubomir,
>
> [ got delete working :) - just doing a few tests and tidy up]
>
>
> I'm looking through divelist.c at the #ifdef/#endif wrapping around debug
> functions.  This is very clear:
>
> #if DEBUG_SELECTION_TRACKING
>     dump_selection();
> #endif
>
> ...but I'm not so sure about the "& 2" below.  Is this a bit masking
> formulation? How does it work?
>
> #if DECO_CALC_DEBUG & 2
>             dump_tissues();
> #endif
>
>
> I realised I didn't pick up your point on sscanf. Your point below cleared
> up one mystery for me.  I couldn't see why you had extra hidden columns in
> the dialog data; now I see why.  Given the usage of nick name editing is an
> edge case for the user and not on a code line likely to be heavily
> exercised, I doubt the performance hit is material.  I like your technique
> and will remember it for other situations, but I may leave the enum as is
> nevertheless.  If I get more comments I'll reconsider.
>
> I wasn't sure about direct use of Linus' remove_device_info in this context.
> Dirk's function has an additional argument which interacts with
> configuration setting storage I think.  So I've used that for the moment.  I
> get the impression there's a debate to be finished before things get agreed
> and tidy in this corner.
>
> I'm still not 100% happy I understand all the interactions here so I'll try
> and round out my understanding and clean up a few things.  Then I'll put in
> a patch and see what additional comments come back.  Appreciate you taking
> the time.  How's the other work going for you?
>


hello amit,

you should CC the subsurface mailing list, since the application is
developed in public.
other folk could follow the discussion as well.

think of:
#if DECO_CALC_DEBUG & 2

as support for debug levels.

0 & 2 = 0
1 & 2 = 0
2 & 2 = 2
3 & 2 = 2

lubomir
--

> On Sat, Jan 12, 2013 at 1:33 AM, Lubomir I. Ivanov <neolit123 at gmail.com>
> wrote:
>>
>> 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.
>
>