Here is the pull request :) https://github.com/Subsurface-divelog/subsurface/pull/1187
2018-04-04 14:35 GMT+02:00 Lubomir I. Ivanov <neolit...@gmail.com>: > On 4 April 2018 at 14:27, Lubomir I. Ivanov <neolit...@gmail.com> wrote: > > On 4 April 2018 at 12:36, Jérémie Guichard <djebr...@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 > -- >
_______________________________________________ subsurface mailing list subsurface@subsurface-divelog.org http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface