Get mobipocket extractor up-to-date, but keep disabled
ClosedPublic

Authored by astippich on Dec 11 2018, 8:48 AM.

Details

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.
Still disabled.

Diff Detail

Repository
R286 KFileMetaData
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
astippich created this revision.Dec 11 2018, 8:48 AM
Restricted Application added projects: Frameworks, Baloo. · View Herald TranscriptDec 11 2018, 8:48 AM
Restricted Application added subscribers: Baloo, kde-frameworks-devel. · View Herald Transcript
astippich requested review of this revision.Dec 11 2018, 8:48 AM
astippich edited the summary of this revision. (Show Details)Dec 11 2018, 8:49 AM

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

astippich updated this revision to Diff 47335.Dec 11 2018, 9:02 AM
  • add missing test file
aacid added a subscriber: aacid.Dec 11 2018, 9:57 PM

I'm afraid this could be a bit of a dependency loop since kdegraphics-mobipocket has a thumbnailer that depends on kio which is a tier 3 framework while kfilemetadata is a tier2 framework.

Is it possible to move the extractor to kdegraphics-mobipocket instead of having it in kfilemetadata? or does it depend on non installed include files?

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.

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?

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

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?

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?

There is support for external extractors in KFileMetaData. As far as I know, it has not yeet been used.
That is probably the safest way to do what you suggest.

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

bruns added a comment.Dec 13 2018, 6:00 PM

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?

There is support for external extractors in KFileMetaData. As far as I know, it has not yeet been used.
That is probably the safest way to do what you suggest.

KFM supports two kinds of extractors, one QPlugin based, and one using external processes.

The first one uses the interface defined and exported in kfilemetadata/extractor.h. Although it is AFAIK currently only used by by the extractors bundled in KFM, it can be used by any other Framework or Application.

The second one uses Json over pipes to communicate with a forked process. The only available documentation is the sourcecode, i.e. externalextractor.cpp. It has more overhead (serializing/deserializing json data), but can be used where incompatible licenses would forbid linking with KFM.

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.

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.

Just a note about this: kio-extras was not meant to be a collector of all possible kios. It's becaming as such, but if this continues we may end up revisiting it and splitting it.

I would really like the module to be in the mobipocket repo, would serve as actual proof the extractors outside the main module work if we have an example of it :)

cmake/FindQMobipocket.cmake
1 ↗(On Diff #47335)

This is wrong, mobipocket installs its own cmake files so you don't need a find module.

astippich planned changes to this revision.Jan 4 2019, 7:04 PM
astippich updated this revision to Diff 55848.Apr 9 2019, 6:07 PM
  • rebase and update
  • update test
  • disable mobi extractor
astippich retitled this revision from Restore mobipocket extractor to Get mobipocket extractor up-to-date, but keep disabled.Apr 9 2019, 6:08 PM
astippich edited the summary of this revision. (Show Details)

So no plans for moving it to kdegraphics-mobipocket? It's a dependency loop.

The plan I was about to propose is to move the thumbnailer to kdegraphics-thumbnailers and remove it from the QMobipocket library, lifting the KIO dependency of QMobipocket. I think a KIO dependency for such a library is inconvenient.

aacid added a comment.Apr 9 2019, 9:37 PM

The plan I was about to propose is to move the thumbnailer to kdegraphics-thumbnailers and remove it from the QMobipocket library, lifting the KIO dependency of QMobipocket. I think a KIO dependency for such a library is inconvenient.

qmobipocket only depends on Qt::Core and Qt::Gui.

The plan I was about to propose is to move the thumbnailer to kdegraphics-thumbnailers and remove it from the QMobipocket library, lifting the KIO dependency of QMobipocket. I think a KIO dependency for such a library is inconvenient.

qmobipocket only depends on Qt::Core and Qt::Gui.

To be more precise, the thumbnailer packaged alongside qmobipocket requires kio

aacid added a comment.Apr 10 2019, 9:30 PM

The plan I was about to propose is to move the thumbnailer to kdegraphics-thumbnailers and remove it from the QMobipocket library, lifting the KIO dependency of QMobipocket. I think a KIO dependency for such a library is inconvenient.

qmobipocket only depends on Qt::Core and Qt::Gui.

To be more precise, the thumbnailer packaged alongside qmobipocket requires kio

Correct, why is that a problem?

It creates a dependency loop as you pointed out in the first comment

The proposed solution to remove the loop is moving the extractor to the kdegraphics-mobipocket repository.

And I propose a different one :)
IMHO bundling these two is sub-optimal as it creates an unnecessary dependency on kde-specific libraries and limits its deployment. To me, this is the equivalent of bundling the audio thumbnailer with taglib.

Well, if we disagree here, I'd still like to merge this as is because it gets the code into a compiling and working state at least.

Sure, this code is not compiled, if you think it's better just commit it (IMHO, not kfilemedata specialist)

@bruns okay for you to merge this one?

since this code is not compiled, I will merge it next week if noone objects

This revision was not accepted when it landed; it landed in state Needs Review.Jul 20 2019, 8:08 AM
This revision was automatically updated to reflect the committed changes.