Updated: Comment #38

Problem/Motivation

When multiple toolkits are available for selection in the Image toolkit form at admin/config/media/image-toolkit, all the detail setup of each toolkit is presented. This is regression from Drupal 7 where only the selected toolkit's details are presented.

Proposed resolution

Make visible only the details of the currently selected toolkit, and use the #states form property to make visible alternative toolkit settings upon change of the selection.

Remaining tasks

  • Review patch.

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mondrake’s picture

FileSize
765 bytes

Patch.

mondrake’s picture

Status: Active » Needs review
claudiu.cristea’s picture

Hm. Pure JS or should we try Ajax like in admin/config/content/formats/manage/basic_html (see WYSIWYG select)?

mondrake’s picture

We can also do AJAX, even though I wonder if it is worth it (how many toolkits would a site install?). Let me know.

claudiu.cristea’s picture

Not necessary the number of toolkits but the details section will expand in #2110591: API/UI for toolkit operations plugin selection. As I see, now, inside toolkit details we'll have another details sections holding a list of all effects a toolkit is implementing. Where there is a competition (same operation & toolkit) there will be a select.

mondrake’s picture

FileSize
3.48 KB

OK. Attached an AJAX version.

claudiu.cristea’s picture

Great!

@mondrake, I would like to play a little bit and test this manually. Do you have some alternative toolkit module just for purpose of this issue?

mondrake’s picture

I played a bit around a conversion of Imagemagick. It falls in the pitfalls we are highlighting in #2105863: [meta] Images, toolkits and operations, but it can do for the purpose of testing this issue.

You can pull the from the sandbox here.

claudiu.cristea’s picture

EDIT: Removed an initial comment.

