Problem/Motivation

When new displays are created in the Views UI, either using the add display button or the "clone as" feature, they initially appear in the correct order (newest last). However, as soon as the view is saved, the displays are reordered. Perhaps the position isn't being set for new displays?

Steps to reproduce

  1. I started with 8.x standard:
    standard_install.png

  2. I added some new block displays.
    adding_a_display.png

  3. Each time, the unsaved display appeared in the correct position in the UI, and opening the modal dialog (without making any changes) would also show the correct order.
    appear_in_correct_order.pngno_reorder.png

  4. But after saving the view (without having done anything other than add a new display) the displays would be reordered, with the newest floating to the top.
    after_saving.pngafter_saving_reorder_dialog.png

    Positions in the saved config entity at the same time (note that the order here bears no relationship to the order in the UI, despite that it is exactly the view being used):

    display:
      block_3:
        position: ''
      feed_1:
        position: ''
      block_1:
        position: ''
      page_1:
        position: {  }
      block_2:
        position: {  }
      default:
        position: {  }
      block_4:
        position: ''

    It appears that, unless you use the reorder dialog, position is stored as an empty string.

  5. If I use the reorder dialog, non-empty weights are saved, but the displays are still listed in the wrong order:

    display:
      block_4:
        position: '6'
      block_3:
        position: '5'
      block_1:
        position: '2'
      feed_1:
        position: '3'
      default:
        position: '0'
      page_1:
        position: '1'
      block_2:
        position: '4'
  6. And, if I add yet another display to the view, it still floats to the top after saving.
    too_many_displays.png
    This time, the displays were finally reordered by position in the file, only after saving twice after the reorder:

    display:
      block_5:
        position: ''
      default:
        position: '0'
      page_1:
        position: '1'
      block_1:
        position: '2'
      feed_1:
        position: '3'
      block_2:
        position: '4'
      block_3:
        position: '5'
      block_4:
        position: '6'

Proposed resolution

The underlying problem appears to be that new displays are created with an empty position, which also results in haphazard file ordering on save. Maybe newly created displays should always have the weight of last visible display + 1?

Files: 
CommentFileSizeAuthor
#9 drupal-1968596-9.patch3.59 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 56,111 pass(es).
[ View ]
#9 interdiff.txt2.11 KBdawehner
#7 1968596-7.patch1.91 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 55,727 pass(es).
[ View ]
#6 drupal-1968596-6.patch1.2 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 54,364 pass(es).
[ View ]
#2 appear_in_correct_order.png7.57 KBxjm
too_many_displays.png7.72 KBxjm
after_saving_reorder_dialog.png26.89 KBxjm
after_saving.png7.22 KBxjm
no_reorder.png26.97 KBxjm
adding_a_display.png26.19 KBxjm
standard_install.png22.44 KBxjm

Comments

xjm’s picture

Title:New displays are reordered on saving.» New displays are not ordered correctly
xjm’s picture

StatusFileSize
new7.57 KB
xjm’s picture

xjm’s picture

Issue summary:View changes

Updated issue summary.

xjm’s picture

Issue summary:View changes

Updated issue summary.

xjm’s picture

Issue summary:View changes

Updated issue summary.

xjm’s picture

Issue summary:View changes

Updated issue summary.

xjm’s picture

Issue summary:View changes

Updated issue summary.

xjm’s picture

damiankloip’s picture

Yep, I think that issue will complete half of the puzzle. When the form is used to reorder, that will fix it. In here we probably just need to fix the fact that the default order is saved as an array if it doesn't have one. We need to give it one. I think it's the config system to blame - I have a feeling a NULL value somehow gets saved as an array.

dawehner’s picture

Status:Active» Needs review
StatusFileSize
new1.2 KB
PASSED: [[SimpleTest]]: [MySQL] 54,364 pass(es).
[ View ]

I think the addDisplay method should just set a proper default value. That's all what we need.

This count() method should do it, but we could also make it a bit more secure, by finding the max position. Any thoughts?

damiankloip’s picture

StatusFileSize
new1.91 KB
PASSED: [[SimpleTest]]: [MySQL] 55,727 pass(es).
[ View ]

I think we just need to also add this weight to the tabs when they are rendered, then the new weights will be reflected in the UI straight away. As the other ordering will only be invoked during save.

We need to wait on #1968020: Convert ReorderDisplays to use table rendering and remove theme function before you can actually re order displays.

damiankloip’s picture

Issue tags:+Needs tests
dawehner’s picture

StatusFileSize
new2.11 KB
new3.59 KB
PASSED: [[SimpleTest]]: [MySQL] 56,111 pass(es).
[ View ]

Let's do it.

damiankloip’s picture

Status:Needs review» Reviewed & tested by the community

New tests look good to me.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed 5dff3de and pushed to 8.x. Thanks!

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

Anonymous’s picture

Issue summary:View changes

Updated issue summary.