UI cleanup of status and re-index button

The previous updates broke the progress bar ... this fixes it. One side effect of this patch is that theme('progress') now has an optional third argument, which is the class. If this class is set to 'inactive', then the progress bar gif is not shown. I don't really like the way this is implemented, because you have to know to use the class 'inactive' but maybe as long as it's document, it's fine.

Testing failures were due to theme_progress_bar's new argument not having a default value... corrected in attached patch.

I always thought it was a feature that the re-indexing happens using cron so I'm not 100% convinced that using the batch API is an improvement. Can't we simply improve people's expectations by changing the name of the label, or adding some extra help text. It seems like that could get the job done with a lot less code.

@dries, yes we could just cleanup the UI, and then leave the batchapi in a contrib module. The contrib module to do this has existed for a couple years and nobody seems to know about it, so that's another problem. We are adding some code to handle the batchapi stuff, but ALL of that code is in a separate include that is only loaded when it is needed, but it's currently 121 lines.

Attached is an updated patch that adds more granular messages based on if all, none, or some things need to be indexed. It also hides the one or the other button when it doesn't make sense. Lastly, I had some help with the messages, and tried to make them more positive, than negative, in tone.

I've added a hook_requirements for the status page, based on #394102: Make search module usable. I also spent some time with Josh working on the messaging and workflow. We may want to break this up into several patches (usability, batch processing, and hook_requirements), but for now, it's one big patch.

Adding pictures of the status page, Bojhan already tells me to remove the progress bar from the status page.

I sat beside douggreen at Drupalcon to discuss these text. The confirmation messages sound all right to me, obviously the title is somewhat wrong - but I want to fix that in a later patch for all confirmation messages.

I think we don't want to use status bar messages in the status report, since it draws to much attention. The text as displayed in the last images for the status report seem oke to me, clear and concise.

There was another commit in system.css that through off the lines here... New patch attached.

OK, some of the nodes were set to reindex (or not reset, during the batch process), but I have no idea why. Investigating some more

Thanks Ken! I saw the same problem this morning, and forgot about. The problem is that search is noticing internal links on the node to itself, via the "Add new comment" link. I've created a separate issue for this, see #569536: Search links cause many nodes to be indexed twice.

I don't get that part.
Why don't we just have search_batch_index() as one single multipass batch operation that selects the next N nodes to reindex, treats them and returns ?
That would be preferable to adding a potentially large number of batch operations each with a potentially large array of nids. Heavy $batch structures are slower to process.

I'm on crack. Are you, too?

doug: you can look at node_access_rebuild() and _node_access_rebuild_batch_operation() for a single-multipass-op example:
- count the 'total' number of nodes the first time the op is called
- on each pass, grab the next N nodes using a query with a range and an 'order by'.

About nodes being added/updated during the processing: the most important thing is to be sure that you don't report '#finished = 1' before you are sure that all nodes have been processed. When you have treated a number of node equal to 'total', you'll need to check whether there are new ones to process. Thus, the order used in the query that retreives the next N nodes should ensure that such nodes come last.
This might cause the progress bar to jump back a little as you found out 'total' is not your initial estimate, but that's not really an issue IMO.

Then again, those nodes that appear during the processing would be indexed during the next cron, right? Is it that bad if the batch reindex skips them ? They were not here when we started...

Side note: you might want to ensure no cron reindex happens while the batch reindex is running. Is that handled already ? If not, this sounds like a use case for http://api.drupal.org/api/group/lock/7

Great ! Additional remarks:

- Please use $context['sandbox'] instead of $context['results'] to store processing data between iterations.
$context['sandbox'] persists only during the iterations of a given multipass batch op.
$context['results'] persists through all operations of the batch, and is used to present 'results' (whatever that might mean for the actual processing being batched) at the end of the processing. In this case, this only applies to the 'Finished indexing @total nodes.' message.

