D25517: Add an option to extract image data and add front cover property

[Alexander Stippich]

astippich abandoned this revision.
astippich added a comment.


  Okay, since there seems to be no interest at all as even a short reply is not 
given, I am done here

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #baloo, bruns, mgallien, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D25517: Add an option to extract image data and add front cover property

[Alexander Stippich]

astippich added a comment.


  Due to the recent circumstances I will probably not implement the support in 
Elisa, so I won't land this unless @bruns explicitly is okay with it (I still 
think this is an improvement over the status quo).

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #baloo, bruns, mgallien, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, GB_2, domson, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D25517: Add an option to extract image data and add front cover property

[Alexander Stippich]

astippich added a comment.


  Since I got one accept and no other response, I will merge on the weekend

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #baloo, bruns, mgallien, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, GB_2, domson, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D25517: Add an option to extract image data and add front cover property

[Alexander Stippich]

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


  - fix label comment
  - tweaks
  - rename flag to ImageData
  - adress review comments
  - udpate since tag
  - update since tag

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25517?vs=75306=77954

BRANCH
  binaryData

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

AFFECTED FILES
  src/extractionresult.h
  src/properties.h
  src/propertyinfo.cpp

To: astippich, #baloo, bruns, mgallien, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, GB_2, domson, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D25517: Add an option to extract image data and add front cover property

[Alexander Stippich]

astippich added a comment.


  ping @bruns
  this has now been open and unanswered for month!
  Same for the dependent patches

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #baloo, bruns, mgallien, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, GB_2, domson, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D25517: Add an option to extract image data and add front cover property

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


  I agree, let's get this in now and refactor it later.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #baloo, bruns, mgallien, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, GB_2, domson, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D25517: Add an option to extract image data and add front cover property

[Alexander Stippich]

astippich added a comment.


  Ping! At least a reply would be nice

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #baloo, bruns, mgallien, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, GB_2, domson, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D25517: Add an option to extract image data and add front cover property

[Alexander Stippich]

astippich added a comment.


  Where are we with this? @bruns are you accepting the proposed solution or do 
you reject it completely?
  I think it is at least way better than the current solution and maintains 
current API.
  A larger refactoring could be done for KF6

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #baloo, bruns, mgallien, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, GB_2, domson, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D25517: Add an option to extract image data and add front cover property

[Alexander Stippich]

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


  - udpate since tag

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25517?vs=73097=75306

BRANCH
  binaryData

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

AFFECTED FILES
  src/extractionresult.h
  src/properties.h
  src/propertyinfo.cpp

To: astippich, #baloo, bruns, mgallien, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, GB_2, domson, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D25517: Add an option to extract image data and add front cover property

[Alexander Stippich]

astippich added a comment.


  friendly ping for the series

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #baloo, bruns, mgallien, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, fbampaloukas, GB_2, 
domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams

D25517: Add an option to extract image data and add front cover property

[Alexander Stippich]

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


  In D25517#588029 , @bruns wrote:
  
  > The problem is the extractor serves two different use cases:
  >
  > - retrieval for searching/caching properties
  > - on-demand extraction
  >
  >   Baloo falls into the first category. It is only interested in properties 
which can be queried/compared in a meaningful way. Even storing the properties 
as a cache for e.g. Dolphin is a little bit of a stretch.
  >
  >   Elisa falls into the second category. It does not store the extracted 
data persistently, but is fine extracting e.g. the front cover on demand.
  >
  >   Currently the only large/blob property is the front cover. But what about 
BackCover, Leaflet pages, ..., Video Thumbnails? Returning all of these even 
when not required is wasteful. If you want a front cover, say so.
  
  
  Yeah, in that case it would be a little bit wasteful. But again, I think if 
we implement this fine-grained control, it should be done for all properties.
  And this is out-of-scope for this patch.
  
  > For KF6, ExtractEverything should just be removed. It is not especially 
useful, not even for unit tests.
  >  Request the data you want and are able to handle, and do so explicitly.
  
  I have adjusted this as requested.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #baloo, bruns, mgallien, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, fbampaloukas, GB_2, 
domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams

D25517: Add an option to extract image data and add front cover property

[Alexander Stippich]

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


  - adress review comments

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25517?vs=71623=73097

BRANCH
  binaryData

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

AFFECTED FILES
  src/extractionresult.h
  src/properties.h
  src/propertyinfo.cpp

To: astippich, #baloo, bruns, mgallien, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, fbampaloukas, GB_2, 
domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams

D25517: Add an option to extract image data and add front cover property

[Stefan Brüns]

bruns added a comment.


  The problem is the extractor serves two different use cases:
  
  - retrieval for searching/caching properties
  - on-demand extraction
  
  Baloo falls into the first category. It is only interested in properties 
which can be queried/compared in a
  meaningful way. Even storing the properties as a cache for e.g. Dolphin is a 
little bit of a stretch.
  
  Elisa falls into the second category. It does not store the extracted data 
persistently, but is fine
  extracting e.g. the front cover on demand.
  
  Currently the only large/blob property is the front cover. But what about 
BackCover, Leaflet pages,
  ..., Video Thumbnails? Returning all of these even when not required is 
