D29381: Thumbnail text: use libmagic to detect encoding

[Harald Sitter]

sitter added a comment.


  I have zero background knowledge here, but it really feels like there must be an existing solution to this problem other than libmagic. Like how does kate figure out the encoding of a text file.

INLINE COMMENTS

> Findlibmagic.cmake:1
> +# - Try to find libssh
> +# Once done this will define

bad copypaste

> Findlibmagic.cmake:64
> +
> +
> +endif (LIBMAGIC_INCLUDE_DIR AND LIBMAGIC_LIBRARIES)

It's more idiomatic to define an imported target, see some of the newer finders in ECM.

I do also rather wonder if we couldn't just use FindPkgConfig's imported target, in theory pkgconfig also works on windows. I may well be ignorant of the finer points of windows support though.

> textcreator.cpp:38
> +#include <config-thumbnail.h>
> +#if LIBMAGIC_FOUND
> +  #include "magic.h"

TBH, I would make libmagic required for building the thumbnail plugin. I can't see much of a rationale for why we'd want to support "broken"/insufficient encoding detection when there's code that makes it better.

> textcreator.cpp:65
> +#if LIBMAGIC_FOUND
> +static QTextCodec* codecFromFile(const QString &path)
> +{

`*` on wrong side of space

> textcreator.cpp:67
> +{
> +    magic_t m = magic_open(MAGIC_MIME_ENCODING);
> +    magic_load(m, nullptr);

better name than m? (:

> textcreator.cpp:69
> +    magic_load(m, nullptr);
> +    const char *ret = magic_file(m, path.toLocal8Bit() );
> +    auto codecName = QString(ret).toUpper().toLocal8Bit();

excess whitespace towards the end. I also wonder if qfile::encodename would be better

> textcreator.cpp:70
> +    const char *ret = magic_file(m, path.toLocal8Bit() );
> +    auto codecName = QString(ret).toUpper().toLocal8Bit();
> +    magic_close(m);

I guess you could just toUpper on a QBA instead of going through a temporary QString since ret is an encoding identifier ajnd would be always an ascii string.
Also, can be const it seems.

> textcreator.cpp:78
> +    if (strcmp(ret, "unknown-8bit")) {
> +        // use latin for unkwnwn 8bit as it is quite versatile
> +        return QTextCodec::codecForName("latin-1");

unkwnwn typo

REPOSITORY
  R320 KIO Extras

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

To: meven, #frameworks, sitter, ngraham
Cc: pino, kde-frameworks-devel, kfm-devel, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, rdieter, mikesomov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20200504/4f64aa76/attachment.htm>