<div dir="ltr">Here is the pull request :) <a href="https://github.com/Subsurface-divelog/subsurface/pull/1187">https://github.com/Subsurface-divelog/subsurface/pull/1187</a></div><div class="gmail_extra"><br><div class="gmail_quote">2018-04-04 14:35 GMT+02:00 Lubomir I. Ivanov <span dir="ltr"><<a href="mailto:neolit123@gmail.com" target="_blank">neolit123@gmail.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On 4 April 2018 at 14:27, Lubomir I. Ivanov <<a href="mailto:neolit123@gmail.com">neolit123@gmail.com</a>> wrote:<br>
> On 4 April 2018 at 12:36, Jérémie Guichard <<a href="mailto:djeBrest@gmail.com">djeBrest@gmail.com</a>> wrote:<br>
>> Hello everybody,<br>
>><br>
>> While reviewing this change adding display for Tags in dive list view<br>
>> (<a href="https://github.com/Subsurface-divelog/subsurface/pull/1184" rel="noreferrer" target="_blank">https://github.com/<wbr>Subsurface-divelog/subsurface/<wbr>pull/1184</a>), Lubomir and<br>
>> Dirk raised a concern regarding tag text that can exceed inputted buffer<br>
>> size. Issue comes from implementation/signature of taglist_get_tagstring<br>
>> function<br>
>> <a href="https://github.com/Subsurface-divelog/subsurface/blob/master/core/dive.c#L3023" rel="noreferrer" target="_blank">https://github.com/Subsurface-<wbr>divelog/subsurface/blob/<wbr>master/core/dive.c#L3023</a><br>
>> Certainly no current user has been using enough tags to actually encounter<br>
>> problems with this but who knows...<br>
>><br>
>> Staring from their input and doing some code digging, here is a short<br>
>> overview of my findings and proposals.<br>
>><br>
>> taglist_get_tagstring function currently prints as many full tags as could<br>
>> be fitted in the inputted buffer and returns the length of the printed<br>
>> string. This approach raise several issues when it comes to long tag lists:<br>
>>  a) It is not possible to know from the returned information if all tags<br>
>> were actually printed.<br>
>>  b) When a user would (in the UI) add a long tag list, the full tag list is<br>
>> (at first) indeed saved. Then, only text returned by taglist_get_tagstring<br>
>> gets displayed. This causes later edits of the tags to delete tags that were<br>
>> not displayed, not good...<br>
>>  c) Looking at DiveObjectHelper class the function is used with 256 buffer<br>
>> size so issue would become more visible if DiveObjectHelper::tags was<br>
>> actually used. It seems this method is not used yet (neither in Desktop nor<br>
>> in Mobile app) but I may be wrong, please point me to its usage if you know<br>
>> one...<br>
>><br>
>> Lubomir mentioned he could look into this 'issue' but did not have much free<br>
>> time, since I do have some on my side I can look into this change. I do<br>
>> prefer to consult the community before doing it though. Here are the<br>
>> different solutions that came to my mind:<br>
>><br>
>>  1) Add a output parameter to last printed struct tag_entry* in the list to<br>
>> the current taglist_get_tagstring allowing callers to iterate when needed<br>
>> (not my favorite)<br>
>>  2) It seems strange for a UI specific function to be part of dive.h/.c I<br>
>> would rather move this functionality to 'Qt level' say in qthelper.h/.c (or<br>
>> other better location if one of you have a better proposal) and implement it<br>
>> using QStrings (avoiding the pre-allocated buffer issue). This is already<br>
>> what is done for get_gas_string for example. It is my preferred proposal<br>
>> since all usage I could find of taglist_get_tagstring are in Qt code and<br>
>> this function is purely UI related code...<br>
>><br>
>> I do have other topics to discuss with you guys but I'll send separate<br>
>> emails not to mix things up.<br>
>><br>
><br>
> the function is kind of part of the C backend and we probably should<br>
> write it in C.<br>
> posted some comments in the issue thread at github about `<br>
> taglist_get_tagstring` as i saw this ML thread just now.<br>
><br>
<br>
</div></div>copy paste from here:<br>
<a href="https://github.com/Subsurface-divelog/subsurface/pull/1184#issuecomment-378566518" rel="noreferrer" target="_blank">https://github.com/Subsurface-<wbr>divelog/subsurface/pull/1184#<wbr>issuecomment-378566518</a><br>
<br>
the approach for taglist_get_tagstring() would be:<br>
<br>
- make taglist_get_tagstring() accept a single argument (only the<br>
list, without buffer ptr and size)<br>
- go through the tags and calculate required size to malloc() (also<br>
consider ', ')<br>
- malloc() a char * buffer once<br>
- go through the tags again and copy the tags over the buffer while adding ', '<br>
- make taglist_get_tagstring() return NULL / buffer instead of int.<br>
- use this buffer to create QString by the callers of<br>
taglist_get_tagstring() and free this buffer too:<br>
<br>
(as Berthold already pointed out)<br>
<br>
char *str_list = taglist_get_tagstring(some_<wbr>tag_list);<br>
QSting ret(str_list);<br>
free(str_list);<br>
return ret;<br>
<br>
lubomir<br>
--<br>
</blockquote></div><br></div>