D12950: add test which checks the property types

[Alexander Stippich]

astippich updated this revision to Diff 51550.
astippich added a comment.


  - fix space

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12950?vs=48691=51550

BRANCH
  property_tests

REVISION DETAIL
  https://phabricator.kde.org/D12950

AFFECTED FILES
  autotests/taglibextractortest.cpp
  autotests/taglibextractortest.h

To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18972: Format EXIF photo flash data

[Alexander Stippich]

astippich added a comment.


  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.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18972

To: astippich, ngraham
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18972: Format EXIF photo flash data

[Alexander Stippich]

astippich created this revision.
astippich added a reviewer: ngraham.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.

REVISION SUMMARY
  Translate the number of the EXIF
  photo data to a human readable
  string.
  
  CCBUG: 343273

REPOSITORY
  R286 KFileMetaData

BRANCH
  format_flash

REVISION DETAIL
  https://phabricator.kde.org/D18972

AFFECTED FILES
  autotests/propertyinfotest.cpp
  src/formatstrings.cpp
  src/formatstrings_p.h
  src/propertyinfo.cpp

To: astippich, ngraham
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D17302: Add test for adding properties to result

[Alexander Stippich]

astippich added a comment.


  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.
  
  
  This can lead to issues if e.g. two ReleaseYears are added from different 
extractors. That is probably impossible right now since KFileMetaData does not 
have multiple extractors for the same mimetype, but still. 
  The result after deserialization is then a QVariantList with the values, and 
probably no client currently handles that since they expect no list.
  IMHO Baloo should not alter the output of KFileMetaData in any way (e.g. 
merging, which it currently relies on for stringlist).
  
  > On the other hand,
  >  `Result::add(prop, "value1"); Result::add(prop, {"value2", "value3"});`
  >  ends up as `{prop: ["value1", ["value2", "value3"] ]}`, which is nonsense, 
should be `{prop: ["value1", "value2", "value3"]}`
  
  I have patches ready for that and was waiting on this revision to land in 
order to continue...
  But as stated above, I also changed my mind, I do not think this is a good 
idea. This can happen if there are two extractors for the same mimetype.
  In that case, the entries may also be duplicated and merging is not a good 
idea.
  
  > After deserializing, we should have `KFM::Propertymap pm.values(prop)` -> 
`QList>({"value1", "value2", "value3"})`.
  > 
  > I am currently moving the serialization (in file/result.cpp) 
/deserialization (in file/file.cpp) into a separate function so it becomes 
testable.
  
  Anyways, the test tests how Baloo::Result _currently_ behaves. If another 
patch then modifies this behavior, the test should be changed when it lands.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D17302

To: astippich, #baloo, bruns
Cc: kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D17302: Add test for adding properties to result

[Alexander Stippich]

astippich added a comment.


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

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D17302

To: astippich, #baloo, bruns
Cc: kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D18910: Use Kformat for bit and sample rate

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:e1fa79b30da2: Use Kformat for bit and sample rate 
(authored by astippich).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18910?vs=51346=51367

REVISION DETAIL
  https://phabricator.kde.org/D18910

AFFECTED FILES
  autotests/propertyinfotest.cpp
  src/formatstrings.cpp

To: astippich, ngraham, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18911: Add units to framerate and gps data

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:a14030bf7182: Add units to framerate and gps data 
(authored by astippich).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18911?vs=51353=51359

REVISION DETAIL
  https://phabricator.kde.org/D18911

AFFECTED FILES
  autotests/propertyinfotest.cpp
  src/formatstrings.cpp
  src/formatstrings_p.h
  src/propertyinfo.cpp

To: astippich, ngraham, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18910: Use Kformat for bit and sample rate

[Alexander Stippich]

astippich added a comment.


  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

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18910

To: astippich, ngraham, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18911: Add units to framerate and gps data

[Alexander Stippich]

astippich updated this revision to Diff 51353.
astippich added a comment.


  - remove unrelated new lines

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18911?vs=51352=51353

BRANCH
  format_units

REVISION DETAIL
  https://phabricator.kde.org/D18911

AFFECTED FILES
  autotests/propertyinfotest.cpp
  src/formatstrings.cpp
  src/formatstrings_p.h
  src/propertyinfo.cpp

To: astippich, ngraham, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18911: Add units to framerate and gps data

[Alexander Stippich]

astippich created this revision.
astippich added reviewers: ngraham, bruns.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.

REVISION SUMMARY
  Add units to some of the properties
  of kfilemetadata. This will mostly
  be useful for baloo-widgets once
  they are ported.

REPOSITORY
  R286 KFileMetaData

BRANCH
  format_units

REVISION DETAIL
  https://phabricator.kde.org/D18911

AFFECTED FILES
  autotests/propertyinfotest.cpp
  src/formatstrings.cpp
  src/formatstrings_p.h
  src/propertyinfo.cpp

To: astippich, ngraham, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18910: Use Kformat for bit and sample rate

[Alexander Stippich]

astippich added a comment.


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

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18910

To: astippich, ngraham, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18910: Use Kformat for bit and sample rate

[Alexander Stippich]

astippich created this revision.
astippich added reviewers: ngraham, bruns.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.

REVISION SUMMARY
  Format bit and sample rate to 3 significant digits

REPOSITORY
  R286 KFileMetaData

BRANCH
  format_samplerate

REVISION DETAIL
  https://phabricator.kde.org/D18910

AFFECTED FILES
  autotests/propertyinfotest.cpp
  src/formatstrings.cpp

To: astippich, ngraham, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18900: Avoid side effects due to stale errno value

[Alexander Stippich]

astippich accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R286 KFileMetaData

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D18900

