Convert views/views_ui/templates/views-ui-display-tab-setting.tpl.php to Twig

first draft

Tagging.

Applies cleanly, displays properly. Gets description, 'zebra' and settings links as needed.

Back to needs review due to missing documentation of variables, sorry.

This could use some logic out of the template and into + cleaned up in preprocess. Assigning to myself. Patch coming up.

Moving to Core queue.

Adding twig tag

#6: drupal-views-ui-display-tab--1843774-6.patch queued for re-testing.

#6: drupal-views-ui-display-tab--1843774-6.patch queued for re-testing.

Thanks for working on that one!
I will hopefully review all twig issues.

+++ b/core/modules/views/views_ui/templates/views-ui-display-tab-setting.html.twigundefined
@@ -0,0 +1,33 @@
+  {% if description -%}
...
+  {%- endif %}

Just wondering why we need the two "-" here.

+++ b/core/modules/views/views_ui/templates/views-ui-display-tab-setting.html.twigundefined
@@ -0,0 +1,33 @@
+    {% set separator = '<span class="label">&nbsp;|&nbsp;</span>' %}
+    {% for link in settings_links %}
+      {{ link }}
+      {% if loop.length > 1 and not loop.last %}
+        {{ separator }}
+      {% endif %}
+    {% endfor %}

Couldn't we just use http://twig.sensiolabs.org/doc/filters/join.html for that? This seems to be the best way to simplify this difficult logic.

Tagging.

Code standards review of #6:

- We're not talking about PHP data types in Twig template documentation.

- Don't mix attributes.class with hardcoded classes like "views-display-setting" in a Twig template if it can be avoided, move these into the preprocess.

+/**
+ * Prepares variables for views-ui-display-tab-setting.html.twig.
+ */

Not quite right for the preprocess function docs, see #1913208: [policy] Standardize template preprocess function documentation. The style is wrong and this is missing @param.

- I like the idea of using join if we can as suggested in #12. Somebody should try that out...

Thanks for reviewing @thedavidmeister. Tagging as novice for the tweaks in #14, but I have a feeling we're not going to be able to solve attributes.class as a whole during conversion, IMO it can be left as is:

Don't mix attributes.class with hardcoded classes like "views-display-setting" in a Twig template if it can be avoided, move these into the preprocess.

Docs update and join added.

The markup before and after patch in #16 match. The Twig version works as expected in my manual testing.

Before:

<div class="views-display-setting views-ui-display-tab-setting odd clearfix">
  <span class="label">Display name:</span>
  <a href="/admin/structure/views/nojs/display/test/page_2/display_title" class="views-ajax-link  views-ajax-processed-processed" title="Page 2" id="views-page-2-display-title">Page 2</a>  
</div>

After:

<div class="views-display-setting views-ui-display-tab-setting odd clearfix">
  <span class="label">Display name:</span>
  <a href="/admin/structure/views/nojs/display/test/page_2/display_title" class="views-ajax-link  views-ajax-processed-processed" title="Page 2" id="views-page-2-display-title">Page 2</a>
</div>

Looks good to me. We'll want to clean up the @todos and the zebra striping as part of followup issues later.

+++ b/core/modules/views/views_ui/templates/views-ui-display-tab-setting.html.twigundefined
@@ -0,0 +1,25 @@
+ * Template for each row inside the "boxes" on the display query edit screen.

Should be default ... see twig standard.

Moved the class attributes to the preprocess, hopefully making it slightly easier to cleanup and drop the zebra stuff later.

Also fixed some documentation pieces, but not sure if I got them all correct. Please have a critical eye:)

