-
Notifications
You must be signed in to change notification settings - Fork 55
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
Render error when change image size #686
Comments
I've seen this also lately under Plone 4.3.11 also; seems that we where using we moved the related code from where it was before to a new location on 1.3b1, the previous one can be seen here: I think we need to refactor that, because it looks to me overly complex; can you help digging a litle bit on it, @rafaeltcc? |
Hi Hector,
I am not sure I have the skills to do so, and I lack knowledgment of this
specific code. But I will try to debug it tonight and see if I can
contribute with my 2 cents.
Thanks,
Rafael
2016-11-30 9:50 GMT-02:00 Héctor Velarde <notifications@github.com>:
… I've seen this also lately under Plone 4.3.11 also; seems that we where
using plone.scale = 1.3.5 before and now we're using plone.scale = 1.4.1.
we moved the related code from where it was before to a new location on
1.3b1, the previous one can be seen here:
https://github.com/collective/collective.cover/blame/1.2b1/
src/collective/cover/tiles/base.py
I think we need to refactor that, because it looks to me overly complex;
can you help digging a litle bit on it, @rafaeltcc
<https://github.com/rafaeltcc>?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#686 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADyyMwJh5GU3oKtgFPc3Brxhm9ymyBmnks5rDWMcgaJpZM4K_rz8>
.
|
apparently, this commit introduced the error plone/plone.scale@8b89545 |
@rodfersou where do we have to fix it? |
@hvelarde still trying to reproduce the problem.. and understand what happen |
@datakurre are you dealing with image scaling on tiles in Mosaic? how do you do that? I would love to remove this code from collective.cover at some point. |
@hvelarde Probably pretty much the same way. There's image tile in plone.app.standardtiles and image scaling view for tile in plone.app.tiles. Major difference is that scales are stored inside the same annotation with the tile configuration:
Yet, in Mosaic 2.0 the persistent image tile is deprecated (not enabled by default) in favor of just uploading images as separate content and linking them (and instead of maintaining the complexity of image tiles, the effort should go for better image upload and optional "media repository" - which are still lacking). |
The problem happen at the code below: value['modified'] < modified_time - KEEP_SCALE_MILLIS And the values at my debug session are ipdb> modified_time
'1480597779.122006'
ipdb> KEEP_SCALE_MILLIS
86400000 The question is.. why modified_time comes as string here.. still looking.. The strange part is that value['modified'] is a string too: ipdb> value['modified']
'1480597779.122006' |
A cast to float in this line should fix the problem. |
please ask someone in the Plone framework team |
@plone/framework-team can someone help giving directions on what is the right way to fix this issue? Locally my pull request fixed the issue with one cast to float. |
@rodfersou A cast to float sounds fine. Question is, why its a string before. |
@hvelarde should I remove the repr and keep the cast conversion to deal with old data? Or an upgrade step to review all data is better option? |
@rodfersou please research a little bit more; see: 7f4f4e3 what's the right value type for that attribute? an upgrade step seems a better fix for me. |
@jpgimenez do you remember this code? |
@hvelarde as your suggestion I investigated and the default Image content type returns DateTime object when we call modified method. Then my steps here would be:
|
@rodfersou what about the Dexterity-based version of the Image content type? |
the same: ipdb> from plone import api
ipdb> portal = api.portal.get()
ipdb> images = portal.listFolderContents({'portal_type': 'Image'})
ipdb> images
[<ATImage at /Plone/0Dv0rhwh_1920x1200.jpg>, <Image at /Plone/0f4znrk5_2560x1600.jpg>]
ipdb> images[0].modified()
DateTime('2016/12/01 11:09:28.406506 GMT-2')
ipdb> images[1].modified()
DateTime('2016/12/02 09:26:17.364917 GMT-2') |
After implement the proposed changes, the bug still happen when change the image size and save layout. Still looking what happened. |
@rodfersou please write a test to demonstrate the issue first. FYI, works all the way around: >>> from plone.scale.storage import KEEP_SCALE_MILLIS
>>> from DateTime import DateTime
>>> import time
>>> DateTime(), type(DateTime())
(DateTime('2016/12/02 10:16:41.941549 GMT-2'), <class 'DateTime.DateTime.DateTime'>)
>>> time.time(), type(time.time())
(1480680980.164985, <type 'float'>)
>>> DateTime() < DateTime() - KEEP_SCALE_MILLIS
False
>>> DateTime() > DateTime() - KEEP_SCALE_MILLIS
True
>>> DateTime() < time.time() - KEEP_SCALE_MILLIS
False
>>> DateTime() > time.time() - KEEP_SCALE_MILLIS
True
>>> time.time() < DateTime() - KEEP_SCALE_MILLIS
False
>>> time.time() > DateTime() - KEEP_SCALE_MILLIS
True
>>> time.time() < time.time() - KEEP_SCALE_MILLIS
False
>>> time.time() > time.time() - KEEP_SCALE_MILLIS
True we better use |
The problem was that my upgrade step was wrong, I need to change the annotation directly to convert the strings into float. |
@hvelarde the pull request is ready |
@rafaeltcc please help us testing the solution provided. FYI @idgserpro |
I will tonight as soon as I arrive home. I will also push the limits and
try to test the product in all its functionalities to see if something else
appears.
Just to confirm, to get this latetes version I need to build the latest
github product and install it as a development package?
Thanks,
Rafael
2016-12-06 0:50 GMT-02:00 Héctor Velarde <notifications@github.com>:
… @rafaeltcc <https://github.com/rafaeltcc> please help us testing the
solution provided.
FYI @idgserpro <https://github.com/idgserpro>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#686 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADyyM7iHNUHTeSerBmaEnrRC_LEBcRTbks5rFM2CgaJpZM4K_rz8>
.
|
@rafaeltcc yes, and you will need to run an upgrade step in the control panel to fix current tiles. |
Hi, Image resizing in Basic Title is now ok. The changes did it! Something that was already occurring before and is still failing is that I need to drag and drop an item (I use mostly newsitems) twice so it get added to a Basic Tile. The first time I drag and drop it just doesn't work. This was malfunctioning before I merged the proposed branch. Should I file a new bug? I noticed some warnings in my log. 2016-12-06 21:12:37 INFO Zope Ready to handle requests |
Something else I just noticed is that the Carrousell Tile just ignores the image size set. The size of the image is adjusted to the space the tile has in the layout. Also that I need, as in adding content to a Basic Tile to drag an drop an item several time before it eventualy gets loaded. |
@rafaeltcc to fix the timezone warning you can do something like this: please open new issues for the other stuff so we can take care in a sane way. |
Hi,
I am using a production 4.3.9 Plone with latest collective.cover. If, in my Layout editor, I choose any image size different of default 200:200 for a Basic Tile I get:
There was an error while rendering this tile
I get the following in my log
The text was updated successfully, but these errors were encountered: