Provide list and form controllers for EntityViewMode and EntityFormMode

We probably need the same for form modes when that lands?

+++ b/core/modules/entity/lib/Drupal/entity/EntityViewModeInterface.phpundefined
@@ -14,4 +14,9 @@
+  /**
+   * @return string
+   */

Needs more documentation.

+++ b/core/modules/entity/lib/Drupal/entity/Form/EntityViewModeFormBase.phpundefined
@@ -0,0 +1,95 @@
+    return TRUE;
+    return (bool) $this->queryFactory

i guess the first line is wrong.

+++ b/core/modules/entity/entity.routing.ymlundefined
@@ -0,0 +1,34 @@
+    _access: 'TRUE'
...
+    _access: 'TRUE'
...
+    _access: 'TRUE'

+++ b/core/modules/entity/lib/Drupal/entity/Controller/EntityViewModeController.phpundefined
@@ -0,0 +1,66 @@
+   * @todo.

+++ b/core/modules/entity/lib/Drupal/entity/EntityViewModeAccessController.phpundefined
@@ -0,0 +1,26 @@
+  protected function checkAccess(EntityInterface $entity, $operation, $langcode, AccountInterface $account) {
+    return TRUE;

actually public access?

Yeah!

Ok, I have tested:

• Added new view mode via the entity links
• Added new view mode via link on top
• Deleted view mode via contextual links
• Deleted view mode via Delete link on edit view mode page
• Edited a view mode to change label and custom settings option

I ran into a couple things:

1) When creating the new view mode, the machine name only has the first word in the label rather than putting dashes between words... but, maybe that is another bug?

2) I got an error after adding the first view mode:

but was not able to reproduce. Sorry the image is so small... something was weird with my screengrab and couldn't reproduce error to get another one.

Do you have someone who can review the code? It looks great to me but I don't know D8 code :P

