Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Internalize deprecated imghdr packet #646

Closed
wants to merge 5 commits into from

Conversation

jcjgraf
Copy link
Contributor

@jcjgraf jcjgraf commented Jun 24, 2023

This PR internalizes the code from imghdr into our our code base. The code newly resides in utils/imghdr. The process is fairly straightforward and also allows for simplifications in utils/files, as our own custom types can be moved to the other imghdr code.

Steps:

  • Copy imghdr code over
  • Cleanup of copied and own code
  • Copyright header
    • imghdr is licensed under "Python Software Foundation License Version 2", which is GPL-compatible, but does not allow re-licensing.
  • Should we rename imghdr.py to imageheader.py or imagetype.py or imagedetection.py or something?
  • How to deal with the imageformats plugin?
  • Testing
    • For that we would need a series of "real" image files of different type. Or what?
  • Docs

implements #579

@jcjgraf jcjgraf changed the title Internalize deprecated imghdr packet WIP: Internalize deprecated imghdr packet Jun 24, 2023
Some types detected by imghdr are not supported by native or extended
QT. Therefore, they were removed.
Some custom types, that were not supported by imghdr have been moved
to the other file type tests.
@jcjgraf jcjgraf force-pushed the core/imghdr branch 2 times, most recently from 4f38bb4 to 6357765 Compare June 24, 2023 22:12
@karlch
Copy link
Owner

karlch commented Jun 30, 2023

Thanks a bunch, this is excellent work! 😊 I love that you removed the files not supported by Qt, that imghdr would have caught as well, and restructured to simplify things significantly.

Let me see

  • Copyright: this is a tricky one where I have zero expertise ... Would it be enough to add another comment stating that this file specifically was taken from python and is licensed differently, i.e. under "Python Software License Version 2"?
  • Rename: sure, why not when we are at it, I like imageheader.py just so we keep the connection to the initial name, but don't use silly abbreviations 😄
  • Plugin: I suppose it is still useful in itself to have the ability and showcase the options. I would move add_image_format directly into the plugin though, from files.py, as we don't use it anywhere else. Possibly just appending, now that we support all the files, and don't have exotic stuff in between?
  • Testing: I mean, that would be ideal in principle, could we use Qt to just create as many image formats as possible with QPixmap.save (c.f. tests/end2end/conftest.py) and the existing fixtures in conftest.py. Fine with leaving this to an extra PR, I can try to take care of it if you like.

@jcjgraf
Copy link
Contributor Author

jcjgraf commented Jun 30, 2023

Glad that you like it. I also think this is a clean way to solve this problem.

I know some people who may be experienced with licensing, I will ask them if they can help us out 😊

The question with imageformats is, what is its purpose? I could come up with the following answers to this question:

  • It allows to add format tests for types supported by QT natively or extended (i.e. with the qtimageformat module installed), but which are not that popular and less frequently used (e.g. PBM, XBM, BMP, etc.). Having less tests in tests may make detection of actually used formats faster.
  • Make a clear split between what is by supported by QT natively and by QT extended. Everything which requires the qtimageformats module is put into this plugin.

At the moment it seems a bit arbitrary which formats required explicit activation via imageformats and which not, and which formats require the qtimageformats module and which not.

For reference, these formats are supported natively, and these are supported when having the qtimageformats plugin installed.

@karlch
Copy link
Owner

karlch commented Jun 30, 2023

Getting licensing experience on board would be great, thanks!

I agree, this is currently a bit arbitrary, personally I would favour this split:

  • Everything supported by Qt itself, both natively and extended, should ideally be in our imageheader plugin directly. This would mean moving jp2 if I see this correctly.
  • User-plugins and other customs can go into imageformats, both avif and cr2 have a user-plugin instead of something maintained by Qt if I remember correctly.

What do you think?

@jcjgraf
Copy link
Contributor Author

jcjgraf commented Jun 30, 2023

True; I forgot that there are also other QT modules which add support for new files types (beside qtimageformat).

@jcjgraf jcjgraf mentioned this pull request Jul 2, 2023
7 tasks
@jcjgraf
Copy link
Contributor Author

jcjgraf commented Jul 2, 2023

Legal stuff is such a pain. I asked different people, and at different places and no one would give me a concrete answer (well, for obvious reasons...). The thing is that we need to maintain their license, but when we apply modifications/add own stuff, things get bit entangled and less clear. But the procedure should be (according to own research and what I was told):

  • Add copyright PSF statement to file header
  • Add second LICENSE file to project
  • Describe in docstring of each function of that file whose work it is, and if modified, what was changed.

Anyways, since I rather write code, than "legally significant" docstrings, I decided to create a completely independent implementation of the file type detection, which I pushed to #650 . This should make it much easier for us to maintain the code in the future.

Therefore, I close this PR in favour of #650

@jcjgraf jcjgraf closed this Jul 2, 2023
@jcjgraf jcjgraf deleted the core/imghdr branch July 9, 2023 16:04
@jcjgraf jcjgraf restored the core/imghdr branch July 18, 2023 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants