Similar to #1929064: [Change notice] Refactor views_ui_rearrange_form to remove theme function for table render, we don't really need a theme function anymore. That issue was a 'test the water' type of thing. So we can go ahead and convert this too.

Patch to follow.

Files: 
CommentFileSizeAuthor
#25 drupal-1968020-25.patch11 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 55,527 pass(es).
[ View ]
#25 interdiff.txt3.8 KBdawehner
#24 1968020-24.patch10.7 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 55,381 pass(es).
[ View ]
#24 interdiff-1968020-24.txt871 bytesdamiankloip
#22 1968020-22.patch10.54 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 55,466 pass(es), 2 fail(s), and 2 exception(s).
[ View ]
#18 1968020-18.patch10.27 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 55,955 pass(es).
[ View ]
#18 interdiff-1968020-18.txt825 bytesdamiankloip
#16 1968020-16.patch10.29 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 55,897 pass(es).
[ View ]
#16 interdiff-1968020-16.txt988 bytesdamiankloip
#14 1968020-14.patch10.3 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 55,405 pass(es).
[ View ]
#13 1968020-12.patch10.37 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 54,473 pass(es).
[ View ]
#13 interdiff-1968020-12.txt2.18 KBdamiankloip
#10 1968020-9.patch8.55 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 54,365 pass(es), 7 fail(s), and 0 exception(s).
[ View ]
#10 interdiff-1968020-9.txt797 bytesdamiankloip
#7 1968020-6.patch8.55 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 54,305 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
#3 1968020-3.patch8.52 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1968020-3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#2 1968020.patch7.96 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 54,308 pass(es), 7 fail(s), and 0 exception(s).
[ View ]

Comments

xjm’s picture

Related: #1968596: New displays are not ordered correctly. @damiankloip said in IRC that this issue will probably solve that one.

damiankloip’s picture

Status:Active» Needs review
StatusFileSize
new7.96 KB
FAILED: [[SimpleTest]]: [MySQL] 54,308 pass(es), 7 fail(s), and 0 exception(s).
[ View ]
damiankloip’s picture

StatusFileSize
new8.52 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1968020-3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Oops, we can remove the declaration in views_ui_theme too.

Status:Needs review» Needs work
Issue tags:-VDC

The last submitted patch, 1968020-3.patch, failed testing.

damiankloip’s picture

Status:Needs work» Needs review

#3: 1968020-3.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+VDC

The last submitted patch, 1968020-3.patch, failed testing.

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new8.55 KB
FAILED: [[SimpleTest]]: [MySQL] 54,305 pass(es), 8 fail(s), and 0 exception(s).
[ View ]

Doh, rerolled after #1963976: Remove theme_views_tab() and theme_views_tabset() declarations from views_ui_theme(). All because it added a comment in that it shouldn't have! :)

