Formatting Dive tags string

[Lubomir I. Ivanov]

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.

at some point we wanted to be able to turn the Subsurface core into a
standalone library that can do parsing, planning calculate profiles
etc.
but then we started introducing Qt C++ "helpers" and the idea kind of
went away as we now depend on Qt for the core module.

lubomir
--