Formatting Dive tags string

[Jérémie Guichard]

Here is the pull request :)
https://github.com/Subsurface-divelog/subsurface/pull/1187

2018-04-04 14:35 GMT+02:00 Lubomir I. Ivanov <neolit123 at gmail.com>:

> 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
> --
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.subsurface-divelog.org/pipermail/subsurface/attachments/20180404/e999c997/attachment.html>