To: bruns, #baloo, #frameworks, ngraham, poboiko, astippich
Cc: kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D17245: Add string formatting function to property info

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:4bedfd6609c6: Add string formatting function to property 
info (authored by astippich).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17245?vs=51271=51330

REVISION DETAIL
  https://phabricator.kde.org/D17245

AFFECTED FILES
  CMakeLists.txt
  autotests/propertyinfotest.cpp
  autotests/propertyinfotest.h
  src/CMakeLists.txt
  src/formatstrings.cpp
  src/formatstrings_p.h
  src/propertyinfo.cpp
  src/propertyinfo.h

To: astippich, broulik, bruns, mgallien, #frameworks
Cc: ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, bruns, abrahams

D17245: Add string formatting function to property info

[Alexander Stippich]

astippich updated this revision to Diff 51271.
astippich added a comment.


  - rebase onto current master
  - rename header
  - update docs

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17245?vs=48690=51271

BRANCH
  display_value

REVISION DETAIL
  https://phabricator.kde.org/D17245

AFFECTED FILES
  CMakeLists.txt
  autotests/propertyinfotest.cpp
  autotests/propertyinfotest.h
  src/CMakeLists.txt
  src/formatstrings.cpp
  src/formatstrings_p.h
  src/propertyinfo.cpp
  src/propertyinfo.h

To: astippich, broulik, bruns, mgallien, #frameworks
Cc: ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, bruns, abrahams

D18819: Use content to determine mime type

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R293:7415aa60d9f6: Use content to determine mime type 
(authored by astippich).

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18819?vs=51112=51258

REVISION DETAIL
  https://phabricator.kde.org/D18819

AFFECTED FILES
  src/file/extractor/app.cpp

To: astippich, ngraham, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18826: Rewrite the taglib extractor to use the generic PropertyMap interface

[Alexander Stippich]

astippich edited the test plan for this revision.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18826

To: astippich, ngraham, bruns, mgallien
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18826: Rewrite the taglib extractor to use the generic PropertyMap interface

[Alexander Stippich]

astippich created this revision.
astippich added reviewers: ngraham, bruns, mgallien.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.

REVISION SUMMARY
  Rewrite the taglib extractor to use taglib's
  PropertyMap. Since this largely unifies the handling of the
  different tag formats, but not quite, a lot of code is removed.
  The resulting code is also faster. Additionally, this avoids the
  usage of a FileRef object, which fixes a potential crash due to
  a known bug in taglib.
  
  BUG: 403902

REPOSITORY
  R286 KFileMetaData

BRANCH
  rewrite_taglib_extractor

REVISION DETAIL
  https://phabricator.kde.org/D18826

AFFECTED FILES
  src/extractors/taglibextractor.cpp
  src/extractors/taglibextractor.h

To: astippich, ngraham, bruns, mgallien
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18819: Use content to determine mime type

[Alexander Stippich]

astippich added a comment.


  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?
  
  
  The file extension always has priority over the content according to docs 
.
  This is actually more a workaround for the bug (but still the right thing to 
do nevertheless), we now make sure that Baloo never calls the crashing 
extractor for the problematic files. The crash inside the extractor actually 
remains.
  I have the real fix for crash actually ready, but it is more a complete 
rewrite of the taglib extractor.

REPOSITORY
  R293 Baloo

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D18819

To: astippich, ngraham, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18819: Use content to determine mime type

[Alexander Stippich]

astippich created this revision.
astippich added reviewers: ngraham, bruns.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.

REVISION SUMMARY
  Determine the mime type for the
  extractors based on the content, not on the file
  extension. This avoids feeding files with a wrong
  or the same file extension into the wrong extractor.
  
  CCBUG: 403902

REPOSITORY
  R293 Baloo

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D18819

AFFECTED FILES
  src/file/extractor/app.cpp

To: astippich, ngraham, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18604: Implement support for writing rating information for taglib writer

[Alexander Stippich]

astippich added inline comments.

INLINE COMMENTS

> bruns wrote in taglibwriter.cpp:229
> but in case file.tag() returns an ApeTag, the dynamic_cast will return a 
> nullptr. This is a dynamic_cast, not a static_cast.

It always returns a nullptr

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18604

To: astippich, bruns, mgallien
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D17245: Add string formatting function to property info

[Alexander Stippich]

astippich added a comment.


  Can someone please give this a yes or a no?

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D17245

To: astippich, broulik, bruns, mgallien, #frameworks
Cc: ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, bruns, abrahams

D18604: Implement support for writing rating information for taglib writer

[Alexander Stippich]

astippich added inline comments.

INLINE COMMENTS

> bruns wrote in taglibwriter.cpp:229
> Is this the same as
> 
>   auto id3Tags = dynamic_cast(file.tag());
>   if (id3Tags) { ... }
> 
> ?

No. Mp3 supports ID3v1, ID3v2 or Ape tags, so simply casting to ID3v2 is not 
possible.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18604

To: astippich, bruns, mgallien
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18601: Rewrite taglib writer to use property interface

[Alexander Stippich]

astippich added inline comments.

INLINE COMMENTS

> bruns wrote in taglibwriter.cpp:70
> When you make this
> `void writeGenericProperties(Taglib::File *file, const PropertyMap 
> )`, you can do `file->properties(); {/* merge */}; 
> file->setProperties(...);` here, saving most of the duplicate code below.
> 
> dito for the specializations in D18604 , 
> just pass in `Taglib::File*`, and call `auto tags = 
> dynamic_cast(file->tag());` there.

That will then require to load and write the property map twice when properties 
only specific to some tagging formats need to be written, see e.g. Ape and 
Vorbis tags in D18604 . I would like to 
avoid this.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18601

To: astippich, bruns, mgallien, broulik, cfeck
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D17302: Add test for adding properties to result

[Alexander Stippich]

astippich added inline comments.

INLINE COMMENTS

> bruns wrote in resulttest.cpp:63
> This looks wrong to me ...
> How many items do you get when you append "keyword3" first and ["keyword1", 
> "keyword2"] next?

It's the same. The properties will get merged regardless of their form if the 
QVariantMap already contains an item with the same key, see 
https://phabricator.kde.org/source/baloo/browse/master/src/file/extractor/result.cpp$52
 so this works as currently intended.
Right now, we rely on this behavior because KFileMetaData (incorrectly) outputs 
multiple properties with the same key instead of a single property made of a 
QStringList. I wrote the test to prepare the switch to string lists at some 
point.
My favorite solution is to remove this merging, once KFileMetaData outputs 
string lists when necessary. This requires a custom function for the conversion 
of the output JSON to a PropertyMap, see 
https://phabricator.kde.org/source/baloo/browse/master/src/lib/file.cpp$118
QJsonDocument::toVariantMap currently does not handle duplicated keys. IIRC 
JSON with duplicated keys is strictly not compliant, but since it is only 
internal, this is okay IMHO.
The own conversion function  will save us an extra for loop, and makes sure 
that the same PropertyMap is generated when either querying via Baloo 
(file.load()) or directly via KFileMetaData's SimpleExtractionResult.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D17302

To: astippich, #baloo, bruns
Cc: kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D17302: Add test for adding properties to result

[Alexander Stippich]

astippich added a comment.


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

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D17302

To: astippich, #baloo, bruns
Cc: kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D18665: Cleanup taglib writer test

[Alexander Stippich]

astippich created this revision.
astippich added a reviewer: bruns.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.

REVISION SUMMARY
  Cleanup taglib writer test and add
  a warning when they are multiple extractors
  available, which may influence extraction
  results.

REPOSITORY
  R286 KFileMetaData

BRANCH
  simplify_writing_test

REVISION DETAIL
  https://phabricator.kde.org/D18665

AFFECTED FILES
  autotests/taglibwritertest.cpp
  autotests/taglibwritertest.h

To: astippich, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18604: Implement support for writing rating information for taglib writer

[Alexander Stippich]

astippich added a dependent revision: D18665: Cleanup taglib writer test.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18604

To: astippich, bruns, mgallien
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18665: Cleanup taglib writer test

[Alexander Stippich]

astippich added a dependency: D18604: Implement support for writing rating 
information for taglib writer.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18665

To: astippich, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18604: Implement support for writing rating information for taglib writer

[Alexander Stippich]

astippich added inline comments.

INLINE COMMENTS

> bruns wrote in taglibwritertest.cpp:409
> What are the differences between the various mp3 test cases?

It tests all possible ratings, since the commonly used numbers are somewhat 
arbitrary, and I wanted to make sure writing and extracting works

> bruns wrote in taglibwriter.cpp:153
> This should probably go into a separate file, togheter with the inverse 
> transform.
> 
> This can then be tested independently, i.e.
> 
>   for rating in {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10} {
> r = wmpRating(rating);
> Q_ASSERT(rating = balooRating(r)); }

We have to test the rest anyway, so I don't see much benefit in this honestly.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18604

To: astippich, bruns, mgallien
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18604: Implement support for writing rating information for taglib writer

[Alexander Stippich]

astippich updated this revision to Diff 50709.
astippich added a comment.


  - rebase on latest parent revision

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18604?vs=50505=50709

BRANCH
  rating_write2

REVISION DETAIL
  https://phabricator.kde.org/D18604

AFFECTED FILES
  autotests/taglibwritertest.cpp
  autotests/taglibwritertest.h
  src/writers/taglibwriter.cpp

To: astippich, bruns, mgallien
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18603: Implement more tags for taglib writer

[Alexander Stippich]

astippich updated this revision to Diff 50705.
astippich added a comment.


  - rebase on latest parent revision

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18603?vs=50501=50705

BRANCH
  enhance_taglibwriter2

REVISION DETAIL
  https://phabricator.kde.org/D18603

AFFECTED FILES
  autotests/taglibwritertest.cpp
  autotests/taglibwritertest.h
  src/writers/taglibwriter.cpp

To: astippich, bruns, mgallien, ngraham
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18603: Implement more tags for taglib writer

[Alexander Stippich]

astippich added inline comments.

INLINE COMMENTS

> bruns wrote in taglibwritertest.cpp:276
> You should probably add a warning when `extractorList.size() > 1`, as in this 
> case the extractor you run would be arbitrary.
> 
> Though, I am currently not aware of cases where one mimetype is supported by 
> several extractors.

I plan to factor this out afterwards in order to share between the different 
tests, I will incorporate it there.

REPOSITORY
  R286 KFileMetaData

BRANCH
  enhance_taglibwriter

REVISION DETAIL
  https://phabricator.kde.org/D18603

To: astippich, bruns, mgallien, ngraham
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18601: Rewrite taglib writer to use property interface

[Alexander Stippich]

astippich added a comment.


  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.
  
  
  Done. It requires a little bit more of boilerplate code, but I have not 
strong preference.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18601

To: astippich, bruns, mgallien, broulik, cfeck
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18601: Rewrite taglib writer to use property interface

[Alexander Stippich]

astippich updated this revision to Diff 50704.
astippich added a comment.


  - rewrite for better readability and to avoid heap allocation

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18601?vs=50497=50704

BRANCH
  rewrite_taglib2

REVISION DETAIL
  https://phabricator.kde.org/D18601

AFFECTED FILES
  src/writers/taglibwriter.cpp

To: astippich, bruns, mgallien, broulik, cfeck
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18450: Add extractor for AppImage files

[Alexander Stippich]

astippich accepted this revision.
astippich added a comment.
This revision is now accepted and ready to land.


  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.

REPOSITORY
  R286 KFileMetaData

