stachenov / quazip

Qt/C++ wrapper over minizip
Other
582 stars 232 forks source link

Remove dependency on QTextCodec for qt6 #199

Open geustache opened 4 months ago

geustache commented 4 months ago

Remove dependency on QTextCodec for qt6 with QStringConvert 3 cases Qt5 -> Use QTextCodec QT6 + Core5Compat -> Use QTextCodec (more encoding available) QT6 without Core5Compat -> Use QStringConvert (less encoding available but probably enought for new projects)

geustache commented 4 months ago

I don't know git very well yet, so I had to delete my previous pull request to put in a clean one. Sorry about that

cen1 commented 4 months ago

Should we even bother with QT6 + Core5Compat -> Use QTextCodec? I don't see a good reason to keep it.

geustache commented 4 months ago

Hello @cen1 I understand your comment but there's a good reason for it Qt6 with QTextCodec requires Core5Compat but allows you to manage more encodings.

QStringConverter only handles the following codecs (see Qt doc for more details):

QStringConverter::Utf8 QStringConverter::Utf16 QStringConverter::Utf16 QStringConverter::Utf16LE QStringConverter::Utf32 QStringConverter::Utf32BE QStringConverter::Utf32LE QStringConverter::Latin1

In practice, for new projects, I think Utf8 will be used

cen1 commented 1 month ago

After a quick review:

  1. The code does not seem to compile. As a result, tests are failing: https://github.com/cen1/quazip/actions/runs/11243129081/job/31258319130
  2. Please rebase to include latest tests
  3. Overall the code seems ok, I'll just need to do a deeper dive into the .cpp portion to understand how things work, haven't been dealing with codecs so far.
  4. Unfortunately this breaks ABI but I don't see a more elegant way to do this. So I'll probably make a release without this patch first and then it will go into the next one.
  5. As you mentioned in your last comment, the new method handles less codecs. There are already a few encoding unit tests, can you add one that specifically looks for a failure (e.g. using an encoding that was supported by QUAZIP_CAN_USE_QTEXTCODEC, then specifically checking for a failure when compat is missing )

We will also need to add Core5Compat into the CI matrix so we test both build scenarios for at least some tests but that's on me.

geustache commented 1 month ago

I commit fix about compilation issue ! As soon as I have a bit of time, I could look into refining the test scripts to take into account codecs that are not present with qt6 (without core5compat).

oetken commented 1 month ago

You should also use target_compile_definitions(${QUAZIP_LIB_TARGET_NAME} PUBLIC QUAZIP_CAN_USE_QTEXTCODEC) instead of add_compile_definitions(QUAZIP_CAN_USE_QTEXTCODEC) otherwise including quazip.h will not work as quazip_textcodec.h will still include even if Qt5 is used. Probably u must move this to quazip/CMakeLists.txt