Add "Tags" column in Desktop app's dive list view

[dje29]

Add DiveItem::displayTags helper method to return Tags as a QString
New Tags column is
by default inserted before "Photos" column
by default disabled

Signed-off-by: Jeremie Guichard djebrest@gmail.com

Describe the pull request:

• Bug fix
• Functional change
• New feature
• Code cleanup
• Build system change
• Documentation change
• Language translation

Pull request long description:

This change simply adds the possibility to display Dive's tags in the dive list view on Desktop app.

Changes made:

Related issues:

Additional information:

Release note:

Not sure if this kind of small change did need a CHANGELOG update, I did add one just in case.

Documentation change:

A new Tags column can be enabled in Desktop app's dive list view. The new column is by default inserted before Photos column. It is not enabled by default to keep current behavior.

Mentions:

[neolit123]

i think the change in terms of code is good apart from a small white space issue.

but i question the addition in terms of screen real-estate.

a dive can have dozens of tags added to it. having the tags listed in the dive list might be a bit too much detail to have at all times and what size does one need for the dive list column to view all of them?

@dje29 without compiling and looking at this i would give it a -1. can you give us a screenshot? thanks.

@dirkhh
what do you think about this change?

[neolit123]

probably this should not have been changed?

[dirkhh]

yep, please move that curly brace back where it belongs :-)

[neolit123]

i think that our taglist_get_tagstring() could have been implemented in a better way.
https://github.com/Subsurface-divelog/subsurface/blob/master/core/dive.c#L3023
if the strings don't fit the buffer size it does a return i; which isn't the correct way to approach this generic fit-this-in-a-buffer problem.

the return is also never used in our code base.
your code is fine given it follows the our existing usage of taglist_get_tagstring() but this function needs changes.

i can take a look at that when i get some free time.

[dirkhh]

the instinctive reaction is of course "who the heck has more than 1024 characters in tags" - but we all know that we are constantly surprised by what our users do. So I'd prefer an implementation that deals with arbitrary length tag string. But for now this is fine, I think.

[dirkhh]

I like this overall. It's off by default, I know a couple of people who use tags in a very structured manner and this allows "at a glance" review of the tags. I think it will be a minority of users who use this, but that's true for a number of our features.
I mark this as approved here - but please fix the whitespace error...

[dirkhh]

yep, please move that curly brace back where it belongs :-)

[dirkhh]

the instinctive reaction is of course "who the heck has more than 1024 characters in tags" - but we all know that we are constantly surprised by what our users do. So I'd prefer an implementation that deals with arbitrary length tag string. But for now this is fine, I think.

[dje29]

Oups, sorry for that coding style mistake! '}' is now back to its rightful place (the next line).

@neolit123 Here is a screenshot of the extra column when tags do not fit the current column size, Qt cuts the string and adds '...' to show that some tags are not displayed.

Regarding the long tag list (more than 1024) case, I'm still thinking through a proposal at the time of writing :) I currently have some free time (after a year not contributing) so I could look into that topic with some guidance regarding coding guidelines.

[neolit123]

@dje29 thanks for the update.

[neolit123]

so I could look into that topic with some guidance regarding coding guidelines.

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:

char *str_list = taglist_get_tagstring(some_tag_list);
QSting ret(str_list);
free(str_list);
return ret;

thanks.