On 4 April 2018 at 12:36, Jérémie Guichard <[email protected]> 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 -- _______________________________________________ subsurface mailing list [email protected] http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
