Fix crash in writer collection and cleanup
ClosedPublic

Authored by astippich on Aug 24 2019, 8:24 PM.

Details

Summary

WriterCollection was apparently never used.
It crashed upon destruction due to bogus
deletion of the writer plugins.
This carries over some code of the
ExtractorCollection and cleans up a little
bit along the way. Adds a simple test.

Diff Detail

Repository
R286 KFileMetaData
Branch
fixWriterCollection
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 15615
Build 15633: arc lint + arc unit
astippich created this revision.Aug 24 2019, 8:24 PM
Restricted Application added projects: Frameworks, Baloo. · View Herald TranscriptAug 24 2019, 8:24 PM
Restricted Application added subscribers: Baloo, kde-frameworks-devel. · View Herald Transcript
astippich requested review of this revision.Aug 24 2019, 8:24 PM
astippich updated this revision to Diff 64543.Aug 25 2019, 10:36 AM
  • further improvements
apol added a subscriber: apol.Sep 7 2019, 1:53 PM
apol added inline comments.
src/writer.h
63

Why's this change?

src/writercollection.cpp
133

maybe it would make sense to use shared pointers there?

bruns added inline comments.Sep 7 2019, 3:01 PM
src/writercollection.cpp
133

No. m_plugin is either a QPluginLoader::instance(), i.e. shared, or a collection-owned ExternalWriter.

astippich added inline comments.Sep 8 2019, 10:01 AM
src/writer.h
63

This is only for cleanup and consistency with the corresponding extractor class.

friendly ping. Since this solves a crash and I want to start using the writers in Elisa, I would like to have it rather sooner in a frameworks release and I'm away the next weeks

astippich updated this revision to Diff 67372.Oct 6 2019, 11:39 AM
  • improve
bruns accepted this revision.Oct 6 2019, 11:45 AM
This revision is now accepted and ready to land.Oct 6 2019, 11:45 AM
astippich updated this revision to Diff 67376.Oct 6 2019, 12:56 PM
  • actually add test file for writercollection
This revision was automatically updated to reflect the committed changes.
justinkb added a subscriber: justinkb.EditedNov 11 2019, 10:41 AM

The test incorrectly fails when a user builds kfilemetadata without taglib

collection.fetchWriters(QStringLiteral("audio/mpeg3")).isEmpty() would be true in that case

Thanks! I pushed a fix which disables the test if taglib is not installed