BRANCH
  addappimageextractor

REVISION DETAIL
  https://phabricator.kde.org/D18450

To: kossebau, #baloo, astippich
Cc: TheAssassin, astippich, broulik, kde-frameworks-devel, ashaposhnikov, 
michaelh, spoorun, ngraham, bruns, abrahams

D18450: Add extractor for AppImage files

[Alexander Stippich]

astippich added a comment.


  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?

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18450

To: kossebau, #baloo
Cc: TheAssassin, astippich, broulik, kde-frameworks-devel, ashaposhnikov, 
michaelh, spoorun, ngraham, bruns, abrahams

D18603: Implement more tags for taglib writer

[Alexander Stippich]

astippich added a dependent revision: D18604: Implement support for writing 
rating information for taglib writer.

REPOSITORY
  R286 KFileMetaData

BRANCH
  enhance_taglibwriter

REVISION DETAIL
  https://phabricator.kde.org/D18603

To: astippich, bruns, mgallien, ngraham
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18604: Implement support for writing rating information for taglib writer

[Alexander Stippich]

astippich added a dependency: D18603: Implement more tags for taglib writer.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18604

To: astippich, bruns, mgallien
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18603: Implement more tags for taglib writer

[Alexander Stippich]

astippich added a comment.


  I forgot to add the dependent revision.
  Thanks for the review, as it's been hard lately to get a reply to anything 
related to KFileMetaData

REPOSITORY
  R286 KFileMetaData

BRANCH
  enhance_taglibwriter

REVISION DETAIL
  https://phabricator.kde.org/D18603

To: astippich, bruns, mgallien, ngraham
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18603: Implement more tags for taglib writer

[Alexander Stippich]

astippich added a dependency: D18601: Rewrite taglib writer to use property 
interface.

REPOSITORY
  R286 KFileMetaData

BRANCH
  enhance_taglibwriter

REVISION DETAIL
  https://phabricator.kde.org/D18603

To: astippich, bruns, mgallien, ngraham
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18450: Add extractor for AppImage files

[Alexander Stippich]

astippich added a comment.


  In D18450#402175 , @TheAssassin 
wrote:
  
  > `LIBAPPIMAGE_BINARIES` (or, more likely, `LIBAPPIMAGE_LIBRARIES`) doesn't 
exist, maybe it existed for a short while but got deleted. @kossebau can you 
please link to `libappimage` directly?
  
  
  Oh sorry, just noticed that I wrongly and consistently wrote 
LIBAPPIMAGE_BINARIES while I meant LIBAPPIMAGE_LIBRARIES, that may have caused 
some confusion.
  
  I will hopefully find some time tonight or tomorrow to test this again

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18450

To: kossebau, #baloo
Cc: TheAssassin, astippich, broulik, kde-frameworks-devel, ashaposhnikov, 
michaelh, spoorun, ngraham, bruns, abrahams

D18601: Rewrite taglib writer to use property interface

[Alexander Stippich]

astippich added a dependent revision: D18603: Implement more tags for taglib 
writer.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18601

To: astippich, bruns, mgallien
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18604: Implement support for writing rating information for taglib writer

[Alexander Stippich]

astippich retitled this revision from "implement support for writing rating 
information for taglib writer" to "Implement support for writing rating 
information for taglib writer".

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18604

To: astippich, bruns, mgallien
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18604: implement support for writing rating information for taglib writer

[Alexander Stippich]

astippich created this revision.
astippich added reviewers: bruns, mgallien.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.

REVISION SUMMARY
  Add support for writing the rating tag to all
  supported mime types. Since the implementations quite
  differ, implement mime type specific paths when necessary.

TEST PLAN
  tests pass

REPOSITORY
  R286 KFileMetaData

BRANCH
  rating_write

REVISION DETAIL
  https://phabricator.kde.org/D18604

AFFECTED FILES
  autotests/taglibwritertest.cpp
  autotests/taglibwritertest.h
  src/writers/taglibwriter.cpp

To: astippich, bruns, mgallien
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18603: Implement more tags for taglib writer

[Alexander Stippich]

astippich created this revision.
astippich added reviewers: bruns, mgallien.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.

REVISION SUMMARY
  Implement more tags for writing for the
  taglib writer including composer, disc number,
  album artist, lyricist, conductor, lyrics,
  copyright and language.

TEST PLAN
  tests pass

REPOSITORY
  R286 KFileMetaData

BRANCH
  enhance_taglibwriter

REVISION DETAIL
  https://phabricator.kde.org/D18603

AFFECTED FILES
  autotests/taglibwritertest.cpp
  autotests/taglibwritertest.h
  src/writers/taglibwriter.cpp

To: astippich, bruns, mgallien
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18601: Rewrite taglib writer to use property interface

[Alexander Stippich]

astippich created this revision.
astippich added reviewers: bruns, mgallien.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.

REVISION SUMMARY
  Rewrite the taglib writer to use taglib's
  unified property interface, which is more extensible.

TEST PLAN
  tests pass

REPOSITORY
  R286 KFileMetaData

BRANCH
  rewrite_taglib

REVISION DETAIL
  https://phabricator.kde.org/D18601

AFFECTED FILES
  src/writers/taglibwriter.cpp

To: astippich, bruns, mgallien
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18450: Add extractor for AppImage files

[Alexander Stippich]

astippich added a comment.


  The first issue is when BUILD_TESTING is not set to false, cmake fails with
  CMake Error at lib/CMakeLists.txt:8 (add_subdirectory):
  
The source directory

  ~/Code/libappimage-0.1.8/lib/gtest

does not contain a CMakeLists.txt file.
  
  Second issue was a missing check for libfuse, otherwise it would fail during 
compiling (squashfuse I think).
  
  The LIBAPPIMAGE_BINARIES cmake variable is used in the target_link_libraries 