xjm’s picture

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/Form/Ajax/ReorderDisplays.phpundefined
@@ -35,57 +35,90 @@ public function buildForm(array $form, array &$form_state) {
+    $form['#title'] = t('Displays Reorder');

This is sort of out of scope, but I was about to file a separate issue for it that will just collide with this: Can we change this title to "Reorder displays" (sentence case)?

xjm’s picture

Status:Needs review» Needs work
damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new797 bytes
new8.55 KB
FAILED: [[SimpleTest]]: [MySQL] 54,365 pass(es), 7 fail(s), and 0 exception(s).
[ View ]

I think that's totally in scope, we are ripping the whole form apart anyway.

xjm’s picture

Actually, it appears that #9 also happens in HEAD, if there are no new displays being created. If you save the view with the patch, though, the displays are in the correct order once you go back to edit a second time. So basically there are like four kinds of broken with display ordering currently, and we can solve those other bugs in the other issue.

Status:Needs review» Needs work

The last submitted patch, 1968020-9.patch, failed testing.

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new2.18 KB
new10.37 KB
PASSED: [[SimpleTest]]: [MySQL] 54,473 pass(es).
[ View ]

With the fixed form, the logic doesn't hold together in submit. I think now it all works properly it should do something more like this. This should fix the tests.

I cleaned up the submit a little while I was there, and moved the increment on $position, so it's easier to read in the code.

damiankloip’s picture

Assigned:damiankloip» Unassigned
StatusFileSize
new10.3 KB
PASSED: [[SimpleTest]]: [MySQL] 55,405 pass(es).
[ View ]
dawehner’s picture

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/Form/Ajax/ReorderDisplays.phpundefined
@@ -143,26 +143,29 @@ public function submitForm(array &$form, array &$form_state) {
-      $order[$display] = $position++;
+      $order[$display] = $position;
+      $position++;

Isn't the code doing exactly the same as before? Why bother with adding a new line.

+++ b/core/modules/views_ui/lib/Drupal/views_ui/Form/Ajax/ReorderDisplays.phpundefined
@@ -35,57 +35,90 @@ public function buildForm(array $form, array &$form_state) {
+      '#empty' => t('No displays available.'),

You must be really good, if you manage to have no displays at all :)

+++ b/core/modules/views_ui/lib/Drupal/views_ui/Form/Ajax/ReorderDisplays.phpundefined
@@ -35,57 +35,90 @@ public function buildForm(array $form, array &$form_state) {
+        $form['displays'][$id]['#attributes']['style'] = 'display: none;';

You can use the element-hidden class.

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/Form/Ajax/ReorderDisplays.phpundefined
@@ -143,26 +143,29 @@ public function submitForm(array &$form, array &$form_state) {
-      $order[$display] = $position++;
+      $order[$display] = $position;
+      $position++;

Isn't the code doing exactly the same as before? Why bother with adding a new line.

damiankloip’s picture

StatusFileSize
new988 bytes
new10.29 KB
PASSED: [[SimpleTest]]: [MySQL] 55,897 pass(es).
[ View ]

Thanks for the review!

Isn't the code doing exactly the same as before? Why bother with adding a new line.

Yep, exactly the same. I just thought it would make it easier to read if the code explicitly used $position then incremented. Otherwise it may not be obvious (or people may forget) that the variable will be used, then incremented. I guess it's just a personal preference :) I have changed it back though.

You never know, someone might manage something with no displays, that should not be possible, and should break before that anyway. I think it might as well stay there.

dawehner’s picture

I really love to kill all these totally useless theme functions! That's a great patch.

You never know, someone might manage something with no displays, that should not be possible, and should break before that anyway. I think it might as well stay there.

Oh I absolutely agree. It should stay there and is it just because of consistency.

+++ b/core/modules/views_ui/lib/Drupal/views_ui/Form/Ajax/ReorderDisplays.phpundefined
@@ -35,57 +35,91 @@ public function buildForm(array $form, array &$form_state) {
     foreach ($displays as $display) {
...
+      $id = $display['id'];

What about just using foreach ( ... $id => $display)

+++ b/core/modules/views_ui/lib/Drupal/views_ui/Form/Ajax/ReorderDisplays.phpundefined
@@ -35,57 +35,91 @@ public function buildForm(array $form, array &$form_state) {
+          'class' => array(),

Just wondering whether it is necessary to set an empty class here.

damiankloip’s picture

StatusFileSize
new825 bytes
new10.27 KB
PASSED: [[SimpleTest]]: [MySQL] 55,955 pass(es).
[ View ]

Thanks again. I made those changes too.

dawehner’s picture

I just had a manual try and realized that the remove link is not working. What can we do about it?

Aside from that, do you think we should hide the link for NON-js users? The link will not work for them anyway.

damiankloip’s picture

Well I think the remove link doesn't work currently before this patch, and neither do any of the remove links actually. So I think that should be another issue.

We could hide the link for non js users, as they will just have the checkbox anyway. What do you think, hide the link and make it visible with js? I will take a look at adding that.

dawehner’s picture

We could hide the link for non js users, as they will just have the checkbox anyway. What do you think, hide the link and make it visible with js? I will take a look at adding that.

Yeah that's what I meant. 'js-show' is the needed css class for that.

Well I think the remove link doesn't work currently before this patch, and neither do any of the remove links actually. So I think that should be another issue.

I had in mind that changing this to a proper build array would maybe solve the loading of the javascript, but well maybe it's just a bug in the ajax system
which will be solved over time.

damiankloip’s picture

StatusFileSize
new10.54 KB
FAILED: [[SimpleTest]]: [MySQL] 55,466 pass(es), 2 fail(s), and 2 exception(s).
[ View ]

Yeah that's what I meant. 'js-show' is the needed css class for that.

Alas, if we add this now the link will disappear because the js isn't working properly for the modal. There is already an issue that will fix this though, right? I remember we had the same discussion in another issue.

I have changed the link to use a build array, that is much better!

Status:Needs review» Needs work

The last submitted patch, 1968020-22.patch, failed testing.

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new871 bytes
new10.7 KB
PASSED: [[SimpleTest]]: [MySQL] 55,381 pass(es).
[ View ]
dawehner’s picture

StatusFileSize
new3.8 KB
new11 KB
PASSED: [[SimpleTest]]: [MySQL] 55,527 pass(es).
[ View ]

Manually using the patch works pretty fine, but there are just a few minor things in the code (points etc.).

dawehner’s picture

Removing and reordering works fine, though there is a unrelated problem that the reorder just happens on a save on the actual views page (but this is unrelated for now).

+++ b/core/modules/views_ui/views_ui.moduleundefined
@@ -135,6 +135,12 @@
+    // Reordering displays.
+    'views_ui_reorder_displays_form' => array(
+      'render element' => 'form',
+      'file' => 'views_ui.theme.inc',
+    ),
+
     // On behalf of a plugin
     'views_ui_style_plugin_table' => array(
       'render element' => 'form',
@@ -144,12 +150,6 @@

@@ -144,12 +150,6 @@
       'file' => 'views_ui.theme.inc',
     ),

-    // Reordering displays.
-    'views_ui_reorder_displays_form' => array(
-      'render element' => 'form',
-      'file' => 'views_ui.theme.inc',
-    ),
-

The idea was to actually remove the hook_theme entry.

damiankloip’s picture

Status:Needs review» Reviewed & tested by the community

ok, let's do that. We have manually tested this today and things are looking good.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed d556b32 and pushed to 8.x. Thanks!

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