+++ b/core/modules/entity/lib/Drupal/entity/Form/EntityViewModeDeleteForm.phpundefined
@@ -50,6 +50,7 @@ public function submit(array $form, array &$form_state) {
+    entity_info_cache_clear();

+++ b/core/modules/entity/lib/Drupal/entity/Form/EntityViewModeFormBase.phpundefined
@@ -103,6 +103,7 @@ public function exists($entity_id, array $element, array $form_state) {
+    entity_info_cache_clear();

not way to inject this?

I'm seeing more errors:

I still have the machine name issue but I'll try to do a new 8.x git clone and start from scratch rather than my current one in case there is some cruft that I'm not noticing.

Quickie review:

- Having the link to the listing live in 'System' seems too generic. Add it as a link directly under Structure for now.
- The two ways of adding a new view mode in https://drupal.org/files/02listing-page.png seem a bit much. Probably best to drop the top blue one and have only the inline ones at the bottom of each table. I proposed adding the big blue one on top of each section for a reworked block page but that doesn't seem to be the direction headed there. Would be good to somehow map this to be more similar to other UI's. Maybe like the 'Add new field' in Field UI where you can immediately add a name?

Some more tidbits:

I'm thinking you most likely create a new view mode because you *want* different display settings. It's correct that the checkbox should be opt-in (default unchecked) but then we want to phrase that option as a way to *not* have custom display settings.

Since a view mode only applies to 1 entity type, we can reconsider the proposal in https://drupal.org/node/1831958 (in that issue ;-)

I tested on a super fresh D8 and still my machine names aren't showing all words... just first word. ??? (I tested in Chrome and Firefox)

@yoroy - did machine names work for you?

Also, I'm unclear as to what the custom display settings option is providing. Maybe more info is needed?

The machine names aren't working in general on my site... tried for a new content type field... weird.

So... you can ignore that as an issue here.

> Also, I'm unclear as to what the custom display settings option is providing. Maybe more info is needed?

Yes, would be good to understand if this is really necessary. If so, can we postpone that to the following screen and just let people not change a thing?

Hmmm... try to use the patch and got this:

[mac:kristen:d8dev]$ patch -p1 < display-modes-2029321-16.patch 
patching file core/modules/entity/entity.module
patching file core/modules/entity/entity.routing.yml
patching file core/modules/entity/lib/Drupal/entity/Controller/EntityDisplayModeController.php
patching file core/modules/entity/lib/Drupal/entity/EntityDisplayModeAccessController.php
can't find file to patch at input line 323
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/core/modules/entity/lib/Drupal/entity/EntityDisplayModeBase.php b/core/modules/entity/lib/Drupal/entity/EntityDisplayModeBase.php
|index 4fe2c12..088ae35 100644
|--- a/core/modules/entity/lib/Drupal/entity/EntityDisplayModeBase.php
|+++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayModeBase.php
--------------------------
File to patch: 

A lot of the form modes pages are giving 404... maybe not all forms have (should have) a form mode?

The requested page "/admin/structure/display-modes/form/add/contact_category" could not be found.

The requested page "/admin/structure/display-modes/form/add/custom_block_type" could not be found.

The requested page "/admin/structure/display-modes/form/add/form_mode" could not be found.

The requested page "/admin/structure/display-modes/form/add/view_mode" could not be found.

The requested page "/admin/structure/display-modes/form/add/field_instance" could not be found.

The requested page "/admin/structure/display-modes/form/add/filter_format" could not be found.

The requested page "/admin/structure/display-modes/form/add/image_style" could not be found.

The requested page "/admin/structure/display-modes/form/add/node_type" could not be found.

The requested page "/admin/structure/display-modes/form/add/shortcut" could not be found.

The requested page "/admin/structure/display-modes/form/add/menu" could not be found.

The requested page "/admin/structure/display-modes/form/add/taxonomy_vocabulary" could not be found.

The requested page "/admin/structure/display-modes/form/add/user_role" could not be found.

The requested page "/admin/structure/display-modes/form/add/view" could not be found.

And for the view modes, just one:

The requested page "/admin/structure/display-modes/view/add/taxonomy_vocabulary" could not be found.

And "Taxonomy vocabulary" doesn't show up in the list when going to admin/structure/display-modes/view/add.

I tested adding/deleting/editing a few form modes and view modes and it worked fine.

Personally I think there are to many screens. I would prefer a dropdown of form mode choices on the add view/form mode screen. Although such long dropdown can be frustrating to.

Anyway I fixed the form issues with previous patch. The prepare entity changes were display mode only so I have to override the class to make it work with form modes.

To explain my vision some more. I worked on the display suite UI revamp for these kinda screens last year. And we had some iterations before we came to our final screens and we got lots of positive feedback and nobody complained. (at least not directly :p)

The *before* version was more or less like the version implemented today.

Display Modes *click*
View modes *click*
Add *click*
Choose entity *click*
Fill in and save *click*
==> 5 clicks and a few of them are simply anoying. What is the use case of a page with 2 links, display modes and form modes?

The *after* version had a slightly different pattern
Display modes *click*
Add *click*
Fill in and save *click*
==> 3 clicks for the same result

The big difference between D7 and D8 is the introduction of form modes but we can easily integrate this in that UI pattern (please don't hate me because I misused the word pattern) by defaulting to display modes and put two tabs on top (one with the current "display modes" and one with "form modes").
With ds we did the same: removed pages with a limited amount of links and add it as tabs and it worked out quite well.

And the second screen I have problems with is the *long* list of entities to choose from.
As suggested above I would explore the possibilities to turn it into a dropdown on the "add" form.
When we shortcut to a specific add form the dropdown will be gone.

And the second screen I have problems with is the *long* list of entities to choose from

We just have many entities, that's bound to happen.

I'd rather not start thinking again about the interface and get this in sooner than later and go to #1831958: Allow to add view modes from Field UI's Manage Display page after that which will add more UX ?

I'm not going to block anything :).
And we need some code reviews.

I agree with @swentel that it would be good to get something in now (since the alternative might be having nothing) but I see that @aspilicious has some good UX ideas :) Is it possible to get it committed with a follow up to work on improving the UX? Not sure how that works with core issues.

I tested the patch and now all the add form mode pages are good (no 404s). I did get this when deleting using the red Delete link (not contextual link):

The requested page "/admin/structure/display-modes/view/manage/block.sadfjk/delete" could not be found.

I did see the view mode 404 still for vocabulary:

The requested page "/admin/structure/display-modes/view/add/taxonomy_vocabulary" could not be found.

Scanned the code, this looks really clean. One small question, one small nitpick, but nothing critical at all.

+++ b/core/modules/entity/entity.moduleundefined
@@ -11,6 +11,102 @@
+    'access arguments' => array('access administration pages'),

Just wondering whether this shouldn't be 'Manage custom view modes' too.

+++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayModeInterface.phpundefined
@@ -0,0 +1,25 @@
+   * Returns the entity type this view mode is used for.

display mode instead of view mode ?

Vocabulary shouldn't be in the listing. Fixed the listing. Can't reproduce the block problem, perhaps I have fixed it somehow. Anyway try with a clean install and report back :)

Thnx for all the testing!

Discussed this some more on irc. We don't want to be able to add view modes for config entities like "node_type" and "view_mode" as they are not fieldable so there is no use case in the UI for them.

To increase UX I decided to filter on entities that are fieldable. This improves the UX a lot and prevent confusion between config and content entities.

Small fix

Yeah in "theorie" it could work on any entity but it causes some troubles in the UI.

For example without the fieldable check "node" and "node_type" are both allowed.
It's very confusing: do you need to add a "content" form display or "content type" form display when you want add a new form display for the add/edit article screen. Swentel and I got confused, and seriously if people working with drupal every day get confused others will be for sure...

When you need display/form modes in other cases than core offers you're probably dealing with advanced custom code. When dealing with those cases you probably can add viem/form modes in code?

Anyway, I don't mind getting rid of the fieldable check if we want a UI that supports everything.

I'm still getting the delete error (for *form* modes only). Have super fresh install (applied patches before install).

1. Create form mode
2. Click "Edit" in contextual links
3. Click "Delete" link on lower right
4. Get 404

The link is wrong and has a "view" in it instead of a "form" in it:

(Correct link) Delete link via contextual links:

http://127.0.0.1:8080/d8dev/index.php/admin/structure/display-modes/form/manage/comment.asdfdsf/delete

(Wrong link) Delete link via edit page:

http://127.0.0.1:8080/d8dev/index.php/admin/structure/display-modes/view/manage/comment.asdfdsf/delete

And, "Taxonomy vocabulary" still shows up so link has a 404.

The code in EntityDisplayModeEditForm.php :

/**
 * Provides the edit form for entity display modes.
 */
class EntityDisplayModeEditForm extends EntityDisplayModeFormBase {

  /**
   * {@inheritdoc}
   */
  public function delete(array $form, array &$form_state) {
    $form_state['redirect'] = 'admin/structure/display-modes/view/manage/' . $this->entity->id() . '/delete';
  }

}

needs to know if it is a form mode or view mode so the link is correct. I would update the patch but I'm unclear how to best figure out which mode you are editing. I see that the information is in the current URL but I assume that is not the right way to get it.

I found one remaining 404 (when clicking on link at main page not on sub page):

The requested page "/admin/structure/display-modes/view/add/file" could not be found.

Btw, I like that there aren't so many entities shown now... it was kind of overwhelming before. Not sure what makes sense DX-wise but I think it is good like this for now.

Thumbs up! Tested:

• Get to add page for each form mode and view mode
• Add a form mode and view mode
• Delete a form mode and view mode from 2 different paths

It got a code review in #28 so I'm marking RTBC.

Thanks! :)