call in the autotest and extractor cmake file.
  
  I will open issues on github.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18450

To: kossebau, #baloo
Cc: TheAssassin, astippich, broulik, kde-frameworks-devel, ashaposhnikov, 
michaelh, spoorun, ngraham, bruns, abrahams

D12950: add test which checks the property types

[Alexander Stippich]

astippich added a comment.


  ping!

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D12950

To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D17245: Add string formatting function to property info

[Alexander Stippich]

astippich added a comment.


  ping!

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D17245

To: astippich, broulik, bruns, mgallien, #frameworks
Cc: ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, bruns, abrahams

D18450: Add extractor for AppImage files

[Alexander Stippich]

astippich added a comment.


  I am having troubles getting it to build (Kubuntu 18.10). Unfortunately, I 
could not find pre-build packages for libappimage. I have overcome two small 
issues in building libappimage, but now I can't get it to work in KFileMetaData 
because cmake complains that LIBAPPIMAGE_BINARIES is not set. Is that me doing 
something stupid? Anyway, I cannot test

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18450

To: kossebau, #baloo
Cc: astippich, broulik, kde-frameworks-devel, ashaposhnikov, michaelh, spoorun, 
ngraham, bruns, abrahams

D18434: exiv2extractor: add support for bmp, gif, webp, tga

[Alexander Stippich]

astippich accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R286 KFileMetaData

BRANCH
  covergiftgabmp

REVISION DETAIL
  https://phabricator.kde.org/D18434

To: kossebau, #baloo, #dolphin, astippich
Cc: aacid, kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D17302: Add test for adding properties to result

[Alexander Stippich]

astippich added a comment.


  ping!

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D17302

To: astippich, #baloo, bruns
Cc: kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D18450: Add extractor for AppImage files

[Alexander Stippich]

astippich added a comment.


  Can you add a test please?
  
  In D18450#397920 , @kossebau wrote:
  
  > There seems to be some bug with the Comment field though, somehow in 
Dolphin the comment is not shown, where "dump" displays it as existing.
  
  
  That property "conflicts" with the xattr comment and is excluded in 
baloo-widgets because of that.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18450

To: kossebau, #baloo
Cc: astippich, broulik, kde-frameworks-devel, ashaposhnikov, michaelh, spoorun, 
ngraham, bruns, abrahams

D18010: Fix failing test of exiv gps data

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:c3fbdea4f7f3: Fix failing test of exiv gps data (authored 
by astippich).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18010?vs=49750=50079

REVISION DETAIL
  https://phabricator.kde.org/D18010

AFFECTED FILES
  src/extractors/exiv2extractor.cpp

To: astippich, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18205: Test empty and zero gps data

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:15d8c42d8ce0: Test empty and zero gps data (authored by 
astippich).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18205?vs=49749=50078

REVISION DETAIL
  https://phabricator.kde.org/D18205

AFFECTED FILES
  autotests/exiv2extractortest.cpp
  autotests/exiv2extractortest.h
  autotests/samplefiles/test_no_gps.jpg
  autotests/samplefiles/test_zero_gps.jpg

To: astippich, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18010: Fix failing test of exiv gps data

[Alexander Stippich]

astippich retitled this revision from "Allow zero altitude/longitude/latitude 
exiv gps data" to "Fix failing test of exiv gps data".
astippich edited the summary of this revision.

REPOSITORY
  R286 KFileMetaData

BRANCH
  fix_zero_gps

REVISION DETAIL
  https://phabricator.kde.org/D18010

To: astippich, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18010: Allow zero altitude/longitude/latitude exiv gps data

[Alexander Stippich]

astippich updated this revision to Diff 49750.
astippich added a comment.


  - fix gps altitude precision

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18010?vs=49317=49750

BRANCH
  fix_zero_gps

REVISION DETAIL
  https://phabricator.kde.org/D18010

AFFECTED FILES
  src/extractors/exiv2extractor.cpp

To: astippich, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18205: Test empty and zero gps data

[Alexander Stippich]

astippich updated this revision to Diff 49749.
astippich added a comment.


  - update

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18205?vs=49377=49749

BRANCH
  gps_tests

REVISION DETAIL
  https://phabricator.kde.org/D18205

AFFECTED FILES
  autotests/exiv2extractortest.cpp
  autotests/exiv2extractortest.h
  autotests/samplefiles/test_no_gps.jpg
  autotests/samplefiles/test_zero_gps.jpg

To: astippich, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18010: Allow zero altitude/longitude/latitude exiv gps data

[Alexander Stippich]

astippich added inline comments.

INLINE COMMENTS

> bruns wrote in exiv2extractor.cpp:298
> when you rewrite this as
> 
>   if (it != data.end()) {
>   auto ratio = it->value().toRational();
>   it = data.findKey(Exiv2::ExifKey("Exif.GPSInfo.GPSAltitudeRef"));
>   if ((ratio.second != 0) && (it != data.end())) {
>   auto altRef = it->value().toLong();
>   if (altRef) {
>   alt = -1.0 * ratio.first / ratio.second;
>   } else {
>   alt = 1.0 * ratio.first / ratio.second;
>   }
>   }
>   }
> 
> does the unit test pass without special handling of the altitude fuzz?

Yes, it does

REPOSITORY
  R286 KFileMetaData

BRANCH
  fix_zero_gps

REVISION DETAIL
  https://phabricator.kde.org/D18010

To: astippich, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D17245: Add string formatting function to property info

[Alexander Stippich]

astippich added a comment.


  @broulik since you've made the feature request, do you mind giving this a 
quick look?

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D17245

To: astippich, broulik, bruns, mgallien, #frameworks
Cc: ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, bruns, abrahams

D18205: Test empty and zero gps data

[Alexander Stippich]

