The views UI should display "All Displays" option only when there are more 1 displays.

Let's do that, unless the user wants to see the master display all the time.

A side from the clear visual bug, which seems to be from another issue. Just hiding this functionality when there are no other displays, or master display sounds sensible.

Oh right, i worked on two issues at the same time.

For clarity, when you have hidden this field, the button will be called "Apply" all the time.

Let's fix the test.

It would be nice to have a possibility to show this option even if there is just one display (probably admin/structure/views/settings is a right place for appropriate switch). It is useful in case you know other displays will be added in the future, using it you can decide what should be cloned to this displays, and what should not.

In principle it should be possible with Master display, though... Would be interesting to hear a word from other people about their workflows in such cases.

It would be nice to have a possibility to show this option even if there is just one display (probably admin/structure/views/settings is a right place for appropriate switch). It is useful in case you know other displays will be added in the future, using it you can decide what should be cloned to this displays, and what should not.

Another classical example that you can't oversimplify what people are doing with views.

#5: drupal-1836384-5.patch queued for re-testing.

#5: drupal-1836384-5.patch queued for re-testing.

#5: drupal-1836384-5.patch queued for re-testing.

#5: drupal-1836384-5.patch queued for re-testing.

tagging

rerolled, looks good and works fine.

@damain could you reroll this?

Yep, sure.

Patch applied ok, this should fix the test failure.

Rerolled

Now we just have to fix the failures :)

Rerolled

Trying to pass.

Needs review.

Removing location tag -- there was a discussion (see 2407325#comment-9513533 and decided to use one tag to keep the autocomplete list of tags short.

Needs review.

+++ b/core/modules/views_ui/admin.inc
@@ -312,6 +312,13 @@ function views_ui_standard_display_dropdown(&$form, FormStateInterface $form_sta
+  }

You could show that in just a single line, right? '#access'] = !\Drupal::config('views.settings')->get('ui.show.master_display') && count($displays) > 2;

Patch applies.

@dawehner, isn't that an if statement?

I agree with @tadiyar, it's an IF statement so it should be in curly brackets.

Could I add one more twist to this? Only remove the field when the section is not currently overridden.

Scenario:
- Add View
- Add two Pages
- Override the Field section in Page 2
- Delete Page 1

With the patch in #36 the user would now NOT get the option to revert the Field section in Page 2 back to default (because there is only one display left).

So add an additional check to see if the current section in overridden, and if so. don't hide the field.

The failed tests show another issue with the fix in #36. Setting #access to FALSE actually translates to 'overridden' and not 'default'.
getOverrideValues() only checks if a value is set, and doesn't check #access. So when adding anything in the UI you will end up with a section that no longer uses the defaults.

The new patch does the check earlier and doesn't add the override field at all, like it is done for the default display.

Interdiff is against #36

Let's see what this does...

A light re-roll from #42...

@pguillard Oh cool, thank you for rerolling this patch. I

+++ b/core/modules/views/src/Tests/Plugin/PagerTest.php
index 6170ed4..fd0434f 100644
--- a/core/modules/views_ui/admin.inc

--- a/core/modules/views_ui/admin.inc
+++ b/core/modules/views_ui/admin.inc

+++ b/core/modules/views_ui/admin.inc
+++ b/core/modules/views_ui/admin.inc
@@ -230,7 +230,11 @@ function views_ui_standard_display_dropdown(&$form, FormStateInterface $form_sta

@@ -230,7 +230,11 @@ function views_ui_standard_display_dropdown(&$form, FormStateInterface $form_sta
     $form['progress']['#weight'] = -1001;
   }

I think we need some additional testing for the new behaviour. Maybe in \Drupal\views_ui\Tests\DisplayTest we already have some tests about the dropdown

Cool

I guess we have dropdown tests :

// Test that the override element is only displayed on pager plugin selection form.
$this->drupalGet('admin/structure/views/nojs/display/test_store_pager_settings/page_1/pager');
$this->assertFieldByName('override[dropdown]', 'page_1', 'The override element is displayed on plugin selection form.');
$this->drupalGet('admin/structure/views/nojs/display/test_store_pager_settings/page_1/pager_options');
$this->assertNoFieldByName('override[dropdown]', NULL, 'The override element is not displayed on plugin settings form.');

I just corrected one test, I don't know for the remaining error!

to not consider..

+++ b/core/modules/views/src/Tests/Plugin/PagerTest.php
@@ -71,7 +71,7 @@
-    $this->drupalPostForm('admin/structure/views/nojs/display/test_store_pager_settings/default/pager', $edit, t('Apply'));
+    $this->drupalPostForm('admin/structure/views/nojs/display/test_view/default/pager', $edit, t('Apply'));

The change in the interdiff in #49 doesn't appear to be in the patch? Am I missing something?

@Lendude : You are right, that line was braking the tests, it was mentioning test_store_pager_settings before we get the view.
Feel free to take control!

This should get it back into the green. Will look into adding more tests per #48.

I guess this is RTBC !

Back to RTBC...

I might be wrong but I can't see any tests confirming that the "all Displays" option is not displayed as expected.

@alexpott you are absolutely right.

Added tests and removed unrelated extra newline.

I guess this is RTBC.

Before patch :

After patch :

+1

Committed c536cbf and pushed to 8.1.x and 8.2.x - as a task this is not eligible for 8.0.x but this is safe for 8.1.x-beta1. Thanks!

Thanks, removing from UX sprint now.