Clean up search settings page

try

These suggestions seem fine, can you post a before/after that works :')?

Search module is often a bit quite, so if you can get another 1/2 reviews on this its RTBC from my POV.

This patch no doubt needs a reroll at this point.

Also I found a related issue that has some wording suggestions:
#678656: Configuration >> Search and Metatdata

Here's another one that I think this patch or at least the images in the issue summary addresses:
#1715366: Disabling the default search module breaks core search
(it's about making sure the default search module is actually enabled)

I am working on rerolling this patch, which doesn't apply at all because the search settings page has been made into a Form class. Meanwhile, here is a "Before" screen shot of the current search settings screen.

OK, here's a rerolled patch, with a few updates. An interdiff is not possible - the previous patch was on search.admin.inc before search was converted to a plugin system, and this is in SearchSettingsForm.php after search started using plugins. So the code is fairly different.

But the effect is pretty much what the previous patch was doing. I did make a couple of minor wording and cosmetic changes, such as moving the status and reindex button to the bottom of the Indexing section, which to me made more sense. And I wasn't sure about the code that did the radios for the default plugin, so I pretty much left that part alone from what it is currently in D8 HEAD.

An "After" screen shot is attached; the Before screen shot is in #9.

Hopefully I didn't screw anything up... it seems to be OK though, and I assume the testbot will tell us if I made mistakes.

s

add issue summary

I just updated the issue summary. By the way, I think the reason those images were not showing up correctly is that they had spaces in the file names.

I don't think this is a huge problem. The two settings are right next to each other, and if someone decides to select a default that isn't one of their active modules, ... it's obviously a mistake, and I don't think we should try to outsmart them by figuring out whether they really wanted to activate that module or really meant to make a different selection. Throwing a form validation error seems like the correct solution to me.

Beyond me... really I don't think this is too much of an issue. The form error is clear, and it's easy to fix. But if you want to take the patch I made and add a table with form states and a test for it, that is fine. I think this is OK as it is and that is more trouble than it is worth for a settings page that most likely people will access once when setting up a site and then never again.

In D7 didn't we set the module to be active if you picked it as default? That would seem the better UX.

I am not sure I agree with #16. If you picked a module as default and didn't pick it as active, that's a mistake. I think the UI should not assume it knows which part was a mistake.

I also looked back at the code for D7 and we were doing the same thing there as what is in the current patch -- throwing an error if the default is not active:

function search_admin_settings_validate($form, &$form_state) {
  // Check whether we selected a valid default.
  if ($form_state['triggering_element']['#value'] != t('Reset to defaults')) {
    $new_modules = array_filter($form_state['values']['search_active_modules']);
    $default = $form_state['values']['search_default_module'];
    if (!in_array($default, $new_modules, TRUE)) {
      form_set_error('search_default_module', t('Your default search module is not selected as an active module.'));
    }
  }

This is more of a task than a feature request.

fix image link

*I* think if we reroll we would be done, but it would also require someone else to mark the patch RTBC. I think it's ready. We probably need Xano and pwolanin to agree that it is fine.

looks fine to me

Looks good to me. I reckon we could trim some of the descriptions down even further, but separate issue if at all.

Committed/pushed to 8.x, thanks!

This was a pretty useless issue that broke a 100k patch for a critical: #2042807: Convert search plugins to use a ConfigEntity and a PluginBag.

Just saying.

Sorry. This issue has been around for a long time, and had been RTBC since October (aside from rerolls). Tag the other issue "avoid commit conflicts" if you don't want conflicts committed, perhaps?

I'd be fine rolling it back to get the other one in, but it had been stuck in the RTBC queue for two weeks without comment so would have been better postponed if necessary.

Actually, I have a bigger problem with this change than it breaking another issue.

The #1 reason I ever visit this page is to check the index status, or reindex. Before this patch, it was the first thing on the page, and easy to find in its own box:

Now it is buried under a bunch of other text and options like "Simple Chinese/Japanese/Korean handling":

I pinged @jhodgdon in IRC, and she said "if you want it rolled back I will reluctantly concur".

So please, git revert 541f24c, and mark this issue postponed. The entire settings page will be changing, and this can be redone in a less intrusive way after that.

Ok, reverted for now. Postponing on #2042807: Convert search plugins to use a ConfigEntity and a PluginBag so we can get that one in first.

In looking at the various patches here, the one at #1 seems like a nice, non-controversial improvement. I think it was mixing that with a bunch of other things (re-ordering stuff, renaming indexes, etc.) that cause troubles. If we're going to do stuff like that, let's be precise about gathering data about how people actually use this page in order to back up design decisions (I can confirm that's 99.999% of the time what I do on this screen as well).

Looking at the existing indexing blocks:
- webchick and tim.plunkett have already pointed out that the index now button is frequently used, so should be obvious and easy to access.
- There is already an issue to move the indexing throttle settings from here to the node plugin (any other plugins should implement their own limits if required) #2123073: Move index.cron_limit setting to NodeSearch
- Indexing settings says "the default settings should be appropriate for most sites". I suggest collapsing this by default to give greater prominence to the plugins below it.

OK, so we can move the Indexing Status and the button to the top, which seems like the main change, and yes a bunch of the node-specific stuff will be moving entirely off this page in #2042807: Convert search plugins to use a ConfigEntity and a PluginBag. We'll just have to come back here and revisit when that issue is done.

There's also another issue about putting the indexing status on the Status Report page (probably in addition to being on Search Settings).
#1288442: Add search index status to the Status Report page

Since this is rolled back, reverting the title.

See #29, we should probably keep this postponed until we fix #2123073: Move index.cron_limit setting to NodeSearch. At which point there will not be much left on the search settings page, and we may not even need to do this at all.

At this point, the settings that were originally making this page huge (rankings, etc.) have been moved off into NodeSearch page configuration.

So what we have now on the main page is:
- Indexing progress [as Tim pointed out in #27, this is good to have in its own box]
- Indexing throttle
- Indexing settings [a set of 2 settings that cause the search index to be rebuilt, so I think it makes sense to keep them separate from the throttle section, which doesn't cause the index to be rebuilt]
- Logging [whether searches are logged or not]
- Search pages

So, these sections make sense to me. I guess we could move the indexing throttle section into indexing progress, because it's related, but we may yet move throttle to the individual NodeSearch plugin settings on #2123073: Move index.cron_limit setting to NodeSearch anyway... and I think it's probably best not to mix a button for "reindex" with a setting for the throttle anyway.

I also think that the descriptions that are there are necessary. So... I think the page is OK, and I'm going to go ahead and close this issue as "works as designed".