jonasundderwolf / django-image-cropping

Django helper application to easily and non-destructively crop arbitrarily large images in admin and frontend.
Other
553 stars 130 forks source link

Extending the FileBrowser backend to add support for ImageField #101

Closed fgmacedo closed 7 years ago

fgmacedo commented 7 years ago

The current file-browser backend only has support for the FileBrowserField. This PR enables the use of FileBrowser as backend with regular ImageFeld fields.

PS: The file-browser was published on pypi with the changes to add django-image-cropping support.

anrie commented 7 years ago

Hey @fgmacedo ,

I plan to release a new version that incorporates your FileBrowser backend in the next days.

I quickly tried to test the backend as I haven't used django-filebrowser before but I ran into some problems. Where am I supposed to crop the image? In the change view of an image? If I try to edit an image (/admin/example/image/5/change/) I get the following error Reverse for 'grp_related_lookup' with arguments '()' and keyword arguments '{}' not found. 0 pattern(s) tried: [] Maybe I just missed something when setting up the app but it's probably a good idea to make the docs a bit more explicit.

On the other hand we currently repeat most of the documentation form django-filebrowser:

https://github.com/jonasundderwolf/django-image-cropping#django-filebrowser vs. https://django-filebrowser.readthedocs.io/en/latest/quickstart.html#installation.

It's probably safe to assume that only people who are already familiar with django-filebrowser will use the backend and therefore I would skip the detailed explanation and just refer to the original docs. What do you think?

Finally I really would like to have some basic integration tests that show that both backends work as advertised and switching backends work. Especially as we are currently not using the FileBrowser backend ourselves it would be good to have some checks so we don't break stuff unintentionally while the app progresses.

fgmacedo commented 7 years ago

Hi @anrie ! Thanks for your reply.

Good to know about the release. Please let me know if you need something.

You probably forgot to add grappelli urls into your urls.py:

url(r'^grappelli/', include('grappelli.urls')),

I totally agree with you about the setup instructions, and about the integration tests.

I'm able to submit another PR with filebrowser related tests. But while I was reading your reply, I realize that the major efforts were put to isolate backend specific logic under a facade.

And since the backend config is a python path for a class, and that all the code that is suposed to run only for django-filebrowser is under backends/fb.py, the backends/fb.pyis shipping with django-image-cropping only for convenience, not by need.

What do you think of instead a PR to add tests, a PR to wipe out django-filebrowser related stuff from django-image-cropping? And we put the custom backend inside another repository, with tests and a basic example project? I can keep this repository up and running, since I'm already using this integration. And you are free from having to maintain code that you are not planning to run.

We can open a section on django-image-cropping's readme to list third-party backends, so maybe in the future I'm not the only one to use django-image-cropping with django-filebrowser.

Please let me know.

Best regards,

anrie commented 7 years ago

Hey @fgmacedo,

I actually had the same idea. I think moving the backend to its own repository is a really good idea as it makes maintaining things a lot simpler. We don't ship code that we don't use and you can iterate much quicker if you plan to extend the backend.

Maybe we could use something like extra_require (http://setuptools.readthedocs.io/en/latest/setuptools.html#declaring-extras-optional-features-with-their-own-dependencies) to allow users to additionally install the desired backends?

I guess we only need a convention/schema to declare compatible version.

Thanks a lot for your efforts!

fgmacedo commented 7 years ago

Nice!

I think that extra_require fits perfectly.

Something like:

pip install django-image-cropping[filebrowser]

I'll work on this in the next days.

sonarun commented 7 years ago

Is it possible to implement this using custom actions?

Like anrie, after implementing the FileBrowser backend, I have no idea where to actually crop the image. In Filebrowser, you can create an "action" on an item and I think that would be the best place to pop a modal or something to actually crop the image.

anrie commented 7 years ago

@fgmacedo I now removed the filebrowser backend from the main package and adjusted the docs accordingly. Let me know when you have repo/package for the additional backends so we can refer to them in the README. Have a nice sunday!