+++ b/core/modules/system/lib/Drupal/system/Form/ImageToolkitForm.php
@@ -65,34 +67,47 @@ public function getFormID() {
+  public function processAjax($form, $form_state) {

Maybe processToolkitDetails()?

Otherwise looks good :)

mondrake’s picture

FileSize
3.5 KB
1.02 KB

OK. Here we go.

claudiu.cristea’s picture

+++ b/core/modules/system/lib/Drupal/system/Form/ImageToolkitForm.php
@@ -65,34 +67,47 @@ public function getFormId() {
+   * AJAX callback.

I know I'm a little bit late (sorry!), but this seems to deserve a better doc/description.

mondrake’s picture

FileSize
3.54 KB
599 bytes

Like this?

claudiu.cristea’s picture

Status: Needs review » Reviewed & tested by the community

Thank you. Looks good. RTBC if turns green.

alexpott’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Looks like we have 0 test coverage of this form

mondrake’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
2.88 KB
6.42 KB

Added tests. In fact this was good as system.routing.yml was setting a non-existing 'administer administration pages' permission for this route. Corrected to 'access administration pages'.

mondrake’s picture

Issue summary: View changes
mondrake’s picture

Noticed a few glitches.

claudiu.cristea’s picture

Thank you for your work.

  1. +++ b/core/modules/system/lib/Drupal/system/Form/ImageToolkitForm.php
    @@ -65,34 +67,47 @@ public function getFormId() {
    +    // Build options and load the settings forms of the current toolkit.
    

    Maybe 'settings form' (singular)?

  2. +++ b/core/modules/system/lib/Drupal/system/Form/ImageToolkitForm.php
    @@ -65,34 +67,47 @@ public function getFormId() {
    -        '#type' => 'fieldset',
    

    I think we should use '#type' => 'details' here. See https://drupal.org/node/1898824.

  3. +++ b/core/modules/system/lib/Drupal/system/Form/ImageToolkitForm.php
    @@ -65,34 +67,47 @@ public function getFormId() {
    +          '#title' => t('@toolkit settings', array('@toolkit' => $definition['title'])),
    

    $this->t() instead t().

  4. +++ b/core/modules/system/lib/Drupal/system/Tests/Image/ToolkitSetupFormTest.php
    @@ -0,0 +1,84 @@
    + * Definition of Drupal\system\Tests\Image\ToolkitSetupFormTest.
    

    "Contains \Drupal\system...". Leading backslash.

  5. +++ b/core/modules/system/lib/Drupal/system/Tests/Image/ToolkitSetupFormTest.php
    @@ -0,0 +1,84 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  protected $profile = 'testing';
    

    Always provided by parent class. Remove it.

  6. +++ b/core/modules/system/lib/Drupal/system/Tests/Image/ToolkitSetupFormTest.php
    @@ -0,0 +1,84 @@
    +    $this->assertResponse(200, 'The toolkit setup form was found.');
    

    This is no needed. The previous $this->drupalGet() is already checking for 200. See the test messages. Should be removed.

  7. +++ b/core/modules/system/lib/Drupal/system/Tests/Image/ToolkitSetupFormTest.php
    @@ -0,0 +1,84 @@
    +    $edit = array(
    +      'image_toolkit' => 'test',
    +    );
    

    Single line? https://drupal.org/coding-standards#array

  8. +++ b/core/modules/system/lib/Drupal/system/Tests/Image/ToolkitSetupFormTest.php
    @@ -0,0 +1,84 @@
    +    $this->drupalPostAjaxForm(NULL, $edit, 'image_toolkit');
    +    $this->drupalPostForm(NULL, $edit, 'Save configuration');
    

    It would be nice here if we can add some fake configs also to the 'test' toolkit. Then change the configs and test here to see if AJAX really worked. I mean if the changed configs get saved if they were exposed as a result of an AJAX load.

    Also drop the message from $this->assertEqual(). We should not use custom messages in assertions.

mondrake’s picture

FileSize
8.39 KB
6.47 KB

Thanks for review @claudiu.cristea

All done. In addition, added a warning message to the user to remind clicking on 'Save configuration' after a toolkit change in the form. The change processed by AJAX is not leading to a change in the config, it must be confirmed. With no message, users can be misled to believe the change is already effective. Similar behavior we have e.g. in the views UI.

claudiu.cristea’s picture

Status: Needs review » Reviewed & tested by the community

Good to go.

mondrake’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
8.4 KB
mondrake’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

alexpott’s picture

Aren't we doing js enhancement wrong here - shouldn't we use it to hide the forms for the unselected toolkits rather cause users without js all the extra page loads. I.e. shouldn't this work in the same way as the database selection form in the installer?

claudiu.cristea’s picture

The 'details' part of the form for each toolkit will expand in the future. See #5. To be onest, I don't know exactly where is the limit between pure JS (aka 'hide') and Ajax. Anyway we want to add more per-toolkit elements in the this form.

webchick’s picture

Assigned: Unassigned » alexpott

Back to alexpott, though not sure that his concern is addressed.

xjm’s picture

21: 2111263-21.patch queued for re-testing.

claudiu.cristea’s picture

Assigned: alexpott » Unassigned
Status: Reviewed & tested by the community » Needs work

Just had a discussion with @alexpott on IRC related to this issue. We agreed to move back to JS form states as it was posted initially in #1. Sorry for all the work caused by my comment from #3.

@mondrake, do you think you can rework #21 with form states? Thank you!

mondrake’s picture

Sure, no problem. We'll have to rethink how to process getRequirements() upon form validation though. But OK, one step at a time.

mondrake’s picture

Status: Needs work » Needs review
FileSize
4.52 KB
6.72 KB

Here we go.

Status: Needs review » Needs work

The last submitted patch, 29: 2111263-28.patch, failed testing.

The last submitted patch, 29: 2111263-28.patch, failed testing.

mondrake’s picture

Status: Needs work » Needs review

29: 2111263-28.patch queued for re-testing.

fietserwin’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/lib/Drupal/system/Form/ImageToolkitForm.php
@@ -69,22 +69,27 @@ public function buildForm(array $form, array &$form_state) {
+        '#collapsible' => FALSE,
+        '#collapsed' => FALSE,

Remove (and rename), see issue(s) #1852104: #type details: Remove #collapsible property (and #1892182: #type details: Rename #collapsed to #open) (or change the #type back to fieldset).

mondrake’s picture

I have updated the sandbox with the halfway conversion of the Imagemagick toolkit (see comment #8), to manually test the patch in #29. Just for info if anyone wants to test.

@fietserwin - re. #33: thanks, now I understand why setting #collapsible is totally irrelevant...

However, we have some contradictions here:

So what shall we do? My proposal is:

  • stay with 'details', HTML5 std
  • remove #collapsible
  • leave #collapsed to straight FALSE (the current logic of making #collapsed = TRUE the non-default toolkits setting form portions is fugly with states...)
  • leave conversion of #collapsed to #open to #1892182: #type details: Rename #collapsed to #open or a follow-up.

@fietserwin, @claudiu.cristea can I have your opinions before coding this?

claudiu.cristea’s picture

@mondrake,

stay with 'details', HTML5 std
remove #collapsible
leave #collapsed to straight FALSE (the current logic of making #collapsed = TRUE the non-default toolkits setting form portions is fugly with states...)
leave conversion of #collapsed to #open to #1892182: #type details: Rename #collapsed to #open. NOT FOLLOWUP.

OK with all. Maybe #1892182: #type details: Rename #collapsed to #open will need a comment & "needs work" after this lands.

mondrake’s picture

Ok. Tomorrow.

fietserwin’s picture

I'm fine with the proposal of #34.

Note: I tested this manually by editing the image_test.info.yml file and setting hidden to false, then enabling the image_test module.

mondrake’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
6.69 KB
692 bytes

Implements #34. The latest patch from #1892182: #type details: Rename #collapsed to #open already was doing a conversion of #collapsed to #open in ImageToolkitForm anyway, so I don't think we need to do anything more about it.

Updated issue summary to reflect we reverted from AJAX to JS states.

claudiu.cristea’s picture

Status: Needs review » Reviewed & tested by the community

Good to go if turns green.

Thank you!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 35d043d and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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