- No biggie, but you don't really *need* to use variable_get('search_cron_limit') here. search_cron_limit makes sense for cron, not really for direct 'under your eyes' rebuild. You can chose an arbitrary value like N = 10, 50 or 100. The function indexes N nodes, and will be called repeatedly by batch API until we exceed 1sec processing time, then we report progress and wait for the next HTTP request. Using huge values like 500 (possible value for search_cron_limit) won't really get you done faster, only report progress less frequently to the users. Since search admin proposes values as low as 10 (10, 20, 50, 100, 200, 500), I'd suggest using 10.

- May I suggest _search_index_batch_operation() for the name of the function ? Stays 'consistent' with _node_access_rebuild_batch_operation()

- The code that loads, builds, renders and indexes the node would be best separated in a helper function and reused with 'regular', cron-based reindex. Or there's 99% chances a later change in the one won't be reported in the other.

Note that I'm strictly helping the batch-related code, not reviewing the whole patch or feature.

Forgot to add : are you sure the query that grabs the next N nodes in _search_batch_index() won't retrieve always the same nodes ?
Typically, we store the nid of the last node treated in the sandbox, and use a WHERE node.nid > $context['sandbox']['last_nid'] SORT BY node.nid in the request that grabs the next N.

If I changed from $context['results'] to $context['sandbox'], how do I reference my variables in the finished function?

If you want to display the number of indexed nodes at the end, you'd store that one in $context['results']['indexed_nodes'].

@dries, We removed the progress bar on the configuration change some time ago based on Bojhan's recommendation. In comment 13 I uploaded new screenshots, but didn't upload one for the config page.

Patch misses search.batch.inc ?

I'm adding the tag "Needs Usability Review," although Bohjan has already taken one look at this, to see if we can get a "Usability Team Approved" tag.

- 'Completed @current of @total' message:
A bit ugly ;-).
The displayed message is the concatenation of 1) the generic $batch['progress_message'] 2) the specific message set by the last batch op in $context['message'].
I'd suggest you set $batch['progress_message'] = ''; in search_batch_index_confirm_submit(), and set $context['message'] = t('Completed @current of @total.', ...); in _search_batch_index().

- A generic 'cancel' feature for batch API is a little tricky. *If* in the case of nodes indexing you can safely just break out without any cleanup work (dunno for sure, I leave it up to you to confirm that), then your approach sounds like it should work.

- I still have one concern voiced in #28: It looks like the query $result = db_query_range("SELECT DISTINCT n.nid FROM {node} n LEFT JOIN {search_dataset} d ON d.type = 'node' AND d.sid = n.nid WHERE d.sid IS NULL OR d.reindex <> 0 ORDER BY d.reindex ASC, n.nid ASC", array(), 0, 100); will keep retrieveing the 100 same nodes over and over... See #28 for the usual approach.

- "I don't know what's ugly about the message": Yes, my previous comment was not clear. I was talking about the code:

Use $context['message'] for this, instead of hacking the batch structure. Set $batch['progress_message'] = ''; when defining the batch in search_batch_index_confirm_submit().

- About cancel: the tricky thing is that your search_batch_index_cancel() callback will be executed while the server is still executing the previous batch HTTP request in a different thread. I'm not sure what race conditions can occur there, running _search_batch_update_totals() while some nodes are still being indexed. That's precisely why a 'Cancel' feature for batch API is not easy ;-)

- OK fo the query not returning the same 100 nodes each time. Could use a comment IMO.

The last submitted patch failed testing.

Rerolled, now that #581594: minor db_query() fix to search for DB TNG was committed.

@douggreen:
I'm on vacation and only occasionally getting net access, so I won't be able to help much :-(
Fixing the 'error' on cancel will probably require a tweak around progress.js, so that the ajax error callback is disabled when the user clicks cancel.

re-rolled to current head

Next I'm going to create a patch that just has the UI improvements and doesn't do the batch changes. I think that we really need to get batchapi pause/resume/cancel working before it's added here. And that's beyond the scope of this patch, and will need to wait until Drupal 8 :(

I missed one change in search.install needed after stripping out the batchapi changes...

Reviewers please look at the new language, especially the messages in _search_get_status_metrics() labeled 'zero', 'none', 'partial', and 'all'.

Should this be bumped up to Drupal 8 at this point?

This improvement is still awesome to happen.