wasteful. If you want a
  front cover, say so.
  
  For KF6, ExtractEverything should just be removed. It is not especially 
useful, not even for unit tests.
  Request the data you want and are able to handle, and do so explicitly.

INLINE COMMENTS

> extractionresult.h:62
> +ExtractImageData = 4,
> +ExtractEverythingIncludingImageData = (ExtractEverything | 
> ExtractImageData),
>  };

Does not belong in the API. It is not really clear what it does.

If you want a convenience flag in your application, please define it locally.

> propertyinfo.cpp:585
> +d->valueType = QVariant::ByteArray;
> +d->shouldBeIndexed = false;
> +break;

shouldBeIndexed is irrelevant for ByteArray, omit it.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #baloo, bruns, mgallien, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, fbampaloukas, GB_2, 
domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams

D25517: Add an option to extract image data and add front cover property

[Nathaniel Graham]

ngraham added a comment.


  In D25517#587957 , @astippich 
wrote:
  
  > I also fear that it may not be clear to distinguish between "simple" and 
"complex" properties and that users of the API would expect that every property 
works for the setExtraProperties, which would require an immense amount of work 
for all extractors.
  
  
  I agree.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #baloo, bruns, mgallien, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, fbampaloukas, GB_2, 
domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams

D25517: Add an option to extract image data and add front cover property

[Alexander Stippich]

astippich added a comment.


  In D25517#587776 , @bruns wrote:
  
  > I have thought about this and have come to the conclusion this is not 
really future proof.
  >
  > The `Property::FrontCover` is IMHO fine, i.e. the changes to 
`propertyinfo.cpp` and `properties.h` could go in as is.
  >
  > The request of additional properties should be done differently. How about 
the following:
  >
  > 1. divide properties into (currently) two sets, "Simple Metadata" (all the 
current ones but frontcover) and "Complex Metadata" (currently frontcover).
  > 2. allow to specify extra properties for SimpleExtractionResult
  
  
  Not strictly against your proposal, but how much do we gain? What is your 
concern about future proof?
  From my Elisa application perspective, it would be very convenient to use the 
proposed ExtractionResult::ExtractEverythingIncludingImageData (or renamed 
ExtractionResult::ExtractEverything for KF6) and the 
ExtractionResult::ExtractImageData flags.
  For the current proposal the API does not need to be changed.
  
  >   SimpleExtractionResult result(fileName, mimeType);
  >   result.setExtraProperties({Property::FrontCover});
  >   plugin.extract();
  >
  > 
  > You can then also do:
  > 
  >   SimpleExtractionResult result(fileName, mimeType, 
ExtractionResult::ExtractNothing);
  >   result.setExtraProperties({Property::FrontCover, Property::BandLogo});
  >   plugin.extract();
  >
  
  This is also possible with the current proposal (when the extractors honor 
the ExtractionResult::ExtractNothing/ExtractMetaData flags properly, which I 
think currently none does) by just setting the 
ExtractionResult::ExtractImageData.
  It is just that one does not have fine-grained control over each individual 
"complex" property, but IMHO we do not need that.
  I also fear that it may not be clear to distinguish between "simple" and 
"complex" properties and that users of the API would expect that every property 
works for the setExtraProperties, which would require an immense amount of work 
for all extractors.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #baloo, bruns, mgallien, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, fbampaloukas, GB_2, 
domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams

D25517: Add an option to extract image data and add front cover property

[Stefan Brüns]

bruns added a comment.


  I have thought about this and have come to the conclusion this is not really 
future proof.
  
  The `Property::FrontCover` is IMHO fine, i.e. the changes to 
`propertyinfo.cpp` and `properties.h` could go in as is.
  
  The request of additional properties should be done differently. How about 
the following:
  
  1. divide properties into (currently) two sets, "Simple Metadata" (all the 
current ones but frontcover) and "Complex Metadata" (currently frontcover).
  2. allow to specify extra properties for SimpleExtractionResult
  
SimpleExtractionResult result(fileName, mimeType);
result.setExtraProperties({Property::FrontCover});
plugin.extract();
  
  You can then also do:
  
SimpleExtractionResult result(fileName, mimeType, 
ExtractionResult::ExtractNothing);
result.setExtraProperties({Property::FrontCover, Property::BandLogo});
plugin.extract();

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #baloo, bruns, mgallien, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, fbampaloukas, GB_2, 
domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams

D25517: Add an option to extract image data and add front cover property

[Nathaniel Graham]

ngraham added a comment.


  LGTM

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #baloo, bruns, mgallien, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, fbampaloukas, GB_2, 
domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams

D25517: Add an option to extract image data and add front cover property

[Alexander Stippich]

astippich added a comment.


  friendly ping! I would like to continue work on this. Is the new name better? 
If so, I will rebase follow-up patches

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #baloo, bruns, mgallien, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, fbampaloukas, GB_2, 
domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams

D25517: Add an option to extract image data and add front cover property

[Alexander Stippich]

astippich retitled this revision from "Add an option to extract binary data and 
add front cover property" to "Add an option to extract image data and add front 
cover property".

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #baloo, bruns, mgallien, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, fbampaloukas, GB_2, 
domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams