The problem with any changes to the extractors its that the baloo database of the users will not be refreshed.

@mgallien Then they won't benefit from this until a file is reindexed.
Users with baloo disabled will benefit, because in that case baloo_filemetadata_temp_extractor will extract the data on-the-fly.
Reindexing just the ebooks can be done with:

find ~/EBooks/ -type f -name "*.epub" | { mapfile -t; balooctl clear "${MAPFILE[@]}";  balooctl index "${MAPFILE[@]}"; }

In D10694#210409, @michaelh wrote:

@mgallien Then they won't benefit from this until a file is reindexed.
Users with baloo disabled will benefit, because in that case baloo_filemetadata_temp_extractor will extract the data on-the-fly.
Reindexing just the ebooks can be done with:

find ~/EBooks/ -type f -name "*.epub" | { mapfile -t; balooctl clear "${MAPFILE[@]}";  balooctl index "${MAPFILE[@]}"; }

Could you also add a test for the compatibility with current unmodified code ? I would like to be sure that we do not introduce any incompatibility.

I am still not convinced we should change the extractors without an automatic solution for the reindexing case. You will have to ensure everybody can use the property both as a string and a stringlist forever.

The only component I could find to be affected by this change is baloo-widgets. I have already adapted it to this change. And yes, it will handle both.
It will take a some time to publish it because some other stuff has to get reviewed first.
I you know of any component or application using the epub-extractor of KFileMetadata apart from baloo-widgets please let me know.

In D10694#210509, @michaelh wrote:

The only component I could find to be affected by this change is baloo-widgets. I have already adapted it to this change. And yes, it will handle both.
It will take a some time to publish it because some other stuff has to get reviewed first.
I you know of any component or application using the epub-extractor of KFileMetadata apart from baloo-widgets please let me know.

I do understand that you have searched inside KDE hosted code. Still, you are modifying the behavior. To me, it is necessary to have a test that ensures that possible existing clients are not affected by your change. Could you add it ?

I have created T8079 to work on baloo database update when extractors are modified and returned different data.

In D10694#213712, @mgallien wrote:

To me, it is necessary to have a test that ensures that possible existing clients are not affected by your change. Could you add it ?

The new behaviour will break clients. No test needed for this. E.g. baloo-widgets will show the label 'Subject' but no content.
To make it clear: I do not want break this for clients apart from baloo-widgets. The question is: Who are the clients? and How to find them except for searching https://lxr.kde.org/? Please help me with this.

The current behaviour is already breaking: I have some epub files with a lot of dc:subject tags. The semicolon-concatenated string is longer than my monitor is wide. As a result in dolphin the tooltip does not display at all. My rationale was to tackle the problem at the root rather than to work around it by splitting the string within baloo-widgets. With regard to T8079 it will be the same. If data is taken from baloo's database that old long string will be displayed

According to the standard an epub can have multiple dc:subject entries. So it is reasonable to expect a string list from KFileMetaData. Plus the possibilities are grand with an KFileMetaData writer for epubs one could use the tag widget to change them. One could unify xattr tags and epub subjects in tags:// protocol and mayby more.

@mgallien : taglibextractor.cpp is very hard to read. Hopefully it is no longer.

Sorry for being so late.

In D10694#215663, @michaelh wrote:

@mgallien : taglibextractor.cpp is very hard to read. Hopefully it is no longer.

This is bad! I have learned baloo itself doesn't handle stringlists. Which in my view would be the natural way to handle token-like items like tags, keywords and subject(s). Until this is changed in baloo I'm putting this diff on hold and fro the time being work around these long subject-strings within baloo-widgets.

@mgallien: Did I understand you correctly, the refactoring helped you to understand it better? That would be an incentive for me to do more refactoring.

In D10694#221719, @michaelh wrote:

This is bad! I have learned baloo itself doesn't handle stringlists. Which in my view would be the natural way to handle token-like items like tags, keywords and subject(s). Until this is changed in baloo I'm putting this diff on hold and fro the time being work around these long subject-strings within baloo-widgets.

I believe this is quite the opposite. I am already getting strings list for some audio metadata. I would like to get your patch in given you make it works like already existing code and apart from the baloo-widgets code everybody should be fine with your modifications.

@mgallien: Did I understand you correctly, the refactoring helped you to understand it better? That would be an incentive for me to do more refactoring.

This is this patch that made me read the code in baloo that store the properties returned by the extractors. This code is handling strings list quite fine and automatically when a property is added several times.

In D10694#221734, @mgallien wrote:

I believe this is quite the opposite. I am already getting strings list for some audio metadata.

That would be great. Please point me to the respective code in elisa. I want to know which properties and how baloo is involved.
Maybe I gave up too soon. With the old kfilemetadata baloosearch subject:Dummy1 has a result. When kfilemetadata delivers subject as string list is does not. I will reinvestigate.

I would like to get your patch in given you make it works like already existing code and apart from the baloo-widgets code everybody should be fine with your modifications.

Please rephrase. It looks as if some words were deleted. Anyway, baloo-widgets is no problem at all. It's easy to adapt and to debug. It is baloo that's giving me headaches.

In D10694#221734, @mgallien wrote:

In D10694#221719, @michaelh wrote:

This is bad! I have learned baloo itself doesn't handle stringlists. Which in my view would be the natural way to handle token-like items like tags, keywords and subject(s). Until this is changed in baloo I'm putting this diff on hold and fro the time being work around these long subject-strings within baloo-widgets.

I believe this is quite the opposite. I am already getting strings list for some audio metadata. I would like to get your patch in given you make it works like already existing code and apart from the baloo-widgets code everybody should be fine with your modifications.

@mgallien: Did I understand you correctly, the refactoring helped you to understand it better? That would be an incentive for me to do more refactoring.

This is this patch that made me read the code in baloo that store the properties returned by the extractors. This code is handling strings list quite fine and automatically when a property is added several times.

Could you please point me to the code location? I would like to investigate how the code is used for D10803.
Also, since this discussion also applies for the taglibextractor: Is a string list preferred for new properties in KFileMetadata when multiple entries are possible? I think right now most of the properties are using a single concatenated string.

@mgallien: I think we have a beautiful misunderstanding here :-)

In D10694#221719, @michaelh wrote:

This is bad! ...

I was referring to baloo inablility to handle string lists and not to this diff.

In D10694#221734, @mgallien wrote:

I believe this is quite the opposite. ...

I took your answer the wrong way completely. Let's rewind our conversation a little

In D10694#222674, @michaelh wrote:

@mgallien: I think we have a beautiful misunderstanding here :-)

In D10694#221719, @michaelh wrote:

This is bad! ...

I was referring to baloo inablility to handle string lists and not to this diff.

In D10694#221734, @mgallien wrote:

I believe this is quite the opposite. ...

I took your answer the wrong way completely. Let's rewind our conversation a little

No problem from side. In fact this is quite the opposite given you made me better understand all this code. I have now to fix Elisa code to handle lists where I was expecting a single string.

Could you please update your diff and we can land it ? This is a useful improvement.

In D10694#224440, @mgallien wrote:

Could you please update your diff and we can land it ? This is a useful improvement.

1. We can't land it yet. baloo searching breaks with this patch. baloo has be adapted first.
2. I don't know what to update. Please tell me once more what you want me to change.

For this change concerted actions are needed. Let's discuss general questions here: T8196

In D10694#224465, @michaelh wrote:

In D10694#224440, @mgallien wrote:

Could you please update your diff and we can land it ? This is a useful improvement.

1. We can't land it yet. baloo searching breaks with this patch. baloo has be adapted first.

I trust you on this one but on some property the taglib extractor is already doing multiple adds of the same property. It means Baloo is already storing lists.
I can help you but have not much time as usual.

1. I don't know what to update. Please tell me once more what you want me to change.

You can have a look at the taglib extractor. There are multiple examples of for loops adding multiple times the same property.

For this change concerted actions are needed. Let's discuss general questions here: T8196

Thanks, this is a good idea.

Ahhhh.... Either I'm completely off-track here or kfilemetadata is not doing this correctly.
I see a lot of statements like artist += ', ' + value -> no list!
result->add calls QMap->addMulti(). This forces the client to iterate over the map, which is not necessary.
In contrary the value type is announced as string list:

kfilemetadata/src/propertyinfo.cpp: 52
     case Property::AlbumArtist:
            d->name = QStringLiteral("albumArtist");
            d->displayName = i18nc("@label", "Album Artist");
            d->valueType = QVariant::StringList;
            break;

The client can call toVariantMap() on the extraction result to get a QVariantMap but not a QVariant::StringList.
I really, really, really do not understand why kfilemetadata is not delivering a string list. It is much more natural and consistent with the announced Property info.
Also:

kfilemetadata/src/properties.h: 282
typedef QMap<Property::Property, QVariant> PropertyMap;
//In taglibextractor never a list --^`

I'm confused.

Finally I understood how those multiple add()s were meant and it makes more sense now.
We're still not good to go because of this:

$ balooctl index test.epub
Indexing ~/frameworks/kfilemetadata/autotests/samplefiles/test.epub
File(s) indexed

$ balooshow test.epub
177442497910278149 2053 41314051 ~/frameworks/kfilemetadata/autotests/samplefiles/test.epub
        Author: Happy Man
        Title: The Big Brown Bear
        Subject: Baloo;KFile;Meta;Data
        Publisher: Happy Publisher
        Creation Date: 2014-01-01T01:01:01Z

$ baloo_filemetadata_temp_extractor test.epub
QMap(("author", QVariant(QString, "Happy Man"))("creationDate", QVariant(QDateTime, QDateTime(2014-01-01 01:01:01.000 UTC Qt::TimeSpec(UTC))))("publisher", QVariant(QString, "Happy Publisher"))("subject", QVariant(QString, "Baloo;KFile;Meta;Data"))("title", QVariant(QString, "The Big Brown Bear")))
AAAABQAAAA ...

Those semicolons in Baloo;KFile;Meta;Data should not be there and I have no idea where they come from.

The semicolons problem is fixed now. See T8196

Sorry to join late here ...

A QMap can store multiple values for one key, and a client reading the Map can use QMap::values() to get a list of all matching properties. If a client naively uses value() instead, it just gets the first value for the key, but so be it.