+++ b/core/modules/views/views_ui/views_ui.theme.incundefined
@@ -15,21 +15,40 @@ function theme_views_ui_container($variables) {
-    $variables['attributes_array']['title'][] = t('Overridden');

Are you sure about this change?

@dawehner re #22 Yes, I am positive.

It's a mistake everywhere it exists:

Well I would say it's used by ViewEditFormController::buildOptionForm().

@dawehner could you point it out explicitly? I can't see anything referring to `attributes_array` in that method, building that key or using it.

See

   $display_id = $display['id'];
    if (!$view->get('executable')->displayHandlers->get($display['id'])->isDefaultDisplay()) {
      if ($view->get('executable')->displayHandlers->get($display['id'])->defaultableSections($id)) {
        $option_build['#overridden'] = TRUE;
      }
    }

@dawehner I think I see what you're are saying... I didn't remove #overridden, but I removed the unused 'title'. Maybe that title should be there and it's just a bug in Views?

Should I change it to the following?

-   $variables['attributes_array']['title'][] = t('Overridden');
+   $variables['attributes']['title'][] = t('Overridden');

OH I see, yeah use the proper key. Thank you!

#29 should do it.

#29: 1843774-29-twig-views-ui-display-tab-setting.patch queued for re-testing.

fix for #23 at #1484704: Remove instances of attributes_array needs re-roll once that get in.

#29: 1843774-29-twig-views-ui-display-tab-setting.patch queued for re-testing.

Tagging for reroll, the Views UI module got moved to core/modules in #1820414: CHANGE NOTICE: Move views_ui.module directly into /core/modules/.

Update to point to right meta issue

this one had a "added by us" merge conflict. I'm guessing the tpl file isn't needed anymore. So i created a rerolled patch without the tpl file.

patch placed the template files in the wrong folder. new patch reflects this change.

New patch needs to remove the .tpl.php template please :)

sure thing.

Code seems ok to me. Two questions:
* why do we need non-breaking spaces there?
* why do we add span tags to the separator? the 'label' class is deceiving, since it is wrapping the separator, not the label... particularly, I rather pass the separator as a clean preprocessed variable with a default value of ' | '.

@barraponto - thanks for reviewing! In my opinion those points are out of scope for a straight conversion - it's not the conversion's job to make decisions like removing   from markup. The conversions are meant to keep markup as close as possible to the pre-conversion state. Please open another issue if you'd like to change the markup. Thanks!

Sorry to nitpick but:

+ *   - link: The main setting's link.
+ *   - description: The settings description.

Should link be "The settings link" or should description be "The main setting's description" (note the inconsistent apostrophes affecting interpretation). What does the word "main" mean in this context anyway?

+ *   - description_separator: (optional) A option to append a separator colon
+ *     to the setting description.

Should be "settings description" or "setting's description", depending on the above.

+ *   - defaulted: A boolean indicating the setting is in it's default state.
+ *   - overridden: A boolean indicating the setting has been overridden from
+ *     the default.

These descriptions imply to me that (defaulted = !overidden) and nothing else. Is this really the case? it seems weird that there would be redundant variables being passed in.

+ *   - description_separator: (optional) A option to append a separator colon
+ *     to the setting description.

"An option"

Tagging for profiling.

@thedavidmeister the "main" is referring to primary so I changed to hopefully make that more clear and it's not the main setting, it's the main link in the setting.
Can't do much about the overridden!=defaulted bit, maybe you can open up an issue on that note?

* - defaulted: A boolean indicating the setting is in it's default state.

"its" not "it's". Whoever first wrote these docs got the meaning of apostrophes backwards :/ sorry that I missed this last time.

An option to append a separator colon
+ *     to the setting's description.

Is this a boolean? if it is, could we make this part of the comment consistent with the rest, eg. "A boolean indicating XXX,

#1993512: template_preprocess_views_ui_display_tab_setting() doesn't need two variables that mean the same thing. as per #47.

Was probably me, I know the difference between its and it's but my fingers refuse to obey my mind:P

code looks clean and was manually tested in #17

This one needs a reroll, likely from #1938430: Don't add a default theme hook class in template_preprocess().

Reroll

Thanks @shanethehat, back to RTBC!

Dibs on this for profiling.

Profiling results for editing frontpage view:
/admin/structure/views/view/frontpage/edit

=== 8.x..8.x compared (5198ed6acdf41..5198eec546553):

ct  : 61,422|61,422|0|0.0%
wt  : 317,151|316,951|-200|-0.1%
cpu : 277,465|277,365|-100|-0.0%
mu  : 16,452,000|16,452,000|0|0.0%
pmu : 16,710,184|16,710,184|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5198ed6acdf41&...

=== 8.x..views-ui-display-tab-setting-1843774-53 compared (5198ed6acdf41..5198eef80c6dc):

ct  : 61,422|62,574|1,152|1.9%
wt  : 317,151|318,977|1,826|0.6%
cpu : 277,465|281,875|4,410|1.6%
mu  : 16,452,000|16,477,224|25,224|0.2%
pmu : 16,710,184|16,742,272|32,088|0.2%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5198ed6acdf41&...

Updated issue summary.

Being that the this is view ui conversion I think the performance regression here is okay.

+1. Ready for #1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch

Add profiling results

Committed f202b56 and pushed to 8.x. Thanks!