astippich retitled this revision from "test empty and zero gps data" to "Test 
empty and zero gps data".

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18205

To: astippich, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18205: test empty and zero gps data

[Alexander Stippich]

astippich marked an inline comment as done.
astippich added inline comments.

INLINE COMMENTS

> bruns wrote in exiv2extractortest.cpp:65
> although only copied, I think it should be written like the other two:
> `QCOMPARE(result.properties().value(PhotoGpsAltiitude).toDouble(), 12.2);`

That's not possible, qFuzzyCompare does not handle this correctly with a too 
small epsilon. It's probably because of the float->double conversion in the 
extractor

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18205

To: astippich, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18205: test empty and zero gps data

[Alexander Stippich]

astippich updated this revision to Diff 49377.
astippich added a comment.


  - move namespace

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18205?vs=49316=49377

BRANCH
  gps_tests

REVISION DETAIL
  https://phabricator.kde.org/D18205

AFFECTED FILES
  autotests/exiv2extractortest.cpp
  autotests/exiv2extractortest.h
  autotests/samplefiles/test_no_gps.jpg
  autotests/samplefiles/test_zero_gps.jpg

To: astippich, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18010: Allow zero altitude/longitude/latitude exiv gps data

[Alexander Stippich]

astippich added a dependency: D18205: test empty and zero gps data.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18010

To: astippich, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18205: test empty and zero gps data

[Alexander Stippich]

astippich added a dependent revision: D18010: Allow zero 
altitude/longitude/latitude exiv gps data.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D18205

To: astippich, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18010: Allow zero altitude/longitude/latitude exiv gps data

[Alexander Stippich]

astippich updated this revision to Diff 49317.
astippich added a comment.


  - rebase on revision

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18010?vs=48789=49317

BRANCH
  fix_zero_gps

REVISION DETAIL
  https://phabricator.kde.org/D18010

AFFECTED FILES
  src/extractors/exiv2extractor.cpp

To: astippich, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18205: test empty and zero gps data

[Alexander Stippich]

astippich created this revision.
astippich added a reviewer: bruns.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.

REVISION SUMMARY
  Create a new test that more extensively
  checks gps data obtained via exiv. The test
  currently fails.

REPOSITORY
  R286 KFileMetaData

BRANCH
  gps_tests

REVISION DETAIL
  https://phabricator.kde.org/D18205

AFFECTED FILES
  autotests/exiv2extractortest.cpp
  autotests/exiv2extractortest.h
  autotests/samplefiles/test_no_gps.jpg
  autotests/samplefiles/test_zero_gps.jpg

To: astippich, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D17391: add support for more mimetypes to taglibwriter

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:b6fa5f7245fb: add support for more mimetypes to 
taglibwriter (authored by astippich).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17391?vs=46994=48918

REVISION DETAIL
  https://phabricator.kde.org/D17391

AFFECTED FILES
  autotests/taglibwritertest.cpp
  src/writers/taglibwriter.cpp

To: astippich, bruns, mgallien
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18010: Allow zero altitude/longitude/latitude exiv gps data

[Alexander Stippich]

astippich created this revision.
astippich added a reviewer: bruns.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.

REVISION SUMMARY
  A zero value was used before to indicate invalid
  data, and thus 0° latitude/longitude or 0 m altitude were
  not displayed while being valid data. Use NaN to indicate
  invalid data.

REPOSITORY
  R286 KFileMetaData

BRANCH
  fix_zero_gps

REVISION DETAIL
  https://phabricator.kde.org/D18010

AFFECTED FILES
  src/extractors/exiv2extractor.cpp

To: astippich, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D12950: add test which checks the property types

[Alexander Stippich]

astippich added a comment.


  ping! I would like to land this as a preparation for T8196 


REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D12950

To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D12950: add test which checks the property types

[Alexander Stippich]

astippich updated this revision to Diff 48691.
astippich added a comment.


  - rebase
  - cleanup

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12950?vs=34396=48691

BRANCH
  property_tests

REVISION DETAIL
  https://phabricator.kde.org/D12950

AFFECTED FILES
  autotests/taglibextractortest.cpp
  autotests/taglibextractortest.h

To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D17245: Add string formatting function to property info

[Alexander Stippich]

astippich updated this revision to Diff 48690.
astippich marked an inline comment as done.
astippich added a comment.


  - add _p to header define

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17245?vs=47816=48690

BRANCH
  display_value

REVISION DETAIL
  https://phabricator.kde.org/D17245

AFFECTED FILES
  CMakeLists.txt
  autotests/propertyinfotest.cpp
  autotests/propertyinfotest.h
  src/CMakeLists.txt
  src/formatstrings.cpp
  src/formatstrings.h
  src/propertyinfo.cpp
  src/propertyinfo.h

To: astippich, broulik, bruns, mgallien, #frameworks
Cc: ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, bruns, abrahams

D17391: add support for more mimetypes to taglibwriter

[Alexander Stippich]

astippich added a comment.


  friendly ping. this is pretty straightforward

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D17391

To: astippich, bruns, mgallien
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D17500: Restore mobipocket extractor

[Alexander Stippich]

astippich planned changes to this revision.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D17500

To: astippich, bruns
Cc: ltoscano, mgallien, aacid, kde-frameworks-devel, #baloo, ashaposhnikov, 
michaelh, astippich, spoorun, ngraham, bruns, abrahams

D17302: Add test for adding properties to result

[Alexander Stippich]

astippich retitled this revision from "add simple test for string merging" to 
"Add test for adding properties to result".
astippich edited the summary of this revision.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D17302

To: astippich, #baloo, bruns
Cc: kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D17302: add simple test for string merging

[Alexander Stippich]

astippich updated this revision to Diff 48688.
astippich added a comment.


  - extend the test

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17302?vs=47329=48688

