Formatting Dive tags string

[Lubomir I. Ivanov]

On 4 April 2018 at 14:27, Lubomir I. Ivanov <neolit123 at gmail.com> wrote:
> On 4 April 2018 at 12:36, Jérémie Guichard <djeBrest at gmail.com> wrote:
>> Hello everybody,
>>
>> While reviewing this change adding display for Tags in dive list view
>> (https://github.com/Subsurface-divelog/subsurface/pull/1184), Lubomir and
>> Dirk raised a concern regarding tag text that can exceed inputted buffer
>> size. Issue comes from implementation/signature of taglist_get_tagstring
>> function
>> https://github.com/Subsurface-divelog/subsurface/blob/master/core/dive.c#L3023
>> Certainly no current user has been using enough tags to actually encounter
>> problems with this but who knows...
>>
>> Staring from their input and doing some code digging, here is a short
>> overview of my findings and proposals.
>>
>> taglist_get_tagstring function currently prints as many full tags as could
>> be fitted in the inputted buffer and returns the length of the printed
>> string. This approach raise several issues when it comes to long tag lists:
>>  a) It is not possible to know from the returned information if all tags
>> were actually printed.
>>  b) When a user would (in the UI) add a long tag list, the full tag list is
>> (at first) indeed saved. Then, only text returned by taglist_get_tagstring
>> gets displayed. This causes later edits of the tags to delete tags that were
>> not displayed, not good...
>>  c) Looking at DiveObjectHelper class the function is used with 256 buffer
>> size so issue would become more visible if DiveObjectHelper::tags was
>> actually used. It seems this method is not used yet (neither in Desktop nor
>> in Mobile app) but I may be wrong, please point me to its usage if you know
>> one...
>>
>> Lubomir mentioned he could look into this 'issue' but did not have much free
>> time, since I do have some on my side I can look into this change. I do
>> prefer to consult the community before doing it though. Here are the
>> different solutions that came to my mind:
>>
>>  1) Add a output parameter to last printed struct tag_entry* in the list to
>> the current taglist_get_tagstring allowing callers to iterate when needed
>> (not my favorite)
>>  2) It seems strange for a UI specific function to be part of dive.h/.c I
>> would rather move this functionality to 'Qt level' say in qthelper.h/.c (or
>> other better location if one of you have a better proposal) and implement it
>> using QStrings (avoiding the pre-allocated buffer issue). This is already
>> what is done for get_gas_string for example. It is my preferred proposal
>> since all usage I could find of taglist_get_tagstring are in Qt code and
>> this function is purely UI related code...
>>
>> I do have other topics to discuss with you guys but I'll send separate
>> emails not to mix things up.
>>
>
> the function is kind of part of the C backend and we probably should
> write it in C.
> posted some comments in the issue thread at github about `
> taglist_get_tagstring` as i saw this ML thread just now.
>

copy paste from here:
https://github.com/Subsurface-divelog/subsurface/pull/1184#issuecomment-378566518

the approach for taglist_get_tagstring() would be:

- make taglist_get_tagstring() accept a single argument (only the
list, without buffer ptr and size)
- go through the tags and calculate required size to malloc() (also
consider ', ')
- malloc() a char * buffer once
- go through the tags again and copy the tags over the buffer while adding ', '
- make taglist_get_tagstring() return NULL / buffer instead of int.
- use this buffer to create QString by the callers of
taglist_get_tagstring() and free this buffer too:

(as Berthold already pointed out)

char *str_list = taglist_get_tagstring(some_tag_list);
QSting ret(str_list);
free(str_list);
return ret;

lubomir
--