I find it odd that settings dealing with "managing" fields are controlling how fields are presented in the UI. That seems like the job of the display fields tab.

CommentFileSizeAuthor
#162 screen_win.png10.07 KBandypost
#160 796658-winLN-d7.patch582 bytesandypost
#153 field_ui_manage_display-553298-152.patch5.1 KByched
#151 head2head-field_ui_redesign.patch3.28 KByched
#147 field_ui_manage_display-553298-147.patch116.59 KByched
#145 field_ui_manage_display-553298-145.patch116.38 KByched
#142 drupal.field-ui-manage-display.142.patch116.38 KBsun
#141 field_ui_manage_display-553298-141.patch116.54 KByched
#139 field_ui_manage_display-553298-138.patch108.15 KBchx
#130 field_ui_manage_display-553298-130.patch116.5 KByched
#128 field_ui_manage_display-553298-128.patch113.95 KByched
#123 field_ui_manage_display-553298-123.patch112.52 KByched
#121 field_ui_manage_display-553298-121.patch99.83 KByched
#119 field_ui_manage_display-553298-119.patch112.21 KByched
#118 defaults.png75.44 KBandypost
#116 field_ui_manage_display-553298-116.patch109.31 KByched
#113 field_ui_manage_display-553298-113.patch106.13 KByched
#110 field_ui_manage_display-553298-110.patch103.33 KByched
#107 field_ui_manage_display-553298-107.patch103.31 KByched
#105 field_ui_manage_display-553298-105.patch131.45 KByched
#101 field_ui_manage_display_fieldsets-553298-100.patch74.16 KByched
#98 field_ui_manage_display_fieldsets-553298-98.patch61.77 KByched
#88 field_ui_manage_display_fieldsets-553298-88.patch36.54 KByched
#87 field_ui_manage_display_fieldsets-553298-87.patch36.54 KByched
#85 field_ui_manage_display_fieldsets-553298-85.patch36.42 KByched
#83 field_ui_manage_display_fieldsets-553298-83.patch36.43 KByched
#83 field_ui_manage_display-BEFORE.png38.17 KByched
#83 field_ui_manage_display-AFTER.png43.29 KByched
#82 field_ui_manage_display_fieldsets-553298-82.patch26.41 KBte-brian
#81 field_ui_manage_display_fieldsets-553298-81.patch26.41 KByched
#79 field_ui_manage_display_fieldsets-553298-79.patch25.2 KByched
#79 field_ui_manage_display_fieldsets_dnd.png66.54 KByched
#68 field_ui_manage_display_fieldsets.patch.txt16.22 KByched
#68 field_ui_manage_display_fieldsets.png44.93 KByched
#66 field_ui_manage_display_fieldsets.patch.txt16.46 KBte-brian
#66 manage-display-fieldsets.png12.87 KBte-brian
#60 field_ui_display_default.png40.46 KByched
#60 field_ui_display_overriden.png59.05 KByched
#59 field_ui_vertical_tabs.patch.txt15.8 KByched
#52 manage-display-vt-default.png22.32 KBte-brian
#52 manage-display-vt-override.png28.66 KBte-brian
#47 display fields.png78.49 KBwebchick
#33 manage-display-mockup.png18.22 KBte-brian
#32 manage-display-mockup.png18.91 KBte-brian
#31 manage-fields-mockup.png10.13 KBte-brian
#31 manage-display-mockup.png19.74 KBte-brian
#6 field_ui_vertical_tabs.patch.txt13.72 KByched
#3 field_ui_vertical_tabs.patch13.72 KByched
#4 field_ui_vertical_tabs_BEFORE.png10.64 KByched
#4 field_ui_vertical_tabs_AFTER.png9.62 KByched
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Title: Move Field UI drag-and-drop to display fields tab? » Overhaul 'Display Fields' screen

The reason for this is that until very recently, there was a single notion of 'order' for edit forms and displayed objects.
Field API now supports different order for edit forms and for each display build mode, the UI doesn't reflect it yet and uses saves the same weights for 'form' and 'view'.

IMO, this is part of a larger overhaul of 'Display Fields' screen to account for (new, D7) Field API features currently missing in the UI:

- per build-mode drag-n-drop
I think this implies forgetting the grouping of build modes under secondary tabs (currently, 'full' and 'teaser' are grouped under 'Basic', 'search_index' and 'search_result' under 'Search'). I think this makes a perfect case for vertical tabs (one tab per build mode)
This opens up the possibility to let the main 'manage fields' screen be an additional "vertical tab" for 'form' on top of the 'display build modes' tabs, thus merging 'Manage Fields' and 'Display Fields' into one screen. Not 100% sure yet whether this last step is a good idea or not, would probably need some testing...

- formatter settings
The 'formatter settings' form depends on the actual formatter being selected, so we need separate subforms for 'select formatter' and 'adjust settongs' - just like what is currently done on the 'Manage Fields' screen for widget type and widget settings.

yched’s picture

Component: field system » field_ui.module
yched’s picture

FileSize
13.72 KB

Attached patch does a 1st step: introduce vertical tabs on the 'Manage Display' screen:
- Move 'Build modes' out of the 'Basic', 'Search', 'RSS', ... secondary tabs where no one sees them
- One vertical tab per build mode, lets you get the full list of available build modes in one place, and modify them without page reload.
- Gets rid of 'Teaser / Full node' settings next to each other in the same table, which makes the table visually hard to parse
- Opens the door for 'per build mode d-n-d reorder' (which the Field API supports, but the current UI makes impossible)

Patch is a proof of concept right now, form submission has not been updated yet.
[edit: obviously, testing the patch requires rebuilding the theme registry]

yched’s picture

Title: Overhaul 'Display Fields' screen » Vertical tabs on the 'Manage Display' screen
Status: Active » Needs review
Issue tags: +Needs usability review
FileSize
9.62 KB
10.64 KB

Screenshots

Status: Needs review » Needs work

The last submitted patch failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
13.72 KB

Let's keep the bot out of this for now.

mrfelton’s picture

I like it. Much more comprehensible, can't find any problems with the UI. Good work!

webchick’s picture

Issue tags: +Usability

Eh. I'm not so sure about this. This definitely needs someone from the UX team to sign off on it. To me, it feels incredibly heavy and is inconsistent with where and how we use vertical tabs elsewhere. It also feels pretty late in the cycle to be making these kinds of dramatic changes to the UI.

yched’s picture

webchick: "This definitely needs someone from the UX team to sign off on it"
That's exactly the reason I didn't embark into a full blown working patch ;-).

This being said, I do find this *way* better than what we currently have, which, let's face it, sucks big deal (er, I need to notice the secondary tabs and clisk through each of them to know what it the list of build modes that I need to configure ?)

I also fail to see how this is inconsistent with other use cases for vertical tabs. Actually, I do think this specific UI is the perfect match for vertical tabs - Collection of similar settings that you want to jump quickly from one to the other and compare one with another.

Bojhan’s picture

I do not see this being the right approach for this screen, although I agree that something needs to be done about this screen enforcing the vertical tabs pattern is not a good idea. Vertical tabs is not meant to be used as navigation (which is implied here) but merely for grouping sets of configuration. As stated before we should really avoid using vertical tabs as the main interaction on a screen, it is not fitted for that.

Normally I am oke with changing things, because it sucks. But I am not feeling much for breaking the vertical tabs pattern for this. Especially at this late in the Drupal cycle, we need to be extra cautious not to break mental models on patterns we have worked hard to get.

Dave Reid’s picture

Issue tags: +vertical tabs

Adding tag...

yched’s picture

Vertical tabs is not meant to be used as navigation (which is implied here) but merely for grouping sets of configuration

I don't get how this patch bends VT into 'navigation'. Grouping sets of similar configuration is *exactly* what it we're doing here. If VTs do not apply here, can you tell me where they apply ? Again, for me here's a perfect use case.

Also, almost everyone agrees that something has to be done for this screen, but no one has come up with anything so far after 4 months or so of this in core. If there's a better idea than VTs, great, but if not, I fail to see how the patch is not an improvement over the current state of the 'Display Fields' UI.

Bojhan’s picture

I can understand that you think that, the vertical tab pattern is very confusing - in the way we intend to apply it. However there is a clear pointer as, you moved secondary navigation into vertical tabs - you are doing something different then previous patches of using Vertical Tabs. In that you are using vertical tabs as the primary navigation between large sets of configuration, where the idea was to use Vertical tabs solely as something that groups functionality that can be hidden/skipped. So given that, you are now using the vertical tabs pattern - to navigate (not to group and hide) and you are using it for the main interactions -- both cases is what Vertical tabs is not designed for. We want the mental model to say (you can skip this if you want).

More importantly, you are using vertical tabs as the main interaction - we designed vertical tabs specifically for the node/add page. Although we acknowledge that its applicable in other places. We don't want to apply it to any interaction, we can somewhat meld vertical tabs into working. This all to keep the mental model in place.

Although I agree its 4 months, no one put the effort to design something good for this. It's not the way we should approach design, we should be very sure that the proposal we put in is better - I do not see this being better. It's late in the development cycle, lets not go for quick-wins that can seriously hurt the way people use new patterns.

This page deserves its own design, that didn't happen - doesn't mean we should just enforce something. I am sorry.

yched’s picture

However there is a clear pointer as, you moved secondary navigation into vertical tabs - you are doing something different then previous patches of using Vertical Tabs

Flawed logic. This patch is using VT as a more natural way of doing something that, short of better solutions in D6, happened to be spanned over several pages.
The fact that the previous UI involved navigation doesn't mean that the patch uses VT for navigation.

Since VT was introduced, every 'use VT for X' patch had to face an initial wall of 'no, VTs don't really apply here' reactions. Sorry, this is just absurd.

I do not see this being better

No ? Beats me. Did you try it ? Add a new field, and then configure its display on all build modes ?

I'm sorry to see that all proposals to improve Field UI (VTs, popups, accordions) have been diluted by purists 'yeah, dunno, this is not ideal' from the UX team without any constructive counter proposals. So the current Field UI still sucks big time, and has blatant lacks over the API (no 'per build mode' order, no formatter settings).

yoroy’s picture

Well, there's about 1 ux designer for every 100 core developers out there. We're sorry too. But because we haven't come up with an acual design for this does not mean we should bolt on these kind of semi-fixes. Even if VTs would improve the actual interaction here (not saying it would), we will still have diluted the value of VT pattern as a whole because we use it in different contexts. That's exactly the kind of WTFs we *don't* want to happen anymore.

Accusing people of being purist seems unnecessary, who else but purists are working on core anyway?

Bojhan’s picture

@yched I am somewhat astound by your reluctance to understand my logic. Look at it from a very basic standpoint - is this an interaction you want on this page? Or is it just any better interaction you want on this page?

Look, we can say - yes to every vertical tabs pattern, thats easier for us to, not having to convince people who are totally set on getting it in. But at the end of the day, we are here with the user experience in mind - and we think that saying no here, will benefit users their mental models of VT's.

webchick’s picture

I think the main reason this doesn't work is because there is no sensible summary we can display on the tabs of what lies within them. Vs. the node edit form, where we can say something currently doesn't have comments enabled, or is authored by 'admin', or what have you.

So instead, we end up using the VTs purely as navigational elements, simply because they are more prominent visually than secondary tabs. I think I would rather explore avenues that make the secondary tabs more visible than trying to shoe-horn VTs in here.

yched’s picture

is this an interaction you want on this page? Or is it just any better interaction you want on this page?

Maybe I wasn't clear. I don't see this as a "half baked workaround for lack of a better solution". My point is that VT *is* the interaction that makes sense for this page. The current UI taken straight from D6, *was* a "half baked workaround for lack of a better solution" back then.

Secondary tabs are the wrong structure here, and just making them more visible won't fix the UX challenge:

- There can be 10+ build modes. Not only because some contrib modules will define their own, but also because the trend where admins create their own custom build modes to use in a side block, in views, whatever, will expand in D7. I'm not fond of everything in it, but http://drupal.org/project/ds definitely has this part right.
- One secondary tab per mode is therefore not an option, won't scale. + quick "at a glance" comparison of settings across build modes is a frequent task when defining your fields, at least between Teaser / full node, but this true for all build modes, really.
- So the current UI packs build modes by 'groups', which are exposed as secondary tabs.
Drawback: you need to click on each group to discover the full list of build modes that are available to you.
Drawback: within each group, how do we present the display settings for the various build modes ? On top of each other ? tedious scrolling and makes comparisons difficult. There can be 10 or 20 fields in the bundle.
- So the current UI presents modes of a group vertically in a table.
Drawback: we cannot reasonably stick more than 2 modes next to each other (and the overlay only makes that even more true)
Drawback: only makes comparison possible with modes in the same group.
Drawback: the resulting form is an overloaded soup of select inputs that is not easy to parse visually.
Drawback: this forbids per build mode d-n-d reorder, which the API supports. Tell me who is not pissed that the form order is the same that the display order ?

Within the 2 years of CCK work, I've been turning this inside out. I can assure you that if D6 core had VTs, CCK contrib would have used them as the natural UI pattern for this.

The 'late for D7' argument I naturally understand (even if I don't actually agree). I can't say this for the arguments in #13.

But yes, 'I do not see this (patch) as better' beats me as not really conceivable nor backed by arguments. Actual UX feedback from actually using the patch ?

And yes, opposed to the above, 'VTs are not meant to be the main interaction pattern in a page' sounds to me as a purist argument.
How does it make the UI in the patch for this page less valid or usable ? How does it break a mental representation of what VTs are, as opposed to the 'block config page' ? Does it mean that if there was a checkbox above the VT group, it would become valid all of a sudden ?

I should also make it clear that not a second I'd reproach anyone for not having the time to work on Field UI within the last months. But I am frustrated that pragmatic and attainable proposals since july or so have been brushed with skepticism leaving no way forward to have the UI suck less or attain missing features.
I won't fight this battle. Too late, too many people to convince, no agreement in sight, and I'm alone on the Field API + UI front. A decent Field UI will have to emerge in contrib anyway.

But right now, this approach is the only known way forward to have core's UI not force 'form order = display order on all build modes'. Hence my attempt to at least take a crack at it.

te-brian’s picture

So I know that yched kinda threw in the towel in #18, but I'd like to pick it up and hand it back.

The current UI has two major flaws (that yched already pointed out). First, there are two different UI patterns in use. You have this "basic" tab (is the word "basic" used anywhere else in core to describe the teaser + full page of a node?), which has two different build modes on it. Then you have more tabs with may or may not have multiple build modes in the tables. To be honest, I didn't even really understand build modes that well until I looked at yched's screenshot of the proposed improvement. Seeing the vertical tabs in this way let me understand the similarity between teaser and RSS and search a lot more easily than before. This would be especially useful if build modes could have a description associated with them, or some help text.

The second flaw is that these tables with multiple build modes will never allow for D+D sorting per build mode (as yched pointed out).

I ultimately agree with the most drastic approach, which is combining the form ordering with the display ordering. These are, after-all, just different ways of interacting with the same entity. But, this is probably a little too far to go in one patch.

So, I support the improvement yched is going for. Maybe its not the slam-dunk, no question UI for this, but it is a clear improvement in my opinion. As yched also pointed out, I haven't seen any better suggestions, and with the tools we currently have in D7, I can't think of one.

[Edit] Alright, I thought of one. At least split the build modes into their own tabs, or stack them on one page, so they can be sorted. But this isn't as pretty as VT in my opinion.

mrfelton’s picture

I used the patch, and felt there was a definite improvement. The usage of vertical tabs may not be 'correct' in some strict sense of the word, but I feel that they make this part of the user interface a hell of a lot more usable - and I thought D7 was all about usability. I would be be very annoyed if Drupal 7 was released with the existing D6 functionality over what this patch has to offer in terms of usability and UI improvements.

yched’s picture

re webchick #17

I think the main reason this doesn't work is because there is no sensible summary we can display on the tabs of what lies within them

True, but I didn't think summaries were required by contract for VTs. Or, said otherwise, why should the fact that you cannot provide a summary (at least a short one) make you a bad candidate for a 'JS blocks activated by vertically stacked links' UI pattern ?

Other than that, possible summaries:
- The list of fields not set to hidden. Would be handy, but could get arbitrarily long.
- A description of the build mode. Could also be added on top of the table within the content of each tab. But te-brian is right, that's a point I forgot: the current UI with 2 modes in a table doesn't provide a place for a description of the build modes. Adding those would be an API change in hook_field_build_modes(), though, since it currently returns an array in the form 'name' => 'label' (that's a real shortcoming, BTW. My bad).
- Or, well, none.

te-brian’s picture

@yched
The docs for that hook are "TODO". If I read you right it just returns a single-dimensional array of labels keyed by name? Too bad it would be an API change, but I would think more of an "info" style hook would be cool. Might be too late for that one though. [Edit] It would, however, be a usability improvement in either interface.[/Edit]

As for tab descriptions, you could just have "# visible, # hidden", although that doesn't seem too useful.

yched’s picture

About "API change in hook_field_build_modes() to allow build mode descriptions" in #21: #664544: Clean-up entity build/view modes

sun’s picture

Category: feature » task

I've carefully read through all comments. To summarize:

  1. At the very least, we need to split teaser + full as well as search index + result build mode configuration into separate tables or tabs (errr, regardless of orientation ;).
  2. The UX team considers vertical tabs to be inappropriate for this page, since the usual pattern for vertical tabs (entirely optional) does not seem to apply and the current interface separates the configuration across multiple pages.
  3. Usage of vertical tabs would allow the user configure build/view modes on one screen, without losing context, which would especially be helpful for configuring newly added fields, and perhaps, comparing the display configuration for a field in various build modes.

Thoughts on that:

  • It is true that horizontal tabs (local tasks) are a very poor instrument for an infinite list of items. Most themes will start to break when exceeding 6 local tasks. On top of that, visual presentation of secondary local tasks is very poor in almost every theme I ever saw. (Actually, I hope that we can finally get rid of secondary local tasks in D8.) Switching from horizontal tabs to vertical tabs therefore would have benefits on its very own.
  • I think the concern about replacing a navigational pattern with vertical tabs is moot and only caused by habit. Just because there is currently a navigation, that does not mean that the navigational pattern is correct. In this case, I tend to agree that it is incorrect. To note a precedent for this kind of change: #558666-70: UX/security: Revamp text format/filter configuration (equally replaced a navigational pattern in favor of keeping the configuration interface for something together on one page)
  • The UX pattern of vertical tabs is invalidated by the proposed approach due to the following reasons:
    1. There is no required (as in "you have to look at this") form section above the vertical tabs.
    2. The usage of tables within vertical tabs (actually even TableDrags) looks very convoluted. Especially when taking additional (but currently missing) formatter settings into account.
  • a) can be resolved easily, because neither the patch nor the screenshot took into account that we actually have required settings for each bundle: The field "display" (widget) configuration for the entity form.

    All fields that are configured for an entity will appear on the entity form, so configuring them for the form is required. And, instead of configuring a formatter for a field when being viewed, we likely want to configure the field widget to use here. Since formatters have settings, and widgets have settings, we'd have the same interface requirements for each item, whether being contained in the form configuration or the view configuration.

    Also note that this goes hand in hand with the observation and trigger in the original post. For users, there is no difference between "display" and "display". They do not care whether there is a field widget or field formatter at the API level behind it.

  • b), however, I have no idea for. I tinkered about using two-level (recursive) vertical tabs (yes, that is indeed supported!), which would not only resolve the convoluted presentation, but also make room for widget and formatter settings. However, I'm not sure whether that wouldn't slow down the actual configuration and perhaps also invalidate the "comparison" benefit (although... actually, I think it would ease comparisons...?), and more importantly, we don't have "draggable vertical tabs" yet ;) If you read this point, then please avoid just posting "-1", but try to think about alternative solutions instead.

Overall, I agree that the current interface is a total mess, and doesn't reflect the API's configuration options at all. Hence, at the very least, we need to do 1). But doing so will automatically increase the amount of secondary local tasks to its visually supported limit.

My recommendation is to implement this change - of course, along with the required display configuration for the "form" build mode (above the vertical tabs). And afterwards, try to replace the tables with a more flexible and compatible interface in a separate issue.

If we cannot agree on that, then I suggest to do 1), i.e. the separation of build mode configurations first, perhaps in a separate issue.

oriol_e9g’s picture

I think that it's a good improvement. It's a clear, easy and homogeneous solution. How is it not you see that?!!? I'm in shock! O.o

kaakuu’s picture

+1 for what yched says.
Keeping subscribed.

te-brian’s picture

@sun
In #24, are you hinting at putting the "view_mode" :) settings below the edit form? If so, I could photoshop a mock-up so people could see what that looks like. I could also add descriptions to the mock-up (since there is a patch that adds that capability #664544).

[Edit] Maybe what you mean is below the Manage Fields form? That could work because then you just call the tab "Fields".

yched’s picture

Many thx for pushing the thread forward, sun.

I must confess that I don't fully get what the a) suggestion looks like.
- On top, DragTable of fields with the order and widget settings applicable in forms
- Below, a VT box with one tab per build mode, each tab containing the DragTable of fields with the order and display settings applicable in for build mode
?

yoroy’s picture

Hooray for mockups and explorations before deciding on a solution. te-brain: please do create some :)

sun’s picture

Sorry for being wishy-washy. That follow-up was a straight braindump.

Yes, basically both te-brian and yched interpreted it correctly but differently ;)

  1. $view_mode settings are contained in vertical tabs.
  2. $view_mode settings are presented below some other, required settings.
  3. Those required settings are basically the same as the $view_mode settings for a single view mode (e.g. 'teaser'), but differ from $view_mode settings in that they deal with the field widgets, i.e. the entity form (widgets), instead of the view (formatters).
  4. But as explained above, when forgetting about field widgets and formatters for a moment, then we basically have the same field configuration widgets and requirements for both types. So for a moment, we just forget about widgets and formatters, and just think about "display", because fields are "displayed" in forms as well as in teasers/full views and whatever. Which means: We can apply the same user interface pattern to both configuration targets. And since we just forgot about widgets and formatters, and both deal with the "display" of a field, I think it would be most natural to put the configuration for both tasks/targets onto the very same page.
  5. Hence, we can copy + paste one of the current $view_mode sections and place it above the vertical tabs as configuration section for the form - not only to forget about technical disabilities (field presentation == field presentation) but also to validate the usage of vertical tabs, since the field widget configuration is required and cannot be skipped.
  6. This probably means that we most likely want to remove some existing info and links from the current "Manage fields" page, which wasn't affected by this issue thus far, because at least the field widget type configuration would now live on the "Manage display" page. But to be honest, that's very very good -- even when knowing most of the technical innards behind the "Manage fields" interface, I definitely clicked the wrong link in 100% of all cases so far.

    I'm not sure how this could be achieved in a simple way that doesn't require to revamp the entire field UI configuration pages though. One very potential solution would probably be to just output a "Configure" link for the currently selected field widget - AND - have the Overlay do its job here. However, that most likely won't work, because the field display configuration is already presented in an Overlay (which is a bug).

    Or to keep it even more simple, we'd just move the field widget type setting to the "Manage display" page. This would mean that we'd - for now - have almost the same interface for configuring field widgets (above vertical tabs) and field formatters per bundle (as vertical tabs).

  7. What I definitely did NOT mean is to merge the "Manage fields" and "Manage display" pages. Fields need to be added first, before their "display" (widget/formatters) can be configured.
te-brian’s picture

Alright, these are either very close to what I think sun is talking about, or they are completely off base :)

First is Manage Fields with widget selection removed (since widget is a "display" option).

Second is Manage Display with Form order and widget on top.

Descriptions have been brought in for the view modes (ala #664544).

te-brian’s picture

FileSize
18.91 KB

Err, I mean this one. (No Operations on Display).

te-brian’s picture

FileSize
18.22 KB

Taken one step further (towards sun I think). This would be if we allow form labels to be styled in the three different ways also (above, inline, hidden).

yoroy’s picture

Bringing the above inline:

Manage fields mockup (#31):

Manage display mockup (#32):

Manage display mockup (#33):

te-brian’s picture

Just a note. I still think just the vertical tabs from earlier in the issue are worth doing, especially if we add descriptions to the view modes (if #664544 goes through in some form with the info hook). I'd hate to see scope creep derail possible improvements. But I'd also be interested to see where the recent discussions take us.

Noyz’s picture

Tend to agree with yched here. Vertical tabs on the node/add is used to flip through various configuration options relating to the thing you are adding. Vertical tabs via Manage Display is attempting to do the same thing... flipping through various display options relating to the content type.

For the record, while I'm agreeing with Yched, I think we're putting lipstick on a pig here. While this UI might become more usabile, the entire process of adding and configuring content types could be sooo much more obvious (http://www.jeffnoyes.com/content/drupal-cck-fields-ui). But so long as the goal is to stay small and not rethink the model for adding content types, I do think vertical tabs is more discoverable than what's present now.

Bojhan’s picture

@Noyz Its not the same, because in this case you are not adding configuration to the thing you are adding - this is the main object you are configuring.

Although there have been many long replies, I am still not convinced this is better - and it is most definitely choosing "anything" is better then "nothing" over improving it. But no one cares, about not improving stuff for the sake of keeping the mental model of vertical tabs only being "side settings" that one can skip.

sun’s picture

But no one cares, about not improving stuff for the sake of keeping the mental model of vertical tabs only being "side settings" that one can skip.

Of course we do. Look once again at the mockups that te-brian created. The configuration of view modes is entirely optional. If you don't want to configure them, because you don't care or are just happy with the defaults, then you can skip the entire vertical tabs. Above the vertical tabs are the required settings, like everywhere else where we use vertical tabs.

Noyz’s picture

Bojhan, I recognize that it's not the same under the hood, but at the end of the day it's about moving around configurable things and using a familiar paradigm to do so. Do you think users that edit this are going to understand the level of distinction your adding?

If I were to make your statement brief, you're saying "you're not configuring, you're configuring." To which I say, if your configuring, you should do so using an interaction model that's already defined. Hence the statement "at the end of the day, its about moving around configurable things."

yoroy’s picture

Could this work? Just a thought…

view modes

te-brian’s picture

@yoroy
I think that interface is ok, but its not really used elsewhere in Drupal, and we are already seeing backlash against not using perfectly crafted design patterns in the perfect way.

Let me summarize the current choices (for the nth time this thread)

A) Split build modes into their own secondary tasks (so that they can be made sortable in another issue, and for improved clarity).

B) Use vertical tabs to navigate build modes (which many agree is an improvement, but others feel is a violation of VT pattern).

C) Use vertical tabs "correctly" by using a similar form layout to other places VT are used.

D) Leave things as they are (where the UI does not match the API).

I'm not sure how we proceed, because those that like the direction of VT are thinking of ways to make it work, whereas those that do not like this application of VT are just saying "no" without much elaboration or alternative ideas. The frustrating thing is that we all in agreement that something needs to change :)

Maybe we need a new design pattern. Lets call is "Vertical Panes". This crazy new design pattern has a tab-like structure and can be used for non-required settings, or settings that are required but have sane defaults.

yoroy’s picture

te-brain. 41 comments in and we've only just started making an inventory of our options instead of argueing over 1 proposed solution. These things take time. Having options to choose from is a good thing, no need to rush towards implementation.
Also, Fields UI has functionality nowhere else seen in core either, so dismissing proposals based on that doesn't make sense imo.

yched’s picture

[edit: crosspost with yoroy's #42]

Bojhan #37:

it is most definitely choosing "anything" is better then "nothing" over improving it

Sorry, but I have to disagree. I'll restate what I wrote in #18:
"I don't see this as a "half baked workaround for lack of a better solution". My point is that VT *is* the interaction that makes sense for this page. The current UI taken straight from D6, *was* a "half baked workaround for lack of a better solution" back then." / "if D6 core had VTs, CCK contrib would have used them as the natural UI pattern for this."

yoroy #40 (dropdown selector for view mode): not sure how this would work
- if page reload, then there's a workflow issue (are changes lost ?)
- if no page reload / JS update of the screen, then it's not clear what you save exactly (I make changes on a view mode, then switch to a different one - will those changes get saved ?). It seems like an awkward VT replacement, "kind of VT only not laid out as tabs, because VT is forbidden".
I agree with te-brian, it's a completely new UI pattern not used anywhere else in core, will confuse people much more than VTs IMO.

te-brian is right when he talks about the need for 'vertical panes' pattern. I've tried to avoid using the big V***s word so far but, whatever feelings we might have regarding Views UI, we all know that it wouldn't be possible without its 'JS tabs stacked vertically'. It's a valid pattern for valid use cases. There will be others in contrib. We can say that D7 core VTs are "very very much similar but should not be used for this, please develop your own", but I cannot refrain from seeing some absurdity in this.

Mixed feelings on the "Display Fields = 'fields in form' table + 'fields in Views modes' VT set" direction in the screenshots #34.
Are people still OK with this when there are 10-20 fields ? VTs for 'View modes' pushed below a table with 10-20 rows ?
As sun points out, it drastically affects the 'Manage Fields' screen, and in fact the whole Field UI to what is kind of unknown territory.
- d-n-d reorder makes no sense on the 'Manage fields' screen anymore. Then in what order do we list the fields ? Possibly tablesort with custom sort by label or field name could be used here.
- Affects the 'create new field' / 'add existing field' workflows too, since the screen does not mention widgets anymore. So we need an additional step to pick the widget type ?
- Affects the 'edit instance settings too' form, since this is where widgets settings currently live.
- Forces us to find a UI solution for 'widget / formatter' settings, whereas current UI at least has widgets settings. Consistency++, and I agree they are conceptually the same pattern that call for the same solution, but in this case this is consistency towards something we have no answer for :-p.

It's very possibly a good direction, it's also much more than what I myself have the bandwidth to tackle, folks :-/. I also feel a little uncomfortable that such a massive change stems from the mere fact that we cannot have 'vertical panes' without putting "something else" on the page.

yched’s picture

"Fields UI has functionality nowhere else seen in core either"
Amen to that ;-). That's also the reason why I don't find it surprising that it has a use case for VT nowhere else seen in core.

About widget / formatter settings:
I'll shortly remind that the main challenge is that the content of the 'Formatter settings' form depends on the actual formatter currently selected in the dropdown select.
From sun #30:

One very potential solution would probably be to just output a "Configure" link for the currently selected field widget - AND - have the Overlay do its job here. However, that most likely won't work, because the field display configuration is already presented in an Overlay (which is a bug).

Yup. I have trouble imagining a solution to 'formatter settings' that doesn't involve jQUI popups in some way. Hence my interest in the outcome of #87994: Quit clobbering people's work when they click the filter tips link.

Bojhan’s picture

Title: Vertical tabs on the 'Manage Display' screen » Redesign the 'Manage Display' screen

@yched Look, not any of my arguments will convince you otherwise - I am really sad to see you are so completely death-set on not believing my arguments. Vertical tabs is not good for this page, for the primary reason that it changes the way people think about vertical tabs - it is now considered a main-interaction and people will not ignore it anymore (as in many cases they can). You may not lift heavy on this, but its my job to do.

Every vertical tabs issue, been pushed - by the same people, I would really love to not be the only critic. I believe we are completely going to push Vertical tabs to its extreme, to fit this page - which we all know needs an tuned own interaction.

@Noyz At the end of the day, we should make the distinction between what one can skip and what one can't. Just as many OS's do, using different patterns for this should be understood by users. Your analogy is because its configuring, just go with it anyway? Well sorry, but core should be of higher level then that.

@te-brian The alternative has been proposed, it is Form builder - and it might even be more leaning towards contextual-links on individual fields. But that whole idea has been shot down many times already :) I made a serious effort, to work on that - but no one cares enough to work on it.

As yoroy said, lets talk about redesigning this page - and not about using Vertical tabs - I am pretty sure that will be much more interesting.

yched’s picture

Vertical tabs is not good for this page, for the primary reason that it changes the way people think about vertical tabs

Yet 'JS vertical panes' is *the* good pattern for this page.

OK
a) We want to visually educate users to identify VTs as they currently stand in core as "something that you can skip in 1st approach"
b) Yet as explained in #41 / #43, there's no escape for the fact that 'JS vertical panes', as the main organisational UI on a page, are a perfectly valid UI pattern for some use cases - quite possibly in UIs more complex than core avarage UIs: Field UI, Views... Technically, they are strictly equivalent to the current VTs. Yet a core UI that needs these cannot use the technical feature, and a contrib UI that would use VTs nonetheless (and they will) would be wrong doing so.

So instead of trying to find workarounds for "JS switchable content that naturally fit VTs but where a) forbids VTs", like #40 does, and which IMO just results in something awkward, couldn't we instead use VTs with a slightly different visual style (bg color, columns width, borders..) ? So that a 'main page VT' doesn't ruin the visual pattern of "skippable settings".

As in: dissociate the cognitive notion of 'priority level' from the actual UI pattern, which are not in fact actually tied. Forcing this cognitive restriction on the current technical implementation is an unneeded and sad limitation.

webchick’s picture

FileSize
78.49 KB

To get this discussion off the merits / demerits of one particular interface widget, let's start with a description of the problem. Here's what we're trying to solve in this screen:

display fields.png

Essentially, what I think we are doing is defining a default pattern for how these fields can be displayed, and then a number of possible overrides for that default pattern (and this number is undefined; it could be 5 it could be 20...).

As an example, on my entry form, I might decide to move the "Tags" field up more prominently, so that I remember to fill something in there, but when I actually show the node to end-users I don't need the tags to be so in your face. Therefore, I would make the default settings show "Tags" last, which would take effect in all other display modes, but in the display settings for my entry form, I would move it up before body.

This is roughly similar to the concept of Views displays, which I think is why yched and others see Vertical Tabs as the proper UI mechanism for this, since it's the closest thing we have in core.

yched’s picture

(side note: 'build modes' are now 'view modes' after #664544: Clean-up entity build/view modes...)

re webchick #40:
Tags (or more generally taxo terms) is indeed a very good example of 'form order = display order' being a regression compared to D6 "non-field" taxonomy.

what I think we are doing is defining a default pattern for how these fields can be displayed, and then a number of possible overrides for that default pattern (and this number is undefined; it could be 5 it could be 20...)

Hm. Yeah. This is violently broadening the scope of the thread :-p.

If I'm not mistaken, you're pointing a known problem of:
- 'Per-view-mode display settings and order (and grouping - fieldgroups)' is needed (reminder, we have 'settings' but not 'order' currently), it's critical flexibility that 'full node', 'teaser', 'search_index', or other contrib-defined modes: 'print', 'pdf', 'mail', 'newsletter',... *can* be displayed differently. We cannot regress on that.
- OTOH, maintaining one set of 'display settings and order and grouping' per view mode is a 'sea of config'. Add a new field, you need to configure it for potentially 10+ view modes.
Enable a module that adds its own 2 view modes, you're not sure of what settings they will use until you visit the 'Manage display' page and actually configure the settings.

Nothing new, we already have that issue in D6.

So, your formulation definitely sounds sensible to me - in fact, you're getting dangerously close to the 'display styles' crackpipe plan I exposed in #367498-2: Introduce 'display' as a way to group and reuse instance and formatter settings. and #664544-3: Clean-up entity build/view modes:
- display style = collection of display settings for each field, + order + grouping
- user can say e.g "full and search_index use display style 1, all other modes use display style 0 (default, with default settings)"

*That*, however, is challenging code and UI... I was not really expecting core could tackle this.

kika’s picture

Note that the pain we keep feeling with input filter config UI is a little bit similar. Add/remove elements, set the order, per item config etc.

te-brian’s picture

I'm back from X-Mas break and am all refreshed.

re #41: Yeah, that was clearly a rant. Sry :)

@Bojhan, Sry if I implied that you are not contributing to this issue. Sometimes a devil's advocate is very important, to keep things in check.

As far as the design patterns go, I don't think I will ever understand why this is not a proper use case for VT. I guess I will have to just concede that point and contribute ideas for a new direction. It's interesting that kika noticed a similar pattern in the filter config. Really, anywhere where there are add/remove + order + settings (maybe even blocks fit this). Designing for all the similar core interface experiences may threaten this thread to live till D9, but we should approach it in the most logical way regardless :)

te-brian’s picture

After thinking about what webchick said in #47, and the general idea of defaults, I like that concept. If we can have a default set of display settings, then when new view modes are added by contrib modules, or if you add new fields, in most cases you will not have to re-configure a bunch of options. Many view modes will be content to use the defaults.

Also, in general the two main problems we need to solve are:
1) Since there can be an infinite number of view modes, we need them to be arranged in the direction with which web browsers comfortably scale infinitely (ie. vertically).
2) We (at least eventually) want to be able to order fields per view mode, so settings between view modes cannot share a horizontal table row.

Here are some screens that incorporate the idea of default settings. I know, I know, its still vertical tabs :) I do think that this brings us closer to the accepted use case for vertical tabs. If we like the general idea of defaults, but not the use of vertical tabs, then we can go from there.

Also, this does not account for the form order or widget settings. Perhaps they could go above the default display settings, but I did not want to worry about that right now.

yched’s picture

I'll let others chime in, but IMO #52 sounds like a reasonable intermediate approach between the current (one independent set of settings per view mode) and my proposal in #48 (user-defined set of 'display settings', each view mode is assigned to one of the 'displays' with some sort of matrix).

te-brian’s picture

One thing I'm not sure about, how would the storage of default settings work? I guess we add a 'default' view mode in addition to the 'teaser' etc. Then the code to output the fields just needs to check the specific settings first, then fall back on the defaults.

yched’s picture

Yes, we'd need a 'default' or '_default' view mode, present for all entity types.

Other than that, not sure of the best way to store the information that "on bundle 'foo', build mode 'bar' uses the default settings".

Also, to be clarified on the screenshots: what happens when you check the 'override' checkbox ? the table appears with JS ?

te-brian’s picture

what happens when you check the 'override' checkbox ? the table appears with JS ?

In theory yes, although I admit that a potentially sortable table full of select boxes is more complex then what we usually hide.

sun’s picture

Looks cool. Although I'm not entirely sure whether Views' approach of putting the defaults into the first vertical tab isn't more natural. But at the same time, I understand that we need to put the defaults separately above the vertical tabs to not invalidate the vertical tabs UX pattern (which in this case is highly debatable, but well, we need to start considering and respecting UX patterns in the same way we are respecting coding standards, which is technically and logically the same, just a different application layer).

What a wishy-washy comment... anyway - if this allows us to proceed here, and if this is actually doable on the API side, then I'd say let's do it

te-brian’s picture

I opened #671268: Add descriptions for view modes. to see if we can get some discussion on description copy going. No matter what I think we should at least add that to the interface.

yched’s picture

Proof of concept patch. Like the previous patch, the 'Submit' part is not done, hitting 'Save' will kill your grandma.

yched’s picture

Screenshots.

I'm not sure the wording is ideal. 'override / overriden' have a specific meaning in Views / Panels / Ctools exportables, that is not exactly the same as here.

te-brian’s picture

Definitely in the right direction. IMO we are using the vertical tab descriptions, the settings contained within are optional, so I don't see a direct pattern violation.

Please disregard any and all copy that I write :) I agree that override may be the wrong verb here. Not sure of an alternative at the moment.

te-brian’s picture

Perhaps the checkbox instead says "Use Defaults" and is checked on by default. If you uncheck it you get the table. It is a little backwards, however, in that you are unchecking to get more options.

Dries’s picture

Instead of having one global default, I'd much rather have a settings that allows me to set $view_mode_b to be the same as $view_mode_a. So on the "RSS view mode" tab, I could simply select Use same settings as "teaser view" or Use same settings as "full node view". I think that would allow the UI to be simplified to just the vertical tabs part, as there is no global default, per se. This seems to be a more common UI pattern, that is also a bit more flexible. Seems like it would be a UX improvement.

Dries’s picture

Visually, the vertical tabs look like a disaster to me -- in this case. I feel it would be much better to use fieldsets, in this case.

1) It would provide more real-estate for the table.
2) It would be visually less clutered
3) I can actually have two fieldsets open at the same time and look at another configuration. Much easier to compare and contrast big node types.

te-brian’s picture

re #63:
"Use same settings as x" seems like a reasonable idea. It would be useful for cases where there are two or more similar view modes, or where you want RSS the same as teasers and whatnot. It is probably easier to implement on an API level as well. The worst case scenario is parsing the current form post and just duplicating the settings from the chosen mode to copy.

The reason we were trying to add something logical above the vertical tabs is because that has been stated as a condition for use of vertical tabs. Which leads us to ...

re #64:
I agree with 1) and 2). Fieldsets would help out on really narrow resolutions, where the sum of the tabs and the table would be pretty tight. Fieldsets also seem more appropriate to put a table inside of.

3) is probably more up to personal tastes. On a monitor turned vertically, or a high resolution display, you'd be able to easily look at two sets of settings at once. On average displays you would need to scroll up and down. Some people probably do prefer scrolling up and down instead of clicking between tabs. I would personally rather click between tabs, but that's "more like an opinion". The one thing I don't like about fieldsets is that it can create visual clutter pretty easily. If you have a couple of them open and a couple closed, it can be hard to find the ones in-between. In this case the fieldsets have identical contents so they might start to blend together as well.

I think fieldsets are worth exploring. I'll either modify one of my photo-shopped images or tweak the last patch and take screens. "Hold on to your butts".

te-brian’s picture

Implemented Dries' comments (visually only; grandma still threatened if you save).

webchick’s picture

Nice. I actually like that much better. What say you, UX team?

yched’s picture

I also challenge Dries's "Much easier to compare and contrast big node types" argument. Much more up and down scrolling involved with fieldsets, for both small and big node types.

Anyway, let it be fieldsets then.

A couple notes:

- Even without VTs, we still need summaries on the fieldsets. #66 forces you to open each fieldset to see which modes have actual settings, and which modes reuse other modes, that's a problem.
Attached patch does that.
Problem: the display of summaries on non-VT fieldsets seems broken right now - see screenshot. I could not find a simple fix so far playing with firebug.

- Override model: agreed that 'A uses B' is more flexible than 'default / override default'.
It is also a little more complex to grasp.
Unlike the 'default / override default' model, it does not fix the case of 'enable a module that defines its own view modes, how are these view modes displayed until the user actually visits the settings ?'.

It also requires both client and server side logic to prevent cycles (A uses B uses C uses A).
Client side (JS): when user sets mode B to 'use C', then
a) update the selected option to 'use C' for all modes currently set to 'use B'.
b) remove all modes currently set to 'use C' from the list of available options for mode C
[edit: or more precisely, ensure that at any given time, no 'use X' option is available anywhere if X itself is set to use another mode]
Server side (PHP): detect cycles and raise a form validation error (when the form was displayed without JS)

- show / hide the 'custom settings' table with Drupal.behaviors.fieldManageDisplaySourceSelect: this should ideally be done using FAPI #states dependency system, but I cannot for the life of me make it work for a rule like "show this element if the selected option of a select input is x". Works with a checkbox, beats me with a select.

sun’s picture

By using fieldsets, we can easily vertical-tabify this page in contrib, so this may be an acceptible compromise.

1) Regular fieldsets should support fieldset summaries as well. At least in Garland they look + work properly. Seven, as always, is a different can of worms. That should be fixed in a separate issue.

2) Circular references are a problem and the proposed way of taking over settings of another view mode overcomplicates things, IMHO. Considering my own view mode scenarios from the past (that probably count as advanced usage even), I'd say that it's very uncommon to configure multiple "sets" of view modes, in which some would be equal but different from the "default". It is much more common to have a default (most often the 'full' view mode) and either inherit the same settings to most or all others, or alternatively having to override every single view mode.

Taking a step back, I'd say that the ultimate configuration interface would allow to setup view mode settings loosely, and assign them to the actual view modes afterwards. Hence, you would most often configure 1-2 view mode settings, and afterwards, assign those to the view modes ('full' => 1, 'teaser' => 2, 'rss' => 2, etc).

That, however, I fear is too complex to rush in at this point. An almost adequate alternative would perhaps be to have 1 select to choose the default view mode from all view modes, and for each view mode a checkbox to define whether to use the defaults or override them.

yched’s picture

Taking a step back, I'd say that the ultimate configuration interface would allow to setup view mode settings loosely, and assign them to the actual view modes afterwards. Hence, you would most often configure 1-2 view mode settings, and afterwards, assign those to the view modes ('full' => 1, 'teaser' => 2, 'rss' => 2, etc).

Agreed, that's exactly the 'display styles' approach I mentioned at the end of #48. Also agreed that it seems out of reach for core D7.

I also have to point that, whatever the override model we pick for the UI, I don't exactly know yet how this fits with the fact that code-defined fields, for instance a new field created in a contribmodule_install(), can do things like

$instance['display']['mode_1'] = array(settings);
$instance['display']['mode_2'] = array(different settings);
field_[create|update]_instance($instance);

and thus potentially conflict with the UI-defined overrides if mode_2 was set to be the same as mode_1. Granularity clash: UI operates on all fields in a bundle, API operates field-by-field.

te-brian’s picture

I agree with the concern about the A uses B approach. There are a lot more issues to deal with than just a default -> override model. Thinking about sites I've set up, default->override might not be as useful as I first thought anyhow. Teaser always differs from Full, RSS usually has a few fields hidden. Search results often match teaser, but not always.

So, I think there is a least agreement that getting these settings on one page is a good thing. Would it be better to break up some of the improvements mentioned in this thread to other issues. We could implement the fieldsets here, work on sort-ability in another, and look at larger (more API intensive) issues a little later. We probably won't get a killer, perfect UI finished before alpha, but I think we can at least get some major improvements in that allow for much more expandability later.

te-brian’s picture

- show / hide the 'custom settings' table with Drupal.behaviors.fieldManageDisplaySourceSelect: this should ideally be done using FAPI #states dependency system, but I cannot for the life of me make it work for a rule like "show this element if the selected option of a select input is x". Works with a checkbox, beats me with a select.

I also couldn't get the select boxes to behave with #states. That's why I made that function. I'm not sure if #states is triggering off the 'change' event properly. When I get back to work I'll look at that code and try to debug.

1) Regular fieldsets should support fieldset summaries as well. At least in Garland they look + work properly. Seven, as always, is a different can of worms. That should be fixed in a separate issue.

So the code is in place for summaries on regular fieldsets, we just need to file a bug against Seven?

tstoeckler’s picture

Using the "A inherits from B" model doesn't cover the case in which contrib modules provide additional display types. I have to visit the "Manage displays" screen everytime display types are added. If there is a default, which I can configure during my initial site setup, then I only have to go there if i specifically want to change something.

Thinking about it, even if there isn't a UI for a 'default' display, the code still needs to be there for newly added display types, because they have to display *something*.

yoroy’s picture

Great design discussion all around.

My feedback based on the screenshots only:
- Definately liking this direction. Dries nails it in 64. As for having to scroll to compare fieldsets: yes, that's the trade-off for fieldsets, but having multiples visible *at the same time* is the bigger win here (compared to compact, but toggling displays).
- Screenshot http://drupal.org/files/issues/field_ui_display_overriden.png shows how the VTs imply hierarchy or inherentance which the lastest comments tell me that's not really desired or that likely a scenario.
- Nor do the tables align well for quick comparison, another drawback of the VT tabs, their own width breaks the flow/alignment with the 'master/default' display.
- If this would give us summaries for fieldsets, even better, but don't let that distract you :)

So, trying the patch soon and will respond back, but for now: yes, work towards the fieldset based solution.

yched’s picture

[edit: crosspost with yoroy's #74]

re @te-brian #72: "So the code is in place for summaries on regular fieldsets, we just need to file a bug against Seven?"
Probably, but apart from this patch I don't think there's a place in current core that uses summaries on plain fieldsets :-/.

re @tstoeckler #73: well, currently we do display something, of course :-). When a field instance contains no settings for a given build mode, we use the settings for 'full' (happens in _field_info_prepare_instance()). 'full' is therefore a de-facto 'default' mode, but it's not made explicit anywhere in the UI - and there's no real reason to hardcode this behavior on 'full'.

yched’s picture

So to summarize the options on the 'override' question so far:

1) 'default settings / override default': one set of 'default' settings + the option for each actual view mode to either use 'default' or its own set of settings. Newly enabled view modes use 'default' settings
Screenshot: http://drupal.org/files/issues/field_ui_display_overriden.png, but with regular fieldsets instead of VTs.

2) 'default mode / override default': same as 1), but with a select on top of the page allowing you to pick which view mode will act as the 'default'. Thus, no additional table for 'default' settings.
Subtlety: what if mode A has its 'use default' checkbox on, and you change 'default' from B to A ?

3) 'mode A uses the settings of mode B'
Screenshot: http://drupal.org/files/issues/field_ui_manage_display_fieldsets.png
Drawback: tricky handling of circular references, doesn't fix the 'newly enabled view modes' issue.

4) 'display styles': let users configure an arbitrary number of 'styles' (collection of display settings for the fields in a given bundle), in most cases 2 or 3, and assign those to the view modes (full => style_a, teaser => style_b, rss => style_b, ...). One of the styles is the 'default'.
Probably the most flexible option, but also the less clearly defined in terms of UI. It also introduces a new 'animal' (display style) that doesn't exist currently.

All of those solutions have to come to terms with the fact that in the current API (field_[create|update]_instance()), display settings are defined field-by-field (see #70).

te-brian’s picture

@yched
Great summary. Don't forget the simple "Combine view modes into one page via VTs or Fieldsets, and add drag and drop sorting". We should leave that on the table too, IMO. Also, minor minor, don't shoot me, comment: plz take your screenshots at a slightly lower res :)

yched’s picture

@te-brian: yes, my summary in #76 was only about the options to limit the actual # of modes to configure.
I consider "Combine view modes into one page via Fieldsets" to be more or less agreed at this point.

Screenshots: duly noted ;-)

yched’s picture

Attached patch implements per-view-mode d-n-d reorder.

- It is built on top of the last patch, i.e with 'mode A reuses mode B' selects, but as mentioned in #76, this is probably not the override model we want.
- Adds a 'Hidden' section to the table, since hidden fields need to get out of the way of the reordering. Thus you can hide a field by selecting '<Hidden>' in the select dropdown or dragging it to the 'Hidden' rows.
Quite similar to the blocks admin, and the supporting code is largely derived from block.js.
- Submit should work. No tests yet, though.
- It is affected by core bug #303189: Tabledrag doesn't hide columns when the whole table is initially hidden. Thus, you get the 'weight' textfield inputs that should normally be hidden. I made the fieldsets start as non-collapsed just to take the screenshot.

Status: Needs review » Needs work

The last submitted patch, field_ui_manage_display_fieldsets-553298-79.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
26.41 KB

The failing tests in locale.test (test that 'hide body field' work) absolutely do not belong there. Removed them for now.

te-brian’s picture

Tiny update to yched's patch. One of the ids being used in the js was removed, so I changed the js to look for #edit-modes instead.

This is from #81

+Drupal.behaviors.fieldManageDisplaySourceSelect = {
+  attach: function (context, settings) {
+    $('div#modes-wrapper select.view-mode-source')
+      .each(function() {
+        toggleViewMode(this);

This is the change.

+Drupal.behaviors.fieldManageDisplaySourceSelect = {
+  attach: function (context, settings) {
+    $('div#edit-modes select.view-mode-source')
+      .each(function() {
+        toggleViewMode(this);

I'm on crack. Are you, too?

yched’s picture

I continued to work on this a bit after the last followups, before some RL matters kept me away.

Getting back at this. I know it's now terribly late, but I have a proposal that (I think) should make everyone happy, so I'm taking a stab.

VTs have been ruled out a couple followups ago - I cannot work around the 'not enough horizontal real estate' argument, esp. with Overlay.
The 'collapsible fieldsets stacked on top of each other' approach taken in the last patches (screenshot from #73) gets terribly confusing as soon as you get more than 3 build modes and 5 fields - which is pretty quick.

So new proposal comes back at secondary tabs :-) :
- One 'Default' secondary tab, where you configure the default display, used for all view modes unless specified otherwise. A collapsible fieldset lets the user pick the view modes for which he wants specific settings.
- One secondary tab per view mode using non-default settings.
Should be clear in the screenshot below. Also attached a screenshot of the current state of the UI in HEAD, for comparison.

As explained in #18 above, secondary tabs could not scale in a 'one secondary tab per view mode' scenario. But in a scenario with one 'default' + one per 'non-default', there shouldn't be more than 4-5 tabs in most cases, and secondary tabs are viable again.

Patch is still in progress. The 'default' settings are not applied yet, and the 'use custom settings' checkboxes currently have no effect.

I'll continue on this if I get feedback that this still has a chance of being accepted. In addition to fixing the (taxonomy terms follow the same order in node forms and node displays' regression from D6, I'd like to point that this has IMO huge potential to let users adjust their content display - including reordering or hiding Fields *and non-Fields* components of their nodes.

Status: Needs review » Needs work

The last submitted patch, field_ui_manage_display_fieldsets-553298-83.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
36.42 KB

Quick reroll (whitespace conflict), otherwise the same as #83

Status: Needs review » Needs work

The last submitted patch, field_ui_manage_display_fieldsets-553298-85.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
36.54 KB

Retry + reroll after #707724: Call them entities instead of objects $obj / $entity rename

yched’s picture

And reroll after the 2nd bunch of renames.

Although I guess this has dropped off the radar...

te-brian’s picture

What you have done uses the space very well, and accounts for all the improvements and api integration points discussed. The only criticism I have is that it is a pretty new interaction pattern. Good defaults and instructions can help with that though. edit: BTW.. sorry for dropping off the map. RL does strike when you least expect it.

catch’s picture

Subscribing.

moshe weitzman’s picture

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Without this, we have seriously tedious configuration. Bots happy, and code looks good.

yoroy’s picture

Status: Reviewed & tested by the community » Needs review

A whole new direction, interesting. Reviewing this now…

yoroy’s picture

Status: Needs review » Needs work

Fresh checkout of head, applied patch, emptied cache, then on visisting the 'manage display' for Article content type. To recap, a before screenshot first:

Before

manage-display-before

After, with patch in #88

manage-display-after

Review

manage-display-after

- Getting "Notice: Undefined variable: view_modes_selector in field_ui_display_overview_form() (line 586 of /Users/yoroy/Sites/d7/modules/field_ui/field_ui.admin.inc)."
- I wonder if the 'Custom display settings' fieldset should be on its own page/tab ('List')? Not sure. If it stays on this 'Default' page it should be moved to below the table. Defaults first, extra customisation second.
- If you expand the custom display settings fieldset, no checkboxes are selected, but the secondary tabs show 3 display modes already (default, teaser, search index):
manage-display-custom-display-list
- It's not really clear what you are looking at: disconnect between the selected 'Default' tab and the table without a title/header above it. The current (ugly!) solution of another table row above with the display name is better in this regard. Of course the warning message is not helping at the moment, taking up all this space.

Visibility of secondary tabs is a generic problem in Drupal which might hurt us here, but overall I do think this direction is the best we've seen so far.

Bojhan’s picture

The "Custom Display settings" should indeed be moved below the table. However I think the disconnect that yoroy points to can be largely fixed, by having the propper checkboxes checked.

The help text is indeed a bit akward, it should be aimed to explain the general concept - not neccairly what full page vs teaser vs search index means, thats what the labels should make clear.

yoroy’s picture

Yeah, I refrained from reviewing the help text as that is not what this issue touches on. but yes, it tries to explain too much.

Discussed this a bit in IRC with bojhan and agreed the 'custom display settings' should stay on this page.

yched’s picture

Thks for the review, folks.

Patch is still a UI proof-of-concept to some extent. From #83 :

Patch is still in progress. The 'default' settings are not applied yet, and the 'use custom settings' checkboxes currently have no effect. I'll continue on this if I get feedback that this still has a chance of being accepted.

I'll try to finish the missing bits if I can, but time is extremely hard to find currently.

yched’s picture

Status: Needs work » Needs review
FileSize
61.77 KB

Moved this forward, uploading to check where this puts us in terms of test failures.

- moved the 'custom display settings' fieldset to the bottom, as per #94-#96,
- UI now actually works and settings are saved,
- 'default' settings are applied accordingly on entity display when a view mode is configured to use default display,
- $instance definitions can specify display settings for the 'default' view mode :

$instance = array(
  'field_name' => 'field_foo',
  'object_type' => 'node',
  'bundle' => 'page',
  // ...
  'display' => array(
    'default' => array(
      'label' => 'above',
      'type' => 'text_default',
    ),
    'teaser' => array(
      // ...
    ),
  ),
);
field_create_instance($instance);

- View modes can specify whether, by default (before the user configures anything about the view mode), they want to use custom settings or not - i.e : on a fresh install, what 'view modes' secondary tabs are visible for customisation.

function node_entity_info() {
      // ...
      'view modes' => array(
        'full' => array(
          'label' => t('Full content'),
          'custom settings' => FALSE,
        ),
        'teaser' => array(
          'label' => t('Teaser'),
          'custom settings' => TRUE,
        ),
        'rss' => array(
          'label' => t('RSS'),
          'custom settings' => FALSE,
        ),

The patch only specializes 'teaser' mode for nodes. Thus by default with a core install you only get 'default' and 'teaser' to configure. Other view modes (RSS, search index, search results) use the 'default' settings, until the admin chooses to specialize them.
- Stop special-casing 'full' as a hardcoded view mode required on all entities

TODO
- Now that it's not special anymore, possibly rename the existing 'full' view mode to something more meaningful entity type per entity type (eg 'page' for nodes, 'taxonomy listing page' for taxo terms, 'profile page' for users ... ?) - should probably be done in a followup to avoid bikesheding.
- fix tests and add new tests for the new behaviour (the 'Display fields' part of the UI is currently not tested at all)
- adjust $instance's currently defined in core (body, field_image, tags...)
- Add some comments and documentation in a couple places (still left as @todos in the patch)

Screenshots are roughly the same as in #83, except that the 'custom display settings' fieldset is now below the table.

Status: Needs review » Needs work

The last submitted patch, field_ui_manage_display_fieldsets-553298-98.patch, failed testing.

moshe weitzman’s picture

Thanks for the push forward. Tests failures don't look too bad.

yched’s picture

Status: Needs work » Needs review
FileSize
74.16 KB

This should fix test failures.

moshe weitzman’s picture

Yup, bot is green. Code looks reasonable to me. I'll work toward RTBC once those TOOD get cleared up.

andypost’s picture

Issue tags: +Needs documentation

Great! now there's ability to reorder fields per-mode.

Still confused why fieldset approach was denied. I'd like a VT style which in this case could be applied from contrib.
This secondary tabs is hard to find under main tabs - this is a most confusing IMO!!!

hook_field_ui_view_modes_tabs() is removed so this change should be reflected in docs.

EDIT: and whats about 'settings for formatter" ?

yched’s picture

Moving slowly on this, very limited quality time for drupal coding these days, plz bear with me.

The 'vertically stacked fieldsets' approach was IMO very confusing as soon as you had 3+ view modes to configure and enough fields for the config table for one mode to take more height that the viewport.

Settings for formatters is not addressed here unfortunaltely. The redesign in the patch would probably be a prerequisite, but they have their own set of UI challenges.

yched’s picture

Updated patch.

In addition to what is described in #98 :

- refactors hook_field_extra_fields(). Having separate d-n-d lists for forms and for displays means we need to be able to separate the non-field components that appear in forms from those that appear in displayed entities.
I tried hard to find a non-API breaking way of doing this, to no avail :-(. The current solution, taken from CCK D6, was awkward anyway - "Give me your 'extra fields' for forms, and tell me if some of them have a different name in displayed entities". In practice, the lists for 'form' and for 'display' are quite different.

- sets fields to 'hidden' if they have no explicit settings in a given build mode, instead of defaulting to the settings used for the 'full' mode, as per #715826: field display mode should default to hidden . No more surprises on your displayed nodes when enabling a random module that creates a new field.

- adds hook_field_display_alter() to let modules alter display settings stored by Field UI before entities are displayed.
a) this lets a smarter "define entity display styles / assign styles to view modes" UI model (described in #48 above) be implemented in contrib.
b) this allows a cleaner solution to the "do not write field labels in search index" issue. Currently hardcoded in field_attach_view(), done in node_field_display_alter() with the patch. Comment.module should do the same - see #680992-24: comments are added to search index without checking access.

- still a couple @todos, mainly documentation

- still no tests :-(

no change UI-wise, screenshots in #83 still apply.

[edit: Oops, patch messes indents in node.module. Will upload a fixed patch later today]

plach’s picture

yched’s picture

Without the cruft hunks in node.module.

Status: Needs review » Needs work

The last submitted patch, field_ui_manage_display-553298-107.patch, failed testing.

andypost’s picture

Any ideas how to review 100k patch and fix all affected tests?

yched’s picture

Status: Needs work » Needs review
FileSize
103.33 KB

Oops, wrong var names in a late refactor. This one should have a tad less exceptions...

andypost’s picture

+++ includes/common.inc	5 Apr 2010 00:56:08 -0000
+++ includes/common.inc	5 Apr 2010 00:56:08 -0000
@@ -6243,6 +6243,11 @@ function entity_get_info($entity_type = 
+        foreach ($entity_info[$name]['view modes'] as $view_mode => $view_mode_info) {
+          $entity_info[$name]['view modes'][$view_mode] += array(
+            'custom settings' => FALSE,

If defaults are set for every entity view mode. What for next lines for every content?

+++ modules/book/book.module	5 Apr 2010 00:56:08 -0000
+++ modules/book/book.module	5 Apr 2010 00:56:08 -0000
@@ -240,6 +240,7 @@ function book_entity_info_alter(&$info) 
   $info['node']['view modes'] += array(
     'print' => array(
       'label' => t('Print'),
+      'custom settings' => FALSE,

this

+++ modules/comment/comment.module	5 Apr 2010 00:56:08 -0000
+++ modules/comment/comment.module	5 Apr 2010 00:56:08 -0000
@@ -108,6 +108,7 @@ function comment_entity_info() {
       'view modes' => array(
         'full' => array(
           'label' => t('Full comment'),
+          'custom settings' => FALSE,

this

+++ modules/field/tests/field_test.entity.inc	5 Apr 2010 00:56:08 -0000
+++ modules/field/tests/field_test.entity.inc	5 Apr 2010 00:56:08 -0000
@@ -14,9 +14,11 @@ function field_test_entity_info() {
   $test_entity_modes = array(
     'full' => array(
       'label' => t('Full object'),
+      'custom settings' => FALSE,
     ),
     'teaser' => array(
       'label' => t('Teaser'),
+      'custom settings' => TRUE,

this

+++ modules/node/node.module	5 Apr 2010 00:56:09 -0000
@@ -195,12 +195,15 @@ function node_entity_info() {
+          'custom settings' => FALSE,

and others

129 critical left. Go review some!

Status: Needs review » Needs work

The last submitted patch, field_ui_manage_display-553298-110.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
106.13 KB

This one should fix test failures.

@andypost #111 : I'm not sure I understand your question :-)

andypost’s picture

@yched I mean - if you set defaults ('custom settings' => FALSE) so there's no need to set this default for each content type

Status: Needs review » Needs work

The last submitted patch, field_ui_manage_display-553298-113.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
109.31 KB

Let's see this one.

re andypost : Yes, it's true that those 'custom settings' => FALSE are not strictly required since FALSE is the default, but since this basically 'disables' the view mode (bypass current display settings for the mode and use 'default' settings instead), in this specific case I prefer to leave explicit values instead of silent defaults.

andypost’s picture

Tested this patch and I think it's ready to go.

andypost’s picture

Status: Needs review » Needs work
FileSize
75.44 KB

default display should not be trimmed!

EDIT: also all new fields for custom displays are falls into HIDDEN part of "manage display"

yched’s picture

Status: Needs work » Needs review
FileSize
112.21 KB

Rerolled, + fixed error on 'body' formatter reported in #118.

also all new fields for custom displays are falls into HIDDEN part of "manage display"

That's the intended behavior for the patch, as per #715826: field display mode should default to hidden . Fields are hidden unless you explicitly configure their display, instead of currently : "fields are displayed with the default formatter in all view modes the minute they're created", which does not make much sense in practice, and is in fact rather a nuisance.
It should be doable to adjust the 'create new field' UI workflow so that when you get to the 'instance settings' form, you can choose the formatter (or hidden state) for the field in the various view modes, rather than having to go through each 'Manage display' page for each view mode. That's for a followup patch, though, this one is big enough IMO.

andypost’s picture

Tested minimal and standard install.
Image, Forum, taxonomy and comments works

Drag'n'Drop for feilds works - FF IE8 Gchrome (win)

Code review - there are a lot of todos and some code indention bugs

Suppose better to commit this patch and fix code-style in follow-ups

+++ modules/field/field.module	11 Apr 2010 17:40:50 -0000
@@ -180,10 +180,10 @@ function field_theme() {
-    ),
+  ),
     'field_multiple_value_form' => array(
       'render element' => 'element',
-    ),
+  ),

typos?

+++ modules/field/field.module	11 Apr 2010 17:40:50 -0000
@@ -231,15 +231,15 @@ function field_modules_enabled($modules)
-    ->fields(array('active' => 0))
-    ->condition('module', $modules, 'IN')
-    ->execute();
+  ->fields(array('active' => 0))
+  ->condition('module', $modules, 'IN')
+  ->execute();
 
   // Track fields whose storage backend is being disabled.
   db_update('field_config')
-    ->fields(array('storage_active' => 0))
-    ->condition('storage_module', $modules, 'IN')
-    ->execute();
+  ->fields(array('storage_active' => 0))
+  ->condition('storage_module', $modules, 'IN')
+  ->execute();
 
   field_cache_clear(TRUE);
 }
@@ -256,18 +256,18 @@ function field_associate_fields($module)

@@ -256,18 +256,18 @@ function field_associate_fields($module)
   foreach ($field_types as $name => $field_info) {
     watchdog('field', 'Updating field type %type with module %module.', array('%type' => $name, '%module' => $module));
     db_update('field_config')
-      ->fields(array('module' => $module, 'active' => 1))
-      ->condition('type', $name)
-      ->execute();
+    ->fields(array('module' => $module, 'active' => 1))
+    ->condition('type', $name)
+    ->execute();

typos?

+++ modules/field/field.module	11 Apr 2010 17:40:50 -0000
@@ -256,18 +256,18 @@ function field_associate_fields($module)
-      ->fields(array('storage_module' => $module, 'storage_active' => 1))
-      ->condition('storage_type', $name)
-      ->execute();
+    ->fields(array('storage_module' => $module, 'storage_active' => 1))
+    ->condition('storage_type', $name)
+    ->execute();

same

+++ modules/poll/poll.module	11 Apr 2010 20:45:05 -0000
@@ -208,16 +208,32 @@ function poll_node_info() {
+    'display' => array(
+      'poll_view_voting' => array(
+        'label' => t('Poll Vote'),
+        'description' => t('@todo'),
+        // @todo
+        'weight' => 0,
+      ),
+      'poll_view_results' => array(
+        'label' => t('Poll results'),
+        'description' => t('@todo'),
+        // @todo

todo?

+++ modules/system/system.api.php	11 Apr 2010 17:40:50 -0000
@@ -133,7 +145,7 @@ function hook_entity_info() {
-      'path callback' => 'node_path',
+      'uri callback' => 'node_uri',

is this related?

110 critical left. Go review some!

yched’s picture

Thx for reviewing, andypost.

Indents - oops. My Eclipse messed with indents in a couple files, I thought I had caught them all. Should be fixed now.

'path callback' / 'uri callback' : no, not really related, but the sample code in hook_entity_info() is currently stale, so updating it with the new code in node_entity_info() also replaces this.

todos : I know... Mainly documentation stuff. I'm slowly trying to address them, but I only have very limited drupal time currently :-(

andypost’s picture

Now code is much cleaner. Still a lot of TODOs about docs.

As I write above patch mostly ready except todos

yched’s picture

A couple TODOs less, still 2 pending :-(

andypost’s picture

Only 5 TODOs, and it's ready!

+++ modules/field/field.api.php	14 Apr 2010 13:15:34 -0000
@@ -9,53 +9,56 @@
+ *   @todo

1

+++ modules/field/field.attach.inc	14 Apr 2010 12:15:56 -0000
@@ -1182,7 +1183,7 @@ function field_attach_query_revisions($f
   // @todo: resolve this more generally for both entity and field level hooks.

2

+++ modules/field/field.crud.inc	14 Apr 2010 12:29:59 -0000
@@ -734,30 +752,55 @@ function _field_write_instance($instance
     // TODO: what if no 'default_widget' specified ?

3

+++ modules/field/field.module	14 Apr 2010 12:36:35 -0000
@@ -365,75 +365,272 @@ function _field_sort_items_value_helper(
+ *   @todo format.

4

+++ modules/taxonomy/taxonomy.module	14 Apr 2010 13:15:07 -0000
@@ -101,6 +101,7 @@ function taxonomy_entity_info() {
         // @todo View mode for display as a field (when attached to nodes etc).

5

yched’s picture

Only 1 and 4 are actually in the patch, the other ones are already in current HEAD ;-)

yched’s picture

Days are *really* too much packed for me right now. I should be able to get some progress on the last missing doc @todos next week (around May 4th or 5th)

joachim’s picture

Subscribe.

yched’s picture

Slowly getting out of the craziness. Straight reroll for now.

Status: Needs review » Needs work

The last submitted patch, field_ui_manage_display-553298-128.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
116.5 KB

Should be done now.

Attached patch :
- should fix test failures
- adds the missing doc @todos
- uses the new 'drupal_alter() hook suggestions' (#765860: drupal_alter() fails to order modules correctly in some cases) for the new hook_field_display_alter() / hook_field_display_ENTITY_TYPE_alter() (alter display settings for a field just before it gets displayed)

joachim’s picture

Tested, and forms seem to work fine.

I think the 'Custom display settings' section of the form would be clearer if it had its own subtab; as it is, it feels like you're editing the thing and its controls in the same place.

swentel’s picture

Subscribe, need to follow this to see what I have to port for my ds project for D7.

amc’s picture

subscribe

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Suppose it's ready to go!

To fix notices like

Notice: Undefined index: default in field_ui_display_overview_form() (line 604 of /var/www/d7/modules/field_ui/field_ui.admin.inc).

Need to re-save fields at admin/structure/types/manage/{type}/display for each node-type or update all instances win custom function because head2head updates are not available

moshe weitzman’s picture

yched’s picture

Status: Needs work » Reviewed & tested by the community

So, hum, I guess this could use a patch summary :

- Lets displayed entities use a different ordering than forms. Each view mode can use its own ordering.
So far, the order defined on the 'Manage fields' screen is used for both forms and displayed entities - which is both painful, and problematic for 'non Field' components, that are usually different in forms and in displayed entities. Note that the Field API supports this since several months, but the UI doesn't (just saves the weights defined on the 'Manage Fields' screen over both form and display).
This also opens the door for form-specific or view-mode-specific 'groups' (fieldsets, "sections", JS tabs...), as opposed to CCK D6 fieldgroups, that apply to both forms and displayed entities.

As a side bonus, non-Field components can be reordered or set to 'hidden' too (see screenshot). Fieldgroup module D7 should let them be inserted in fieldgroups too.

- Introduces the notion of 'default' display settings, used for all view modes that are not explicitly specified to use custom display settings, and adds the corresponding UI.
Screenshots: Before / After
(only difference : the "CUSTOM DISPLAY SETTINGS" fieldset is now below the table, as requested in #94 / #96)

This mitigates the 'sea of config' effect (1 display setting per field per view mode), which already exists, but is made much worse by 'per-view-mode order'.
Users can select amongst all available view modes ('display flavors') the ones that use a dedicated set of settings, and the ones that just fall back to 'default'. This is done bundle by bundle. Meaning, 'rss' view mode can be customized for articles but simply fallback to default for pages.

--> In most cases, configure 'default' and 'teaser', voilà. Later when you want your RSS feeds to look different, tick 'rss' and configure it.

- Changes the behavior of displayed fields
Fields for which no display settings are found for a given view mode, are hidden (instead of currently falling back to the display settings specified for 'full' mode, which is a mess and adds lots of unpredictability)

--> All fields are hidden in all view modes until they are explicitly set to be visible in a given view mode, as per #715826: field display mode should default to hidden .
This applies to newly added fields, either programmatically (only visible in the view modes specified in the $instance definition) or through the UI (hidden everywhere until the user visits the 'Manage Display' page),
and also to newly exposed view modes (view modes exposed by a module you just enabled - by default they will re-use the settings of the 'default' mode).
This highly reduces the uncertainty about what happens when a module adds fields (but doesn't know about all existing view modes) or view modes - currently, the minute you enable the module, "something" is displayed that you don't really know about until you visit the 'Manage display' screens.

Also, field_create_field() makes sure that a weight is provided for every context (form, view modes). If not, it gathers the weights of existing fields and makes sure the field appears at the bottom. No more random / erratic placement.

More technically :

- Stops special-casing the 'full' view mode, which currently acts as both a 'default' mode and a regular-but-required view mode on all entity types, and that each view mode uses differently according to its needs - node page for nodes, listed comment for comment, taxonomy listing header for taxo terms, etc...).
'full' is now a regular view mode like the others. It might make sense to keep it or rename it on an entity type per entity type basis - see #721754-36: A node cannot be displayed in different view mode on its own page.

- 'default' is not an actual view mode, only a collection of settings, which each view mode can use (or not). You're not supposed to call field_attach_view($entity, 'default'), but field_attach_view($entity, 'full') or field_attach_view($entity, 'teaser') like before.
The $instance definition arrays fed to field_create_instance() / field_update_instance() accept settings for the 'default' mode, in addition to (currently) settings for each separate view mode. See code samples in #98.
This also means that those display settings in $instance ("This field in this view mode uses this formatter / is hidden") are not 100% prescriptive anymore. They are saved, but will only be actually applied at runtime if/when the view mode is configured to use its own settings instead of falling back to the default.

- 'bundle settings' :
which view modes use 'default' and which use custom settings;
weight and visibility of non-Field components in each view mode and in forms;
are stored in a serialized 'field_bundle_settings' variable, and accessed by helper functions :
field_bundle_settings()
field_view_mode_settings()
field_extra_fields()

- API addition: in hook_entity_info(), each exposed view mode can specify whether, before any user action, the view mode should use the default settings (by default) or custom settings. See code samples in #98.
Core uses this to specify that only node 'teaser' is special (the core-defined body and image fields use specific display settings for this mode), all other node view modes use 'default' by default - meaning, they all are displayed like on the 'node page' by default.

- API addition: introduces hook_field_display_alter(), to alter the display settings used for a field before it gets displayed. Comes with a companion hook_field_display_ENTITY_TYPE_alter().
This is *the* enabler for alternate contrib Field UIs (even possibly partial). Also, we use it to hide field labels from the search index (currently hardcoded in field_default_view() - see #680992-24: comments are added to search index without checking access)

- API removal : Field UI exposed the b*tt-ugly hook_field_ui_view_modes_tabs(), to collect info on how to group view modes in secondary tabs on the 'Manage display' page. This is now gone, each view mode *that is configured to use custom settings* now gets its own secondary tab. Actual visibility of the tabs are determined by the _field_ui_view_mode_menu_access() menu access callback.

- API break: hook_field_extra_fields() (exposes non-Field components in entities) now needs to separate the components that appear in forms and in displayed entities. This is the only API change here - I've tried hard, but no way around this. If we want d-n-d reordering, we need a full list of what's in there, and it's usually fairly different in forms and in displayed entities.

yched’s picture

Note : this redesign is also a pre-requisite for "UI for formatter settings", which might not be as far away as I thought it would. This requires the screen real-estate reshuffle brought by this patch, though.

Will try to post a proof of concept patch in #796658: UI for field formatter settings soon.

chx’s picture

Rerolled against HEAD as one hunk failed.

chx’s picture

joachim’s picture

Status: Reviewed & tested by the community » Needs work

Patch in 139 is missing a file -- dreditor reports 32 rather than 33 files and I'm getting a theming error.

I'm heading out to enjoy the weather ... When I get in tonight I'll look at moving the Custom display settings fieldset to its own tab. I think that makes the UI clearer and this " if ($view_mode == 'default') {" suggests it'll make clearer code too :D

If someone can reroll this in the meantime that would be ace :)

yched’s picture

Failing hunk was after #805228: Unnecessary query in taxonomy_field_extra_fields(). Rerolled with the missing new file (patch renames a template)

I'm really not sure about moving the Custom display settings fieldset to its own secondary tab, but feel free to take a crack at it so that people can compare :-).

sun’s picture

I'd highly recommend to get this sucker in first; smaller changes like moving that fieldset to a new tab can easily follow afterwards. We also want to allow yched to finally be able to forget about this major issue. :)

Awesome work, yched!

Attached patch merely fixes and cleans up a couple of comments, coding style issues, and other minor stuff.

sun’s picture

Priority: Normal » Critical
aspilicious’s picture

+/**
+ * Alters ...
...
+ */
+function hook_field_display_ENTITY_TYPE_alter(&$display, $context) {

+/**
+ * Alter ...
....
+function hook_field_extra_fields_display_alter(&$displays, $context) {

Why the difference in verbs?

yched’s picture

Same as #142, with aspilicious' remark in #144 fixed.

re @joachim - UI adjustments : actually, I agree with sun. The UI design took several weeks and 10s of followups from many people to settle earlier in the thread before I embarked in the actual underlying code work, and the current version seems to have an overall agreement. So while it's fully possible that it can be further adjusted, I'd rather not open a new round of design debate before the main patch is in :-).

Many thanks for reviewing the monster and taking care of the fixes, sun !
I overall agree, with two nitpicks :

- #141 had : (@defgroup field_structs PHPdoc in field.crud.inc, about the options in $instance['display']['teaser'])

+ *         Those options might be overlooked if the site administrator
+ *         specified that the 'teaser' view mode should use default settings
+ *         for this bundle.

Got removed in #142.
IMO this is important info that should appear somewhere. Do you think it's inappropriate there, or should use a better wording ?

- #141 had : (PHPdoc for field_create_instance(), about $instance['display'][$view_mode])

+ *     View modes not present on [er, should probably be "in"] the definition are left empty, and the field will
+ *     not be displayed in this mode until site administrators configure fields
+ *     display with Field UI.

#142 has :

+ *     The field instance will be hidden for view modes that use custom field
+ *     display settings, unless defined here.

which I find less accurate - the '"unless admin uses Field UI to say differently" part is important IMO. Can we find a middle-ground ?

sun’s picture

I was irritated by what those comments were trying to tell me, so I removed/changed them. We can re-add/optimize them further, but not sure whether it's too important, since they mainly state that something can or needs to be configured (oh really? ;) -- at the very least, those comments shouldn't hold up this patch.

+++ modules/field_ui/field_ui.admin.inc	22 May 2010 13:48:20 -0000
@@ -155,17 +155,17 @@ function field_ui_field_overview_form($f
     $form[$name] = array(
       'label' => array(
-        '#markup' => t($extra[$name]['label']),
+        '#markup' => t($extra_field['label']),

1) t() looks wrong to me here; the hook implementations already use t() for the returned extra definitions.

2) Missing check_plain()?

Powered by Dreditor.

yched’s picture

- Rerolled (fuzz after #629252: field_attach_form() should make available all field translations on submit)

- Fixed the t() / check_plain() issue pointed in #146.

- Reworded the polemical comments a bit.
1st one :
"Those options will only be actually applied at run time if the view
mode is not configured to use default settings for this bundle."
That's the "display settings in $instance are not 100% prescriptive anymore" part I outlined in my summary in #136, and this is important to be written down somewhere. "Hey, I created my instance to be hidden in 'rss' mode, why does it still show up ?" can be surprising at first.

2nd one :
"View modes not present in the definition are left empty, and the field
will not be displayed in this mode."
Removed the "... until configured differently in field UI because, you know, Field UI lets you configure things" part :-p.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Reviewed and committed to CVS HEAD. We'll want to move this issue to the "head 2 head upgrade" project so I recommend creating a new issue for follow-up work.

Dries’s picture

Project: Drupal core » HEAD to HEAD
Version: 7.x-dev »
Component: field_ui.module » Code
Status: Fixed » Needs work

Moving to HEAD to HEAD. Will need an upgrade path.

yched’s picture

Yay. Thx Dries.

Upgrade path should consist in adding display settings for 'default' mode to every existing field instance. Will try to write an update func soon.

yched’s picture

Status: Needs work » Needs review
FileSize
3.28 KB

Here's a patch for head2head. My 1st actual encounter with the module, I hope I got the logic right.

yched’s picture

Project: HEAD to HEAD » Drupal core
Version: » 7.x-dev
Component: Code » field_ui.module
Status: Needs review » Reviewed & tested by the community

Oops, back to Drupal project. The commit missed the file addition / file deletion.

Patch attached for the missing bits :
added : modules/field_ui/field_ui-display-overview-table.tpl.php
removed: modules/field_ui/field_ui-display-overview-form.tpl.php

yched’s picture

forgot the patch

Status: Reviewed & tested by the community » Needs work

The last submitted patch, field_ui_manage_display-553298-152.patch, failed testing.

hgurol’s picture

lol, its the first time I have seen spam here...

catch’s picture

webchick’s picture

Status: Needs work » Fixed

Committed #153 to HEAD. Not sure what testbot was freaking out about; it seemed to apply here just fine.

yched’s picture

Posted an initial patch in #796658: UI for field formatter settings

andypost’s picture

Status: Fixed » Reviewed & tested by the community
FileSize
582 bytes

Patch from #153 has broken format - there's a windows line ending at the end of 'modules/field_ui/field_ui-display-overview-table.tpl.php'

This should be fixed before #796658: UI for field formatter settings

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 796658-winLN-d7.patch, failed testing.

andypost’s picture

FileSize
10.07 KB

This patch could not be applied because of this bug. Here is a screenshot

catch’s picture

Status: Needs work » Reviewed & tested by the community
yched’s picture

+1 on applying the fix in #162 asap. It seems to be making the bot unable to apply patches touching that file.
Sorry about that.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Applied the patch in #160.

rfay’s picture

Issue tags: +API change

I guess it must be an API change if HEAD2HEAD has an issue filed....

Please let me know when thing that break everything go in, and I'll announce it on the Dev list. If you give me a short description of how the API has changed (or will break existing modules, if that's not the same thing) then I'll write it up for the Developers' list.

Broke #148 broke Node Example in Examples (again). Can't look away for a minute.

Please folks: Write an issue summary and an implications summary when you RTBC or when an issue like this is committed. How would I ever guess that an issue titled "Redesign the Manage Display Screen" would break Node Example?

Undefined index: custom settings, field.module, line 462

yched’s picture

If I get things right HEAD2HEAD adresses db changes, not API changes.

FWIW, API changes were listed in #136 (end of the post). AFAICT, they don't affect node_example.module ?

The issue was not tagged 'API break', though. [edit : er, was tagged 'API change', which in fact I think is the correct tag ?]

rfay’s picture

node_example.module was broken because 'custom settings' must now be provided for 'view modes'. Technically, you could say that it wasn't "broken", but only issued a new warning, but that meant that the tests fail, of course. But code was required to make it better. A casual look at the patch said to me that this update was done on all the core node types.

#136 is in fact an excellent summary. It's a great thing to mention a great summary like that when something goes RTBC or gets committed, since we don't have an "issue summary" field yet. Hoping we'll get that into project module.

#811556-1: HEAD broken on Node example again fixed node_example.module.

yched’s picture

node_example.module was broken because 'custom settings' must now be provided for 'view modes'. Technically, you could say that it wasn't "broken", but only issued a new warning, but that meant that the tests fail, of course. But code was required to make it better. A casual look at the patch said to me that this update was done on all the core node types.

Hm, interesting. entity_get_info() has code to provide default values for missing entries in hook_entity_info(), which includes defaulting 'custom settings' to FALSE for view modes. However, this 'fill-in defaults' phase happens between hook_entity_info() invocation and hook_entity_info_alter() invocation, so it doesn't fix the data added in alter phase. Which means hook_entity_info() can be concise and forget implicit default values, while hook_entity_info_alter() can't.

I think the logic behind this was to allow the hook_alter implementations to be sure of what they find in the $entity_info argument -meaning no 'if (isset($entity_info['node'][$some_property])' dance to take care of implicit values not filled-in yet. However, this has the drawback outlined above. 2 competing DX considerations. Ideally, defaults should be filled-in after both invocations.

This goes beyond just the 'custom settings' property introduced here, and beyond hook_entity_info() itself. hook_*_info() / hook_*_info_alter() is a very common pattern throughout drupal (including hook_menu() and other hooks out of the 'info' space)...

catch’s picture

Status: Fixed » Active

I'm re-opening this because I think the current 'custom display' logic is a bit wonky.

If you define field instances via the API, then it's perfectly possible to add display settings to an instance whether or not it has custom display settings defined. It took me a bit of time to figure out why my field instances was using 'default' instead of 'full' on a node view.

Ideally the custom settings should apply to the UI only, and if I define an instance setting for a display mode, then it should use that if available, and fall back to default if it's not - rather than completely disabling the view mode and using default anyway. Since we have the instance available in field_get_display(), is there any reason not to do this?

yched’s picture

re #170
- then UI would say 'mode_a' = 'default', except in practice there would be differences, which the user wouldn't even be able to see in the UI since it offers no dedicated 'Manage display' page for 'mode_a'. Bad.
- besides, we'd have no way of knowing if those settings are there because they were API-specified, or UI-specified (ie view mode was once 'custom' and switched back to 'default' later)

I pointed that behavior in the summary in #136 ("display settings in $instance ("This field in this view mode uses this formatter / is hidden") are not 100% prescriptive anymore" - post is big, so I outlined this part in bold). I even had to 'fight' a little with sun to add back a comment about this behavior (#145 - #147).

catch’s picture

Priority: Critical » Normal

hmm, switching back from the ui is tricky, unless we went through and deleted settings for view modes when they're set back to default. What about making the API enforce not allowing display settings when creating or update instances for view modes which have no custom settings?

yched’s picture

"What about making the API enforce not allowing display settings when creating or update instances for view modes which have no custom settings?"
that would mean a contrib module doing a f_create_instance() is in a tricky position because it doesn't know where it lands (how did the admin configure the view modes in the existing site ?)

catch’s picture

Hmm, I don't really see a good way 'round this, either the UI gets unreliable or programmatic instance creation does, however I think it's worth a mention in field_create_instance() that if you want to ensure display settings other than default are used, you'll also have to ensure in hook_entity_info_alter() that 'custom settings' => FALSE. Will have a think on how to word it.

pwolanin’s picture

Seems related to #886152: All fields are hidden after the administrator newly configures a view mode to not use 'default' display - but this thread is too long to really grasp what's going on and what's proposed versus current.

ksenzee’s picture

I ran into this as well when working on the upgrade path from upload.module to file fields. Ideally we'd show a file table on the full node view, but hide uploads in the default view mode. I don't see a way to make that happen programmatically, which seems a shame. I understand the reasoning here, though, and I'm not sure what the right solution is.

And I agree #886152: All fields are hidden after the administrator newly configures a view mode to not use 'default' display is related. With the patch there, the instance display settings are respected after you enable custom display settings for the view mode. Without the patch, the instance display settings might as well have never existed.

tstoeckler’s picture

Status: Active » Fixed

This has been fixed through #886152: All fields are hidden after the administrator newly configures a view mode to not use 'default' display.
Also, in case of follow-up issues, it probably makes sense to open new issues, as this has seen quite some different directions/discussions.

Status: Fixed » Closed (fixed)
Issue tags: -Needs usability review, -Usability, -API change

Automatically closed -- issue fixed for 2 weeks with no activity.