I looked at the interdiff. Yeah this one is ready now :)

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

I've tested this it works great.

But I think the UI is a bit confusing with the different checkboxes on the entity level (admin/structure/display-modes/view/add/node) compared the bundle level (admin/structure/types/manage/article/display and admin/structure/types/manage/page/display) and quite confusing.

I'm not sure that we actually need the checkbox on the entity level. To quote yoroy in #10

I'm thinking you most likely create a new view mode because you *want* different display settings.

Why do we need the followup than? https://drupal.org/node/2041593

Yep not setting the whether or not the display mode is customisable is an improvement...

Ok now just noticed something else... but perhaps for a followup. So the "CUSTOM DISPLAY SETTINGS" checkboxes on the node bundle manage display page (admin/structure/types/manage/article/display) is only on the Default display and not on the teaser or any custom display modes you create...

To me this opens up the question of why the "Manage display" is not manage "Manage displays" and instead of showing the fields that are on the default display it should list the displays available, allow the user to set whether they are customisable or not and then click on a link to manage that specific display.

As it stands the current UI is more than a bit confusing.

Also the UI for adding display modes (admin/structure/display-modes/view) has a column for custom settings which now can not be managed from here...

To me this opens up the question of why the "Manage display" is not manage "Manage displays" and instead of showing the fields that are on the default display it should list the displays available, allow the user to set whether they are customisable or not and then click on a link to manage that specific display.

It's possible to call entity_view() with a view mode that has no configuration at all. Default is there to have at least a sensible presentation of your content.
Whether it makes sense or not to force to have a configuration for a view mode when calling it, is completely of topic for this patch.

I can live with renaming to "Manage displays" though. And maybe even removing the 'custom settings' column from the list.

Woops, didn't want to make it RTBC again, but let's not wait to long though.

Just reviewing this now. It's likely part of my review does not touch on this patch alone.

1) I am in agreement with Alex that our current UI for manage displays is unsustainable. In previous discussions I did throw the idea of using a table of content type related parts (content, comment) + view modes. That was dismissed at the time, but hopefully its open for discussion again. Because its clear we are using a model that simply doesn't scale to this size and by making it a table where the disabled view modes are available, we would greatly simplify the whole interaction.

2) Starting at the beginning, this has no useful description.

3) The actual list UI has a number of UI elements. I dislike the "add ** view mode" at the bottom. First of all the title is unnecessarily long, and I wonder if we truly need this. Because if we don't it would make the tables a lot cleaner. Same goes for "custom settings" if this cannot be configured here, lets remove it from the table. It also makes for some strange effects in the alignment of this column.

4) Overall could this benefit from some helpful intro descriptions and a help page. Right now, its very sparse in its information. Given that each patch should take us closer to release, I'd like to see some follow ups for that. For those that don't know what view modes are, this functionality is currently quite poorly described.

Just visualizing how my suggestion works:

* Small note, ideally enable/disable is in a dropbutton. I couldn't figure out how to firebug that effectively.

Ok, talked this through with Bojhan on IRC and why I am getting annoyed (again). Basically, this is the second time we're getting close to a commit to just simply manage view modes in core which is not a day to day task in your website.

I honestly think we should do this in two steps: fix description, rename 'manage display' and remove the 'custom settings' column as Bojhan also suggests and open a follow up for the other idea of moving the screens around. I don't think it's a bad idea, but moving the UI around again just makes me just give up, and I didn't even wrote along on this version. However, I'm aware how sensitive UI is (I honestly never doubt that, don't get me wrong on that) and it's probably also easily forgotten when we start the follow up ...

• Removed the 'custom settings' column
• Moved 'Content' up in weight as this is usually the most important entity type you're managing the displays for in your site
• Added a better description

Didn't do

• Rename to manage displays: that opens a can of worms and a 80kb patch because you
  • need to rename the field ui permissions
  • need to change the menu paths
  • need to change a lot of tests
  • do the same for manage form display ..

  let's not go there ...

• hook_help - we have critical tasks for that, opened here: #2041819: Update hook_help and menu entries for Entity module to describe display modes

We have an existing issue from yoroy, see #1831958: Allow to add view modes from Field UI's Manage Display page - which we can use to discuss whether to make more adjustements like Bojhan also suggests.

The plans in #56 look like a great way forward

Great, thanks!. I moved the suggestion in #53 to #1831958: Allow to add view modes from Field UI's Manage Display page, as does look like quite a way to improve this interface which can easily become a bit unwieldy.

Good to go!

Committed 9cd2885 and pushed to 8.x. Thanks!