BRANCH
  test_result

REVISION DETAIL
  https://phabricator.kde.org/D17302

AFFECTED FILES
  autotests/unit/file/CMakeLists.txt
  autotests/unit/file/resulttest.cpp
  src/file/extractor/result.h

To: astippich, #baloo, bruns
Cc: kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D17245: Add string formatting function to property info

[Alexander Stippich]

astippich marked 3 inline comments as done.
astippich added a comment.


  Thanks for the review!

INLINE COMMENTS

> bruns wrote in formatstrings.cpp:50
> Why is midnight an invalid time?

Same with the previous discussing around the orientation values, I simply 
copied the code. My plan was to improve it later on, but that did not plan out 
as expected :)

> bruns wrote in propertyinfo.h:93
> I think you should hand in a KFormat here if you want to avoid constructing a 
> new one for each value.

I decided against that and used a local KFormat. Baloo-Widgets would need 
bigger changes otherwise as it handles everything on a per property basis, and 
currently already constructs a KFormat per property. It's also not used 
everywhere in the display functions. Others also seem to construct it locally, 
so I guess it's not expensive.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D17245

To: astippich, broulik, bruns, mgallien, #frameworks
Cc: ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, bruns, abrahams

D17245: Add string formatting function to property info

[Alexander Stippich]

astippich updated this revision to Diff 47816.
astippich added a comment.


  - remove static kformat
  - always show time
  - change default display function handling

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17245?vs=47660=47816

BRANCH
  display_value

REVISION DETAIL
  https://phabricator.kde.org/D17245

AFFECTED FILES
  CMakeLists.txt
  autotests/propertyinfotest.cpp
  autotests/propertyinfotest.h
  src/CMakeLists.txt
  src/formatstrings.cpp
  src/formatstrings.h
  src/propertyinfo.cpp
  src/propertyinfo.h

To: astippich, broulik, bruns, mgallien, #frameworks
Cc: ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, bruns, abrahams

D17245: Add string formatting function to property info

[Alexander Stippich]

astippich added a comment.


  String freeze is approaching next week, and I would like to get this in for 
5.54 so that I can start porting Dolphin and baloo-widgets. Anyone?

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D17245

To: astippich, broulik, bruns, mgallien
Cc: ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, bruns, abrahams

D17245: Add string formatting function to property info

[Alexander Stippich]

astippich updated this revision to Diff 47660.
astippich added a comment.


  - improve code readability

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17245?vs=46859=47660

BRANCH
  display_value

REVISION DETAIL
  https://phabricator.kde.org/D17245

AFFECTED FILES
  CMakeLists.txt
  autotests/propertyinfotest.cpp
  autotests/propertyinfotest.h
  src/CMakeLists.txt
  src/formatstrings.cpp
  src/formatstrings.h
  src/propertyinfo.cpp
  src/propertyinfo.h

To: astippich, broulik, bruns, mgallien
Cc: ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, bruns, abrahams

D17548: Fixed link to the coding style wiki page

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R293:5d9f4415f7a8: Fixed link to the coding style wiki page 
(authored by janpr, committed by astippich).

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17548?vs=47473=47659

REVISION DETAIL
  https://phabricator.kde.org/D17548

AFFECTED FILES
  README.md

To: janpr, #baloo, broulik
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D17500: Restore mobipocket extractor

[Alexander Stippich]

astippich added a comment.


  In D17500#376221 , @aacid wrote:
  
  > In D17500#375753 , @astippich 
wrote:
  >
  > > Oh, thanks for the hint, didn't know that. That makes it a lot more 
