Make FileBrowseField return a FieldFile. Fix #69.

[ryneeverett]

See the docs on FileField and FieldFile:

When you access a FileField on a model, you are given an instance of FieldFile as a proxy for accessing the underlying file.

Implementation Details

• Use the same descriptor mechanism upstream FileField uses to return a specific class of object rather than the from_db_value/to_python methods. These won't work because FieldFile needs to be instantiated with a model instance, which these methods do not have access to.
• Break most of FileObject out into FileObjectMixin so it can be subclassed. We need a separate class for compatibility with FieldFile because each one has a property with no setter that the other implements as an instance attribute, which causes AttributeError's. (See http://stackoverflow.com/a/27627094/1938621).

[ryneeverett]

I'd like to add that this would have been a lot simpler were I not retaining 100% of the FileObject API, both when the FileBrowseField is accessed on a model instance and when a FileObject is sent on a post-upload signal. I'm not sure that post-save signal is worth the confusion of an extra mixin class.

[stephenmcd]

I'd like to add that this would have been a lot simpler were I not retaining 100% of the FileObject API, both when the FileBrowseField is accessed on a model instance and when a FileObject is sent on a post-upload signal. I'm not sure that post-save signal is worth the confusion of an extra mixin class.

Does anything in Mezzanine etc make use of this? If not, let's leave it out until there's a concrete use-case.

[ryneeverett]

I think I've changed my mind on the advantages of getting rid of that signal. The browse view uses a lot of the FileObject API and I'd rather not duplicate it. (Only appears to call a couple methods, but those methods call more methods.)

[stephenmcd]

Fair enough :-) I'll try and test it out soon, thanks a lot Ryne.

[stephenmcd]

Thanks Ryne - I also just merged some other commits of yours on this repo, thanks for those too.

[stephenmcd]

Sorry, I've merged this prematurely as it appears to break some things, namely thumbnails stop working (in the gallery admin inlines for images for example), and also the test case in https://github.com/stephenmcd/mezzanine/issues/1513 seems to break even further in relation to the changes here.

I've manually reverted the merge for now in e4db44ff367fd30b16101f7b1402c63cdbefed90

[ryneeverett]

@stephenmcd Sorry about the inadequate testing. I'll get back to this some time.

[stephenmcd]

No problem :-)

[ryneeverett]

@stephenmcd Got around to rebasing and testing this and I can't reproduce either issue. Inline thumbnails work and I can make the field-injection migration just fine. I tested with mezzanine master and this branch (rebase is not reflect in the PR).

[stephenmcd]

Yeah the migrations were unrelated (sorry my mistake) but I still get the thumbnails breaking, specifically on a default install with demo data, the inlines for images in the default gallery page - sorry I should have been specific about that.

I took a look further just now and it looks like filefield.path goes from being relative to absolute, which causes the thumbnail tag to break. Oddly enough in the front-end templates, all uses of thumbnail tag use filefield.url. So I tried changing to that which mostly works, except in the admin inlines where the issue is, images containing non-ascii chars in their names don't work. Even more oddly, they work fine in front-end templates.

FWIW some screenshots below showing this. I'm writing out {{ value.path }} in this template: https://github.com/stephenmcd/filebrowser-safe/blob/master/filebrowser_safe/templates/filebrowser/custom_field.html#L13

Current tip of FB dev branch:

Revert back to merge commit of the original PR:

Change from {{ thumbnail value.path }} to {{ thumbnail value.url }} in the template, all work bar the non-ascii name:

[ryneeverett]

took a look further just now and it looks like filefield.path goes from being relative to absolute

That is correct and noteworthy. I thought I was able to avoid changes to the FileObject API but I was wrong.

I think your filefield.path -> filefield.url change is correct. I guess I don't have that character set installed on this machine because the "Ávila Spain" thumbnail shows up just fine. I'll have to investigate this further.

Thanks for the detailed report.

[stephenmcd]

I think your filefield.path -> filefield.url change is correct.

Either way I guess we can change the FB template to use url for consistency, but we should still see if we can retain the old behaviour since it might break someone's project somewhere.

Thanks for all your help Ryne.

[ryneeverett]

we should still see if we can retain the old behaviour since it might break someone's project somewhere.

This would be easy to do but I don't think it's a good idea. I think full support for the FieldFile API is more important than full preservation of the FileObject API. Someone's project may depend on the FileObject api, but they can fix that directly, unlike third party apps that depend on the FieldFile API. Could we just log a warning that the path behavior has changed when it is accessed?

[ryneeverett]

Of course, this breaking change should probably coincide with a non-point-release of mezzanine. We could preserve the behavior until then if there is likely to be a release of filebrowser-safe before that.

[ryneeverett]

Just pushed a commit to my branch preserving the behavior and logging a warning. Is that acceptable?

[ryneeverett]

Ok, I was able to reproduce the non-ascii issue in python2. I pushed a fix by switching from filefield.url to filefield.name, which is the equivalent property of the relative filefield.path we were using before.

[ryneeverett]

FYI, all the buginess of those non-ascii file names in python2 (e.g., your screenshot: "Ávila Spain" vs correct: "Ávila Spain") is fixed in chardet master. Still not sure why I'm getting box-drawing characters in python 3.

[ryneeverett]

Ah, fixed it.

--- a/mezzanine/galleries/models.py
+++ b/mezzanine/galleries/models.py
@@ -74,8 +74,8 @@ class BaseGallery(models.Model):
                 if isinstance(name, bytes):
                     encoding = charsetdetect(name)['encoding']
                     tempname = name.decode(encoding)
-                else:
-                    tempname = name
+                else:  # python 3
+                    tempname = name.encode('cp437').decode('utf-8')

                 # A gallery with a slug of "/" tries to extract files
                 # to / on disk; see os.path.join docs.

cp437 seems to be the closest thing to a standard for zip files encoding.

[ryneeverett]

Sorry for having such a long conversation with myself. I've opened #77 and stephenmcd/mezzanine#1609 to resolve everything discussed.