Sort views displays by display name

The failures are a little bit more tricky than expected.

There we go.

1. +++ b/core/modules/views/src/Entity/View.php
@@ -301,6 +301,11 @@ public function calculateDependencies() {
+    ksort($display);

   This I am ok with. The position is more a UI thing anyway.

2. +++ b/core/modules/views_ui/src/Tests/DisplayCRUDTest.php
   @@ -33,7 +33,7 @@ class DisplayCRUDTest extends UITestBase {
   +  public function ptestAddDisplay() {

   @@ -62,7 +62,7 @@ public function testAddDisplay() {
   +  public function ptestRemoveDisplay() {

   @@ -105,7 +105,7 @@ public function testDefaultDisplay() {
   +  public function ptestDuplicateDisplay() {

   Oops

3. +++ b/core/modules/views_ui/src/ViewFormBase.php
@@ -47,6 +47,21 @@ protected function prepareEntity() {
       if (empty($this->displayID)) {
+
+        uksort($tabs, function($a, $b) {

   Isn't a #weight property already added in getDisplaytabs()? Also, the hardcoding of page, block seems a bit meh.

Isn't a #weight property already added in getDisplaytabs()? Also, the hardcoding of page, block seems a bit meh.

Well, this is sadly a bit tricky to be honest. We do sort the displays with the following code:

<?php
      $tabs[$id] = array(
        '#theme' => 'menu_local_task',
        '#weight' => $display['position'],
        '#link' => array(
?>

which means that the order of the tabs is (as you would expect) caused by the priority managed in the UI.
Meh I don't have a better suggestion for that at the moment but hardcoding here feels better than the clusterfuck of moving displays around all the time.

Oops

Where is the problem ;)

Based on discussion on last week's CMI meeting, I opened #2361539: Config export key order is not predictable, use config schema to order keys on all levels.

Unfortunately #2361539: Config export key order is not predictable, use config schema to order keys on all levels probably won't help here since displays is a sequence. Unless decide to have a new rule that all sequences are key sorted. Which would not be so silly imho since what we could then say is mappings are sorting according to schema, associated arrays (sequences) are key sorted and indexed arrays (sequences too) are saved in the order provided.

The one issue with this idea is code like this where we want the default to always come first.

Setting back to needs work because we totally needs tests for this type of change cause it is so easy to break.

#2361539: Config export key order is not predictable, use config schema to order keys on all levels would only help if we decide to introduce something like an "order_by_value_of_key: id" or something along those lines, where we would provide the name of key to take the value to order the sequences by. That may or may not be a good idea or in scope there. I would not postpone this issue on that.

Adding views UI test.

Thanks @webflo. looks good. here is the test-only patch for #19 to prove it fails.

/me saved the patch as 2350821-view-display-order-19-test-only.php. May be bed time :)

The other implementation indeed looks a bit nicer.

1. +++ b/core/modules/views_ui/src/Tests/DisplayOrderTest.php
@@ -0,0 +1,70 @@
+
+    $this->assertNoLink('Master*', 0, 'Make sure the master display is not marked as changed.');

   Huch, I am confused, don't we hide the master display by default, so that there is also no 'Master' on that page? The text feels a bit misleading.

2. +++ b/core/modules/views_ui/src/Tests/DisplayOrderTest.php
@@ -0,0 +1,70 @@
+    // Add a new block display.
+    $this->drupalPostForm(NULL, array(), 'Add Block');
+    $this->assertLinkByHref($path_prefix . '/block_1', 0, 'Make sure after adding a display the new display appears in the UI');
+    $this->drupalPostForm(NULL, array(), t('Save'));
+
+    // Test that the new view displays are in the correct order.
+    $view = Views::getView($view['id']);
+    $displays = $view->storage->get('display');
+    $expected_displays = array('default', 'block_1', 'page_1');
+    $this->assertEqual(array_keys($displays), $expected_displays, 'The display names are in correct order.');
+  }

   Should we also explicit check the order in the UI instead of just the config?

This is a bug as can be seen from the patch attached to #2316909-17: Revisit all built-in test/default views configuration in core - the orders of things shouldn't change just because we save them. Also this kind of blocks progress on that issue so bumping priority.

• Ignored point 1), this was BS
• Added a dedicated UI test for point 2)

How come we're not sorting by weight in the UI?

I've just postponed #2316909: Revisit all built-in test/default views configuration in core on this - so this is now critical

@alexpott

Well, you know, people came up with that behaviour in order to improve UX in d7.
The page display is considered as more important than a block display, especially because on admin/structure/views/add
you see the same kind of order.

1. +++ b/core/modules/views_ui/src/ViewFormBase.php
@@ -77,6 +78,36 @@ protected function prepareEntity() {
+    $default = array_filter(array_keys($tabs), function ($value) {
+      return (strpos($value, 'default') === 0) ? TRUE : FALSE;
+    });

   This is unnecessary since the default display is only shown when it is the only display.

2. +++ b/core/modules/views_ui/src/ViewFormBase.php
@@ -77,6 +78,36 @@ protected function prepareEntity() {
+    $page = array_filter(array_keys($tabs), function ($value) {
+      return (strpos($value, 'page') === 0) ? TRUE : FALSE;
+    });
+
+    $block = array_filter(array_keys($tabs), function ($value) {
+      return (strpos($value, 'block') === 0) ? TRUE : FALSE;
+    });

   This functionality appears untested since we add page_1 and block_1 in the test. If a view did have view displays with these ID then it would break the ability to reorder displays in the UI.

Basically I don't see the point of ViewFormBase::sortDisplayTabs() or the DisplayOrderTest test since views display re-ordering is tested in DisplayTest::testReorderDisplay()

So #29.1 is wrong since this is configurable. but there is already code preventing you from re-ordering that so I still stand behind my statement that I don't see the need for the test or the code.

What is the problem with storing the displays in the same order as they appear in UI?

@olli because then when all you do is re-order them you get a massive diff when all that has changed is the weight.

So manual testing has proved that the master display always comes first in the UI and users can re-order to whatever they like and this is tested. So this patch should just concentrate on the order that displays are saved.

Patch attached does that.

Ok we need to sort the tabs by weight to choose the correct one when no display is set.

Thank you @alexpott!!

Alright, Page still appears before block +1

Committed/pushed to 8.0.x, thanks!

#33: Ok, thanks!

I think it might be a good idea to add a method somewhere which you can use to get a list of view displays ordered by position (the same order as display tabs in UI). That could be used in views ui for "attach to", "link display" and in view area plugin.