• use Qlatin1

In D19098#414973, @bruns wrote:

In D19098#414729, @astippich wrote:

It already does at two different places, because it fuses different information into a single QMap later on (xattr, file size etc...)
https://phabricator.kde.org/source/baloo-widgets/browse/master/src/extractor.cpp$65
https://phabricator.kde.org/source/baloo-widgets/browse/master/src/filefetchjob.cpp$62

This can be done by using a KFM::PropertyMap directly, and adding property types for the UserMetaData (tags, comment, rating). Note, the strings returned by PropertyInfo::name() are not shared ...

It is not only xattr, also everything from kfileitems{group,size,owner...}. Adding these as property with no users in KFileMetaData does not seem clean.
Also, you would have to construct the properties from the name. Why not use the names directly then? Changing everything to a PropertyMap requires a rewrite of large parts, and I certainly will not rewrite baloo-widgets right now.
This is just for a small cleanup.

In D19109#414968, @bruns wrote:

In D19109#414758, @astippich wrote:

A few general remarks:

• I really do not like that there are two lists of supported mimetypes now which have to be kept in sync

I think this is trivial enough. Also this is covered by the unit test.

The stringlist case is still not handled. And a test case for multiple ints (2 ReleaseYear properties) will also likely fail. When going with JSON, I think this has to be done by always inserting values as an array (even if there is only one value), and removing this during deserialization. Stringlist will then be an array in an array in JSON. That will ensure that the map is of the same structure as it was put in, and this should also be possible to do in a backwards compatible way.

• do not use QString::number

• do not use QString::number

• rebase
• use exact value for test

• rebase on master
• use exact value in test

A few general remarks:

• I really do not like that there are two lists of supported mimetypes now which have to be kept in sync
• Do we really need versioning per mimetype? IMHO it is sufficient to have a version number per extractor. From my experience, fixing an extractor usually impacts all its supported mimetypes, and rarily affects only one mimetype. Also, this makes the list hard to maintain, also regarding file types which have multiple mime types, e.g. audio/wav and audio/x-wav
• Do we need an x.y version? I think a single integer is enough or what do you have in mind?
• I prefer to directly construct the qvariantmap in the extractors, and re-use the mimetype list which is already available.

• emphasize test properties

It already does at two different places, because it fuses different information into a single QMap later on (xattr, file size etc...)
https://phabricator.kde.org/source/baloo-widgets/browse/master/src/extractor.cpp$65
https://phabricator.kde.org/source/baloo-widgets/browse/master/src/filefetchjob.cpp$62
I would like to consolidate the two different implementations of the conversion functions. I think it is also useful for debugging purposes, e.g. dumping a property map to cmd line.

Thanks! Who should I ask for a dependency rebuild? sysadmin?
If you don't mind, please also have a look at D19106, which does the same for baloo-widgets. I'd like to land them at the same time for consistency.

This can only be merged when it is okay that KDE applications can rely on KF 5.56

This can of course only be merged when it is deemed okay to rely on KF 5.56

ping

ping. I know this is quite a large diff, but it fixes a potential crash

I was wondering if using QDataStream would be simpler, but the data size was over twice the amount compared to the size using JSON during my testing. But this would solve a lot of the limitations which JSON imposes, like handling int and doubles, qdatetime objects, and there would be no need for merging/unmerging items after fixing KFileMetaData.

If no-one objects, I will land this next week

• fix space

The data is taken from exiv2 library:
https://github.com/Exiv2/exiv2/blob/master/src/tags_int.cpp#L348
Unfortunately, reusing that data would introduce a lot of #ifdefs in the code, hence I added it to KFileMetaData.
This is the only property remaining that needs translation this way which KFileMetaData queries.

In D17302#410189, @bruns wrote:

Currently, both
Result::add(prop, "value1"); Result::add(prop, "value2");
and
Result::add(prop, {"value1", "value2"});
are serialized (JSON) in the same way as {prop: ["value1", "value2"]} by Baloo, which is IMHO fine.

Is this a please don't merge or can I land it?

friendly ping

The unit will be corrected (it is _bit_rate) and three significant digits will always be shown:
123 400 will be displayed as 123 kbit/s
1 234 000 will be displayed as 1.23 Mbit/s
12 345 000 will be displayed as 12.3 Mbit/s

• remove unrelated new lines

will be superseded by D18910

I actually have not started porting Dolphin to the new feature yet, since this will require Kf 5.56 and is probably stuff for 19.08. If a screenshot is a must I will adjust priorities :)

• rebase onto current master
• rename header
• update docs

In D18819#407360, @ngraham wrote:

The default match mode tries both file extension and content; this would make it just do content. So I guess if they don't match, the default match mode uses the file extension rather than content? That seems kinda like the opposite of the behavior you'd want; might be worth a Qt bug report and aldo a comment in the code here explaining why we're doing this.

The change itself seems sane since we're actually doing less, more correct work now.

What more is needed to fix 403902?

Can someone please give this a yes or a no?

Since nobody seems to care and this is just testing the status quo, I'll merge it next week unless somebody objects

• rebase on latest parent revision

• rebase on latest parent revision

In D18601#402905, @bruns wrote:

I think this becomes better structured when you:

1. Create one function per file type, with parameters (Taglib::Filestream, KFM::PropertyMap)
2. From this function, call a generic updateProperties(oldProperties, newProperties) -> mergedProperties
3. Call the type specific function from TaglibWriter::write(...)
   
   Especially when taking the changes for writing the rating into account, this would make the code easier to read - handling of different types just once (not once for reading and once for writing), and no upcasting/dynamic_cast of Taglib::File*. It also saves the heap allocation of the concrete TagLib::File implementation.

• rewrite for better readability and to avoid heap allocation

Just noticed, you never use the AppDataParser.name(). Is that intentional?
Otherwise looks good, but you may want to wait for someone more experienced than me.

got it up and running!
One question: Why is the extra desktop file parser necessary? Shouldn't all required information be available from the app data parser?