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
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

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.


subsurface mailing list

Reply via email to