complicated than a straight port :(
  > >  Looking at the code of kdegraphics-mobipocket, shouldn't the thumbnail 
extractor actually be part of kio-extras? Seems quite KIO-specific, and a quick 
look at lxr didn't reveal any usages of the thumbnailer.
  >
  >
  > Why would it be part of kio-extras? the beauty of plugins is that they can 
live wherever, no?
  
  
  My thinking here was that a lot of other thumbnailers are located in 
kio-extras. Moving the thumbnailer there too would lift the KIO dependency and 
make qmobipocket easier to deploy and to be used by others.
  
  > Do I understand that the answer to my "Is it possible to move the extractor 
to kdegraphics-mobipocket instead of having it in kfilemetadata? " question is 
no?
  > 
  > If so, that probably needs fixing, the fact that you can't have external 
plugins means that the code is probably not as good as it should
  
  No, the answer was I don't know :) But as @mgallien and @bruns stated, it's 
possible. But I lack the time and motivation to work on this.
  
  But honestly, I'm not super interested in this, I don't have any mobipocket 
files. I just saw that this code lay there disabled for years, and thought it 
would be easy to enable, which it isn't.
  Would you be fine if I disable the mobiextractor like before, and merge the 
changes anyways? This way, it does at least compile and runs if someone enables 
it in cmake.
  Otherwise I will abandon this revision.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D17500

To: astippich, bruns
Cc: mgallien, aacid, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D17302: add simple test for string merging

[Alexander Stippich]

astippich added a comment.


  I was recently wondering if this is actually desired behavior. Right now it 
is definitely required since KFileMetaData wrongly outputs QStrings instead of 
a QStringList, but when this is fixed, this behavior should be removed imho. 
Otherwise, querying metadata via KFileMetaData and via Baloo differs in output, 
which it shouldn't.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D17302

To: astippich, #baloo, bruns
Cc: kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D17500: Restore mobipocket extractor

[Alexander Stippich]

astippich added a comment.


  Oh, thanks for the hint, didn't know that. That makes it a lot more 
complicated than a straight port :(
  Looking at the code of kdegraphics-mobipocket, shouldn't the thumbnail 
extractor actually be part of kio-extras? Seems quite kio-specific, and a quick 
look at lxr didn't reveal any usages of the thumbnailer.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D17500

To: astippich, bruns
Cc: aacid, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D17500: Restore mobipocket extractor

[Alexander Stippich]

astippich updated this revision to Diff 47335.
astippich added a comment.


  - add missing test file

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17500?vs=47334=47335

BRANCH
  mobi

REVISION DETAIL
  https://phabricator.kde.org/D17500

AFFECTED FILES
  CMakeLists.txt
  autotests/mobiextractortest.cpp
  autotests/samplefiles/test.mobi
  cmake/FindQMobipocket.cmake
  src/extractors/mobiextractor.cpp

To: astippich, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D17500: Restore mobipocket extractor

[Alexander Stippich]

astippich added a comment.


  I was unable to get a version check running with cmake, any help is 
appreciated

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D17500

To: astippich, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D17500: Restore mobipocket extractor

[Alexander Stippich]

astippich edited the summary of this revision.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D17500

To: astippich, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D17500: Restore mobipocket extractor

[Alexander Stippich]

astippich created this revision.
astippich added a reviewer: bruns.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.

REVISION SUMMARY
  Restore the mobipocket extractor based
  on the QMobipocket library, which was
  apparently dormant since the KF5 transition. 
  Adjust existing code where needed and fix tests.

REPOSITORY
  R286 KFileMetaData

BRANCH
  mobi

REVISION DETAIL
  https://phabricator.kde.org/D17500

AFFECTED FILES
  CMakeLists.txt
  autotests/mobiextractortest.cpp
  cmake/FindQMobipocket.cmake
  src/extractors/mobiextractor.cpp

To: astippich, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D17245: Add string formatting function to property info

[Alexander Stippich]

astippich marked 3 inline comments as done.
astippich added a comment.


  any further comments/objections?

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D17245

To: astippich, broulik, bruns, mgallien
Cc: ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, bruns, abrahams

D17301: add documentation to result class

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
astippich marked an inline comment as done.
Closed by commit R293:60d6fc82ae5a: add documentation to result class (authored 
by astippich).

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17301?vs=46987=47330

REVISION DETAIL
  https://phabricator.kde.org/D17301

AFFECTED FILES
  src/file/extractor/result.h

To: astippich, #baloo, bruns
Cc: yurchor, kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D17302: add simple test for string merging

[Alexander Stippich]

astippich updated this revision to Diff 47329.
astippich added a comment.


  - updates for master

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17302?vs=46710=47329

BRANCH
  test_result

REVISION DETAIL
  https://phabricator.kde.org/D17302

AFFECTED FILES
  autotests/unit/file/CMakeLists.txt
  autotests/unit/file/resulttest.cpp
  src/file/extractor/result.h

To: astippich, #baloo, bruns
Cc: kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D16716: fail writing test if mime type is not supported by the extractor

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:239b0ef2b23b: fail writing test if mime type is not 
supported by the extractor (authored by astippich).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16716?vs=46990=47328

REVISION DETAIL
  https://phabricator.kde.org/D16716

AFFECTED FILES
  autotests/taglibwritertest.cpp

To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D17338: Move typesForMimeType helper from BasicIndexingJob to anonymous namespace

[Alexander Stippich]

astippich accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R293 Baloo

BRANCH
  submit

REVISION DETAIL
  https://phabricator.kde.org/D17338

To: bruns, #baloo, #frameworks, astippich, ngraham
Cc: kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D16716: fail writing test if mime type is not supported by the extractor

[Alexander Stippich]

astippich retitled this revision from "skip writing test if mime type is not 
supported by the extractor" to "fail writing test if mime type is not supported 
by the extractor".

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D16716

To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D17391: add support for more mimetypes to taglibwriter

[Alexander Stippich]

astippich created this revision.
astippich added reviewers: bruns, mgallien.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.

REVISION SUMMARY
  adds support for basic tag writing to
  aiff, wav, wavpack, wma/asf, ape and speex files

TEST PLAN
  tests pass

REPOSITORY
  R286 KFileMetaData

BRANCH
  taglibwriter_mimetypes

REVISION DETAIL
  https://phabricator.kde.org/D17391

AFFECTED FILES
  autotests/taglibwritertest.cpp
  src/writers/taglibwriter.cpp

To: astippich, bruns, mgallien
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D16716: skip writing test if mime type is not supported by the extractor

[Alexander Stippich]

astippich added a comment.


  Well, if it is not even signaled via ctest, that's pretty bad, I agree

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D16716

To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D16716: skip writing test if mime type is not supported by the extractor

[Alexander Stippich]

astippich updated this revision to Diff 46990.
astippich added a comment.


  - use QFAIL

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16716?vs=44983=46990

BRANCH
  skip_missing

REVISION DETAIL
  https://phabricator.kde.org/D16716

AFFECTED FILES
  autotests/taglibwritertest.cpp

To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D17301: add documentation to result class

[Alexander Stippich]

astippich marked 2 inline comments as done.
astippich added inline comments.

INLINE COMMENTS

> bruns wrote in result.h:79
> The TermGenerator's do not contain any data themselves, but
> 
> - keep/update the position state when adding data
> - add the data to the referenced Baloo::Document when supplied with new data 
> (`index*Text()`)

Would be nice if you could add that information to the TermGenerator :)

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D17301

To: astippich, #baloo, bruns
Cc: yurchor, kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D17301: add documentation to result class

[Alexander Stippich]

astippich updated this revision to Diff 46987.
astippich added a comment.


  - rephase description of TermGenerator

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17301?vs=46864=46987

BRANCH
  result_documentation

REVISION DETAIL
  https://phabricator.kde.org/D17301

AFFECTED FILES
  src/file/extractor/result.h

To: astippich, #baloo, bruns
Cc: yurchor, kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams