search

plone / Products.CMFPlone

The core of the Plone content management system
https://plone.org
GNU General Public License v2.0
268 stars 194 forks source link

PLIP: Cleanup and enhance icon and thumb aspects #1734

Closed fgrcon closed 7 years ago

fgrcon commented 8 years ago

Proposer : fgrcon

Seconder : Jens W. Klein

Abstract

Pre plone 5 icons to decorate content types were implemented using tags. Since plone 5 these image tags have been replaced by fontello fonts in css rules (:before{content=\e80...} ). Furthermore mimetype icons where used for file types such as .pdf, .xls and many more until plone 4.x. - in plone 5 all those are replaced by a single fontello font (paper clip).

There has been some confusion about icons and preview images (aka thumbs). In summary views thumbs were shown for objects containing a lead image. In plone 5.02 the option to show thumbs in further list and table views, folder content listing , portlets etc. was added.

Configurability of this feature (where to show thumbs, where to suppress them, individual thumb size ) is insufficient (only never, authenticated users only, always).

The implementation of this feature reuses an obsolete meta data catalog item: getIcon (now bool: True when item is an image cor contains an image (e.g. lead image) - this adds to the confusion between thumbs and icons enormously.

Concise but sufficient documentation (user and developer) is to be provided.

Motivation

Cleanup, better usabilty, make life easier for users, integrators, developers. get same behavior, look and feel allover the framework

Assumptions

see abstract

Proposal & Implementation

- [ ] change meta-data name getIcon to hasImage (keep getIcon with deprecation warning for a while ?) This should be moved to an independent PLIP beacause it needs further discussion. Renaming this metadata item might generate unacceptable effort for reindexing or cause problems for add ons. Maybe just to be solved with explanations in documentation.

for all listed portlets: nav-portlet

before e.g.: tableview-before

after: tableview-after

finished:

- renaming getIcon might cause troubles for addons postponed see above..

fgrcon

most of the neccessary modifications are already implemented and tested in private environment. Pull requests will follow shortly.

Related issues

branchname: plip1734thumbs https://github.com/plone/buildout.coredev/tree/plip1734thumbs

fgrcon commented 8 years ago

http://jenkins.plone.org/job/pull-request-5.1/

fgrcon commented 8 years ago

http://jenkins.plone.org/job/pull-request-5.1/596/

jensens commented 8 years ago

The title of this PLIP is still WIP - shall the Framework Team vote on it? It needs a seconder.

fgrcon commented 8 years ago

@jensens @plone/framework-team : this is more or less finished. upgrade step (reapply profile,.. ) might be needed. anyone to second help ... most welcome !

to test / Try out : https://github.com/plone/buildout.coredev/tree/plip1734thumbs

http://jenkins.plone.org/job/pull-request-5.1/695 :

https://github.com/plone/Products.CMFPlone/pull/1739 https://github.com/plone/plone.app.contenttypes/pull/368 https://github.com/plone/plone.portlet.collection/pull/15 https://github.com/plone/mockup/pull/703 https://github.com/plone/plone.app.content/pull/106 https://github.com/plone/plone.app.portlets/pull/85 https://github.com/plone/plone.app.event/pull/242 https://github.com/plone/plone.app.event/pull/242 https://github.com/plone/plone.app.contentlisting/pull/19 https://github.com/plone/plonetheme.barceloneta/pull/116

jensens commented 8 years ago

thanks for this!

@hvelarde @davilima6 or anybody else would second this plip?

May we rethink the mimetype icons? I think we can do better than using an outdated set of pngs.

I found http://www.flaticon.com/packs/file-formats-icons and I am sure there are other, maybe better, icons out in the internet. Ideas?

hvelarde commented 8 years ago

I recently stumble upon this again while trying to make collective.cover compatible with Plone 5; this is becoming more and more complex and hard to understand and maintain.

I'm not backing this until we have a clear migration path and documentation.

fgrcon commented 8 years ago

@hvelarde (cc @jensens, @plone/documentation-team ): I agree, that you might have stumbled about something - but the question is what!

configurability of this feature has to be done with CSS and not by a bunch of registry records; what was the point of using font icons if not?

I only extended the options available in site control panel (as seems to be standard with plone...). Of course this settings are used to generate/modify css classes ....). Would you like to have your end users (e.g. site admins) to edit css ? - what a fantastic user experience!

lead image story is broken as mentioned here: https://community.plone.org/t/what-is-the-canonical-way-to-get-lead-image/2821?u=hvelarde

My proposed modifications don't even touch lead image implementation. As far as I can see, it also is not broken. As far as I understand the mentioned issue, your are unhappy with requirements/design of the lead image feature . btw: I think, that a little bit (more?) of requirements engineering (including traceable documentation) could help plone development tremendously

I'm not backing this until we have a clear migration path and documentation.

migration path (?): I think one upgrade step (reapply profile for site settings ) should do , one more ( upgrade profiles for existing folders and collections in p.a.contenttypes to enable IThumbIconHandling ) might be useful (not implemented - need help).

Documentation is work in progress, see detailed information above as a preliminary sketch - but anyway, what is documentation for if nobody reads it?

For finishing/integrating documentation (much work to be done after modifications accepted I will need some advice/support from documentation team. (Btw: I located several spots in current documentation´concerning this topics which need corrections, clarifications, updates ... etc..)

hvelarde commented 8 years ago

@fgrcon I understand, but the point is we need to evaluate the icons story in Plone 5.1 before going further with this kind of new features, as they takes a lot of time and work, something you may already noticed.

IMO, the implementation of icons in Plone 5 (using fonts instead of plain images) is limited, more complicated for everyone, and has no real advantage over the previous one using images.

when I say we need a clear migration path and documentation, I meant for the icons story; so, I'm not against you feature, but in favor of reviewing some things before to make Plone 5.1 easier to maintain and configure.

fgrcon commented 8 years ago

@hvelarde , cc @plone/framework-team : I am fully aware that the decision to use fonts instead of icon-images could be problematic (e.g.: how to generate an icon for an custom dexterity type ...) - but this was not my decision!. When I stumbled into all this more than one year ago i found quite a mess ! (icon fonts and icon images, often 404 ..., lot of confusion and dead code ...).

The bunch of prs is more or less the second step of cleanup, and tries to answer a number of issues reported. (I really spent a huge amount of time digging around sometimes undocumented or deprecated code. )

If it should be decided, to replace fontello by images again, it would not be much effort to adapt this.

Once again i opt for a better (transparent, traceable ,..., impact analysis) requirements engineering/design process!

Anyway I finish for now (open to serious discussion, tests...) rebased everything: http://jenkins.plone.org/job/pull-request-5.1/895/

All pull requests:

https://github.com/plone/plone.app.upgrade/pull/97 https://github.com/plone/plone.app.contenttypes/pull/368 https://github.com/plone/plone.portlet.collection/pull/15 https://github.com/plone/plone.app.content/pull/106 https://github.com/plone/plone.app.portlets/pull/85 https://github.com/plone/plone.app.event/pull/242 https://github.com/plone/plone.app.contentlisting/pull/19 https://github.com/plone/plone.app.layout/pull/97 https://github.com/plone/plonetheme.barceloneta/pull/116 https://github.com/plone/mockup/pull/703 https://github.com/plone/Products.CMFPlone/pull/1739

If there is no interest in this, I will delete related branches and close all the prs (and just use the stuff on my own).

hvelarde commented 8 years ago

don't misunderstand me: if your work will make the maintenance easier, I'm totally fine with that. please don't trow it away.

as I have no time to review your changes in depth, and I don't know the implications, I can't second this, that's all.

jensens commented 8 years ago

In fact we plan to replace icons with svg's which are then inlined using a transform chain step. /cc @agitator

This is a massive plip. is it possible to divide it into 2-3 better consumable ones?

fgrcon commented 8 years ago

@jensens or anyone who has time to help ....:

Can anybody help me to understand the jenkins results above ? I could not spot any relevance for the mentioned prs or do I need to debug all the tests ?.

@jensens: the whole thing is not that massive or complex at all. Of course it would be possible to make some inconsistent patchwork out of it.

In fact we plan to replace icons with svg's which are then inlined using a transform chain step

sounds complicated - is there already any documentation / time frame ... for that? As long there is nothing working existing yet, I think the proposed changes would be better than nothing.

jensens commented 8 years ago

@fgrcon if you're fine just call me now, @agitator and I are working together at the moment and we would like to talk about this.

jensens commented 8 years ago

I second this proposal.

After a review and testing this in a site I really think this improves our handling of images in listings and portlets. It gives the control that was missing.

It also reintroduces file-types icons which many people are missing. The quality of this icons - located in MTR - are not the best, but @agitator and I are planning to replace them and other icons in another PR by inline-svgs, as alrady discussed with a broader audience and @RobZoneNet UX-Team at Plone conference in Boston (expect here a PLIP soon, but decoupled from this one)

@fgrcon I just found one glitch in the portlet: if there are file-type icons shown, the indent is wrong. Also you're aware of the missing file type icons in folder_contents. I guess @thet can give you a hint where to hook in here.

jensens commented 8 years ago

@RobZoneNet would you please have a look at this PLIP from a UX point of view?

jensens commented 8 years ago

to test this PLIP simply checkout the plip1734thumbs branch in buildout.coredev. This is not standard procedure, but works fine.

fgrcon commented 8 years ago

@jensens: Thanks, great news !!!! The glitches you mentioned, had been fixed already. But after rebasing last time I can not find structure/pattern.js anymore when I run in development mode(chrome web tools...). When I run plone-compile resources modifications in mockup/patterns/structure are not reflected in the results. Dont know whats going wrong here ( @thet: any ideas ? )

fgrcon commented 8 years ago

update: Resource registry > development mode develop js (plone-logged-in) works: contents new

But: build bundle .... :

Bundle Builder
You are about to build the plone-logged-in pattern.
This may take some a bit of time so we will try to keep you updated on the progress.

Press the "Build it" button to proceed.

building javascripts
Saved javascript bundle, Build results:
FUNCTION
----------------
./++resource++mockup/inlinevalidation/pattern.js
./++resource++manage-portlets.js
.....
./++resource++plone-logged-in.js

building CSS bundle
1 css files to build
less compilation error on file http://192.168.1.15:8080/Plone1/++plone++static/plone-logged-in.less: Error: 'http://192.168.1.15:8080/Plone1/++resource++mockupless/patterns/tooltip/pattern.tooltip.less' wasn't found (404)
.......
less compilation error on file http://192.168.1.15:8080/Plone1/++plone++static/plone-logged-in.less: Error: 'http://192.168.1.15:8080/Plone1/++resource++mockupless/patterns/tooltip/pattern.tooltip.less' wasn't found (404)

I was not able to recompile resources in order to make it work in production mode. (plone-recompile ...'

confirm: identation of mimetype icons in (only navigation?-) portlets is wrong --> will try to fix it.

fgrcon commented 8 years ago

@jensens: tired of updating .... http://jenkins.plone.org/job/pull-request-5.1/980/ some small css stuff still to fix (mostly unnecessary/ugly bullets in barceloneta ...)

btw.: huge troubles with compile resources, modifications only work in development mode (.Js only, css is totally broken ...) see also https://github.com/plone/Products.CMFPlone/issues/1843

ebrehault commented 8 years ago

Hi @fgrcon, your PLIP has been approved by the @plone/framework-team. If you have any question or if you need help during the implementation, do not hesitate to contact the team.

fgrcon commented 8 years ago

@plone/framework-team : thanks for approving. From my point of view implementation is mostly done (some smaller fixes might follow...). I will take care for documentation subsequently. BUT: I need help with the testresults (as clear as mud to me !) and the issue mentioned in my last comment above Btw: I will be more or less offline from 11/30 untill 12/15 (only email is possible).

fgrcon commented 7 years ago

one more try ...: http://jenkins.plone.org/job/pull-request-5.1/1135

Params: https://github.com/plone/plone.app.upgrade/pull/97 https://github.com/plone/plone.app.contenttypes/pull/368 https://github.com/plone/plone.portlet.collection/pull/15 https://github.com/plone/plone.app.content/pull/106 https://github.com/plone/plone.app.portlets/pull/85 https://github.com/plone/plone.app.event/pull/242 https://github.com/plone/plone.app.contentlisting/pull/19 https://github.com/plone/plone.app.layout/pull/97 https://github.com/plone/plonetheme.barceloneta/pull/116 https://github.com/plone/mockup/pull/703 https://github.com/plone/Products.CMFPlone/pull/1739

ebrehault commented 7 years ago

@fgrcon I ran your tests: http://jenkins.plone.org/job/pull-request-5.1/1311/

Regarding the CMFDiffTool failures, that's quite simple: in plone.app.contenttypes, you have added 5 new attributes to contents (via your IThumbIconHandling schema), so when we make a diff, we have 19 answers instead of 14. I have created a new branch in CMFDiffTool where the tests are fixed accordingly: https://github.com/plone/Products.CMFDiffTool/tree/plip1734thumbs

ebrehault commented 7 years ago

I have also updated Products.CMFPlone with master. Let's see if the tests are bette now: http://jenkins.plone.org/job/pull-request-5.1/1313/

ebrehault commented 7 years ago

@fgrcon As I was trying to understand the tests issues with plone.app.caching, I have noted few things:

fgrcon commented 7 years ago

@ebrehault: thanks a lot. Its fine for me not to enable the behavior by default. fixed in p.a.contenttypes If i remember correctly the margin issue is only with navigation portlet ...

Right now I am very busy with diving :-). Unfortunately I will be back by end of next week and go on with work.

ebrehault commented 7 years ago

Diving!!! Lucky guy :)

fgrcon commented 7 years ago

@ebrehault @jensens ..:
I have fixed everything now (even got rid of the ugly and superfluous bullets in the portlets). Everything works fine for me but I suppose some of the tests (which are so fantastically documented ) will still not work.

PLEASE HELP !!

see http://jenkins.plone.org/job/pull-request-5.1/1380/ (fixed typo see below)

fgrcon commented 7 years ago

@ebrehault : after not enabling behavior IThumbIconHandling by default we get the oposite errors first we had 19 != 14 (http://jenkins.plone.org/job/pull-request-5.1/1311/) including pull request https://github.com/plone/Products.CMFDiffTool/pull/24 we get 14 != 19 (http://jenkins.plone.org/job/pull-request-5.1/1380/)

If we ommit https://github.com/plone/Products.CMFDiffTool/pull/24 it looks fine again with CMFDiffTool see http://jenkins.plone.org/job/pull-request-5.1/1385/ This again makes me doubt about quality and reasonableness of many of the tests !

Remaining test failures: plone.app.testing.layers.rst plone.app.caching.tests.test_integration.TestOperations.test_disabled plone.app.content.tests.test_widgets.BrowserTest.testUntranslatableMetadata plone.app.portlets.tests.test_configuration.TestGenericSetup.testExport plone.app.portlets.tests.test_navigation_portlet.TestRenderer.testBottomLevelZeroNoLimitRendering plone.app.portlets.tests.test_review_portlet.TestPortlet.testInvokeAddview

ebrehault commented 7 years ago

after not enabling behavior IThumbIconHandling by default we get the oposite errors

yes it makes sense, now the behavior is not enabled, we go back to the initial situation, so just forget about my CMFDiffTool branch and use master

fgrcon commented 7 years ago

http://jenkins.plone.org/job/pull-request-5.1/1387/ (some tests fixed) @jensens, @ebrehault : I do not understand these 3 remaining test results - urgently need support. I want to get this PLIP merged and closed possibly today !!!**** 

File "/home/jenkins/.buildout/eggs/plone.app.testing-5.0.6-py2.7.egg/plone/app/testing/layers.rst", line 183, in layers.rst
Failed example:
    getSiteManager()
Expected:
    <BaseGlobalComponents test-stack-2>
Got:
    <BaseGlobalComponents test-stack-3>
fgrcon commented 7 years ago

@jensens: urgent please see https://github.com/plone/Products.CMFPlone/issues/1734#issuecomment-284687256 (updated)

jensens commented 7 years ago

hmm, possible is, this are test isolation problems. Otherwise, it does not make much sense. @ebrehault what do you think?

In Plone core we run tests in different isolation groups. Usually, its is not needed for PR's and PLIPs. But here it touches that many packages, it could be possible.

jensens commented 7 years ago

@fgrcon as far as I can see we do not have a PLIP job runner on Jenkins for https://github.com/plone/buildout.coredev/blob/5.1/plips/plip-1734-icons-and-thumbs.cfg - this may help.

@plone/testing-team @gforcada may you please create a Jenkins job for this PLIP configuration? TIA!

jensens commented 7 years ago

@fgrcon btw, the last commit at plip-1734-icons-and-thumbs.cfg is from @ebrehault and 20 days old. Please check if everything in there is set and up to date!

fgrcon commented 7 years ago

as far as can I see this should be ok. Its quite the same like in my approach (branching buildout.coredev ...). I only updated existing prs - no new ones.

jensens commented 7 years ago

You do not need to branch, that's we have the plips folder for, with its jobs. Just run it with bin/buildout -c plips/plip-1734-icons-and-thumbs.cfg.

jensens commented 7 years ago

Btw., do tests are passing on your local machine if you run bin/alltests --all (or with robot tests bin/alltests --all)? (! takes a while !)

fgrcon commented 7 years ago

I know it takes some time, just started new local test again... . last local test looked quite ok but I'm not sure anymore

fgrcon commented 7 years ago

tried to create jenkins job: https://github.com/plone/jenkins.plone.org/pull/199 @jensens: bin/buildout -c plips/plip-1734-icons-and-thumbs.cfg and my branch approach of course make no difference . last bin/test --all running localy.

Last rebases/merges done.

After that I will give up!!

fgrcon commented 7 years ago

Thanks @gforcada for http://jenkins.plone.org/view/PLIPs/job/plip-1734-icons-and-thumbs/ @jensens : local tests give same results how to proceed ? remaining failures seem to be caused by badly designed tests? I definitely will not rebase eleven pull requests over the next weeks and months

fgrcon commented 7 years ago

finished merge with PLIP: Retina image scales #1483.

Local tests (bin/alltests -all):

#### Finished tests for z3c.formwidget.query ####
Packages with test failures:
Failing tests in group plone_app_testing
Total time elapsed: 8501.199 seconds
Grand total: 90 packages, 1 failures
----------
Failure in test Scenario Thememapper LESS builder (test_thememapper.robot)
Failure in test testUntranslatableMetadata (plone.app.content.tests.test_widgets.BrowserTest)
Failure in test /plone/bld51/plips/eggs/plone.app.testing-5.0.6-py2.7.egg/plone/app/testing/layers.rst
Scenario: Thememapper LESS builder                                    | FAIL |
Test Thememapper                                                      | FAIL |
Scenario: Location query :: This sometimes fails with: Element loc... | FAIL |

Robot Test Log.pdf

Jenkins http://jenkins.plone.org/view/PLIPs/job/plip-1734-icons-and-thumbs/3/

fgrcon commented 7 years ago

Testresults:

@jensens, @ebrehault : How to proceed ? Please help!!!!

jensens commented 7 years ago

Wow, these test failures are scary. I observed the first two also a couple of times and the third and fourth should not happen too. They are for sure not related to this PR. Number 5+6 could indicate a test isolation problem. Or something different!

Ping @plone/testing-team @gforcada @tisto @tomgross may one of you take a look on this? I am a bit lost with it.

tisto commented 7 years ago

@jensens without looking into it I would suspect that those test failures are a result of running the tests in a different order. Did you add a new package to the alltests group? Did you change the order? Did you introduce a new test fixture that might affect the test execution order (zope.testrunner tries to be smart and changes the test execution order to make the tests run faster).

I've been there multiple times. I'm pretty sure this has nothing to with the pr itself. Though, unfortunately, we still need to fix it. :(

tisto commented 7 years ago

@fgrcon any chance you recall adding a "transaction.commit()" to an integration test fixture test somewhere? Just (another) wild guess...

tisto commented 7 years ago

@fgrcon have you tried running the packages that have test failures individually? e.g. "bin/test -s plone.app.content". Do the failures still occur? If not, we have a test isolation problem...

fgrcon commented 7 years ago

@tisto .....: I never added something like transaction.commit() (I 'm just too silly for something like that. :-) ) and yes I run bin/test -s something quite often:

e.g.: and just to draw attention to this again (see above):

Products.PortalTransforms.tests.test_transforms. SafeHtmlTransformsWithScriptTest.test_entities_outside_script unrelated to this plip - reproduced with vanilla 5.1 locally /does not occur with last release Products.PortalTransforms 2.2.2 http://jenkins.plone.org/job/plone-5.1-python-2.7 does not contain tests for Products.PortalTransforms??

fgrcon commented 7 years ago

@tisto, @jensens
updated see https://github.com/plone/Products.CMFPlone/issues/1734#issuecomment-289431784 new job started: http://jenkins.plone.org/view/PLIPs/job/plip-1734-icons-and-thumbs/4/

jensens commented 7 years ago

Yeah, only one is left. And it looks like for some reason a component registry was pushed to the stack, but did not get popped on teardown. If have no idea why, and no idea where.

Failed doctest test for layers.rst
  File "/home/jenkins/.buildout/eggs/plone.app.testing-5.0.6-py2.7.egg/plone/app/testing/layers.rst", line 0

----------------------------------------------------------------------
File "/home/jenkins/.buildout/eggs/plone.app.testing-5.0.6-py2.7.egg/plone/app/testing/layers.rst", line 183, in layers.rst
Failed example:
    getSiteManager()
Expected:
    <BaseGlobalComponents test-stack-2>
Got:
    <BaseGlobalComponents test-stack-3>
----------------------------------------------------------------------
File "/home/jenkins/.buildout/eggs/plone.app.testing-5.0.6-py2.7.egg/plone/app/testing/layers.rst", line 263, in layers.rst
Failed example:
    getSiteManager()
Expected:
    <BaseGlobalComponents test-stack-2>
Got:
    <BaseGlobalComponents test-stack-3>