Cleanup of PLIP #1734

[thet]

Followup of plone/Products.CMFPlone#1734

[fgrcon]

@thet, cc: @plone/framework-team :
quite funny: i changed your code from iconsize to thumbsize in mockup months ago because we were talking about preview images ( aka thumbs) and not any icons ...

but when you decide on our own to rename things (which might be ok or not ) .. ok ..
but clean up means something different than renaming! (btw. this PlIP was quite well documented and lasted for 9 months ! -and you easily could have contributed ).
So often you dont even repond to questions....

If I were you, I would spend a little bit more energy in cleaning up the mess in resource registry,/plone-compile..., etc. (because this horror cost me and other people days to get done!) and help with documentation.

short summary: I'm quite fed up!

[thet]

@fgrcon sorry for not responding earlier. I actually had to take a look into this PLIP even though I didn't volunteer to review it. I got an error with a missing thumbSize value while upgrading a project.

While thumb is better than icon (thanks for changing that) I noticed that thumbSize is just wrong in terms of the semantics of Plone's image handling. A size is a dimension like 200:400 while a scale is a name for that size. You were using scale names but named the variable "size". That's nitpicking, but it's in core so it should be named consistently to avoid confusion. Also all ov_ prefixes are not necessary. Why would one assume an attribute be called ov_thumbSize_summary while on ISiteSettings it's called thumb_size_summary.
I would have given that as feedback if I would have commited to review your PLIP but I didn't because of time issues.

After all, this pull request isn't merged yet.

Regarding the resource registry: I'm sorry that you have problems with it. For me, it works surprisingly well although I hope we can proceed using better tools like webpack which hopefully makes everything easier.
But don't blame me on the situation of the resource registry. I can't do more than improving it when it doesn't work for me, helping out on community.plone.org and discussing the successor of the JavaScript build chain.

[thet]

I realize that I invented the iconSize myself. Did make the semantic error in first place then. Yes, that's funny.

[fgrcon]

@thet

I hope you're ok with this after-PLIP cleanup. Sorry for not stepping in earlier and reviewing.

see conversation in #703 ????
If it makes you happy right now please go on - but then I would ask you to also rename the getIcon metadata item to has_image or the like. I did not dare to do so fearing a shitstorm because of necessary reindexing.

I got an error with a missing thumbSize value while upgrading a project.

something wrong with upgrade steps?

Also all ov_ prefixes are not necessary. Why would one assume an attribute be called ov_thumbSize_summary while on ISiteSettings it's called thumb_size_summary. ...

attributes without ov_ prefix hold site wide default values whereas prefixed attributes are individual overrides of this default values - this was intended just to make things clearer

[thet]

Regarding the error I got - yes, I probably forgot to run the upgrade step. My fault. But the implementation of the getters are more robust now. And the variable naming is more consistent. You can easily see where the settings are coming from if you look at the context. People like me always have to look up code if the naming is not consistent.

The getIcon metadata column cannot easily be changed without breaking other's code. It's named that way since very long.

I'm sorry that I made you upset, though I do not fully understand why. I did not ask you to change anything but did it myself. Because I know I'm too late to the discussion.

Still, NOW is the time to do any changes - none of your changes were yet released. Except for plone.app.contenttypes, but since that branch only targets Plone 5.1, I think changing it is OK too.

[thet]

I just talked directly to @fgrcon and we sorted this dispute out.
All is fine now!

And sorry again for not stepping in earlier.

Plus: thanks for taking care of the image/thumb/icon story in Plone!

[fgrcon]

yeah indeed but i forgot one absolute important message : covfefe :)

[fgrcon]

@thet cc @jensens I run through my manual test scenarios : everything looks fine, only some minimal edits (size -> scale) found : plone/plone.app.portlets#100 and plone/plone.app.contenttypes#411

But I found some new issue: Disabling icons and/or thumbs globally does not work any more. This was done using css classes ( .thumbs-off .image- {display:none;} in plone.less.

But fixing this would lead to clashes with other css-classes like image-left,etc (e.g .for use with tinymce...).

I will open necessary prs to rename these classes to thumb-<scale> e.g. `thumb-icon, thumb-thumb `` etc. This should make things more clear. Please comment and help to get this merged asap.