Skip to content
This repository has been archived by the owner on Sep 29, 2023. It is now read-only.

dimensions in refKeys and refresh task #134

Merged
merged 2 commits into from
Mar 25, 2019
Merged

Conversation

sveneberth
Copy link
Member

It would be nice and sometimes necessary to get the dimensions of an image.
So I added width and height to refkeys of fileBone and update the dimensions on refresh from blobstore.

phorward
phorward previously approved these changes Feb 18, 2019
Copy link
Member

@phorward phorward left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! 👍

@phorward phorward changed the title diemensions in refKeys and refresh task dimensions in refKeys and refresh task Feb 18, 2019
Copy link
Contributor

@tsteinruecken tsteinruecken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unhappy with this. We can add width and height to the preset of refkeys, but trying to refresh the dimensions on each refresh is a waste of resources:
a) Files are static, a file can never change inside the blobstore (so it's dimensions can't neither)
b) These are set on upload. If it didn't work then i can't see a reason why it should work now.
c) Refresh is not only called when you rebuild a searchIndex but also when updating inbound relations.

If you have valid images that didn't have them set they are either veeery old or we have a bug in there (and should investigate that instead)

@sveneberth
Copy link
Member Author

Hello @tsteinruecken,
I updated the PR. Now there are only the added refKeys and I set the associated bones as readonly.

@skoegl
Copy link
Member

skoegl commented Feb 20, 2019

I don't like turning on readOnly mode on width and height.

When uploading images, we are using blobstore.fetch_data() and image.Image to get us the dimensions. But when you are using big images in terms of filesize >=1 MiB we'll hit a hard resource limit and this codepath will raise. But there are scenarios where external programs can provide this information later or as metadata in kwargs, so it would still be nice to be able to set them via fromClient()

@phorward
Copy link
Member

Good morning @skoegl, I can understand your objection, but in first place it is correct and valid that the user shall not be able to change these values. Readonly bones can still be set and overridden by custom functions within the module or by a customized, exposed function not using fromClient() to be called by external programs.

@sveneberth sveneberth added this to the v2.4 milestone Feb 27, 2019
@tsteinruecken tsteinruecken merged commit 24b5dec into develop Mar 25, 2019
@tsteinruecken tsteinruecken deleted the feature/fileDimensions branch March 25, 2019 11:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants