Related: #1759174: Allow to customize the number of context lines in drush features-diff. (And I tried Features Override, as mentioned there... but it didn't give me what I wanted.)

Call me crazy, but I often like my diffs side by side... so I often go to the Drupal UI instead of using 'drush fd'.

There are two things which are a bit irritating though, for repeated lists of similar array structures... (like field instances):

  • The context is often too small to see where we are;
  • Arrays are not indented, because the browser 'squashes' spaces.

So I hacked a little settings form with a reload button into the diff output. See if you like it.

There are probably all kinds of issues with it (among others, taking too much screen real estate... and wasting resources on submiting the form, because features_access_override_actions() first does a whole lot of processing for nothing, before doing a drupal_goto()). But it works, and form input (through URL arguments) is correctly validated/sanitized.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

roderik’s picture

FileSize
4.25 KB

Ahem, wrong patch. When the 'context' argument was left out, it would be set to 0.

This new patch does not set it (i.e. it defaults to a diff variable setting, which is 2 by default).

mpotter’s picture

Status: Needs review » Needs work

Please call it "context-lines" or something other than just "context". Too many "context" terms in Drupal makes it confusing.

Also feel free to add some simple stuff to features.css to get everything all on one line (both fields and the apply button).

roderik’s picture

FileSize
4.87 KB

Cool, I didn't know if you'd like the idea or the implementation. Thanks for review/feedback lots faster than the average queue maintainer :)

I think 'lines' will do for an URL argument in this case, then. Simple CSS for adding things to one line added, too. The patch still adds the CSS for indenting conditionally in admin.inc, 'inline'.

roderik’s picture

Status: Needs work » Needs review

da dum dum

Status: Needs review » Needs work

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

roderik’s picture

Status: Needs work » Needs review
FileSize
4.91 KB

Reroll against RC3

mpotter’s picture

Status: Needs review » Needs work

Still needs a bit more work I think.

1) The "lines" field needs a default. Leaving it blank and checking the "indent" checkbox and then clicking the refresh button complains when the Lines field is blank.

2) Seems like it should be getting values from $form_state['values'] rather than from $_GET[].

Sweetchuck’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
6.45 KB
Sweetchuck’s picture

+++ b/features.admin.inc
@@ -1348,23 +1372,97 @@ function features_feature_diff($feature, $component = NULL) {
+      $formatter->leading_context_lines =
+        $formatter->trailing_context_lines =
+        $diff_settings['lines'];

Maybe the formatter customization could be before the foreach loop instead of in the foreach loop.

Sweetchuck’s picture

Sweetchuck’s picture

Sweetchuck’s picture

Status: Needs review » Needs work

I have some new idea.

Sweetchuck’s picture

Status: Needs work » Needs review
FileSize
17.64 KB
Sweetchuck’s picture

Admin settings.

Sweetchuck’s picture

The huge diff table was exploded by the feature components (1 table per component) and each table goes into a separated jQuery UI Tabs.

I think the markup creation is very ugly, because it does not use any theme() function.

nevergone’s picture

I tested, I think it is good.

Sweetchuck’s picture

Notice error fix.

Sweetchuck’s picture

Sweetchuck’s picture

Replace context lines textfield with a select list.
hook_requirements() to inform about the available difference viewers.

Sweetchuck’s picture

In this patch: New configuration options for default diff provider.
Next step is: Add theme functions for diff output.

Sweetchuck’s picture

YesCT’s picture

I think the issue summary needs updating, and some screenshots would be nice.

I was just wishing for a setting of the number of context lines.

nagy.balint’s picture

This functionality would be very useful for me, as I use the review overrides page quite often, and this patch makes diffs easier to review by showing each category in the feature as a separate tab, and also by providing a way to select how many lines to display before and after.

I reviewed the patch and it worked fine for me, i hope this gets in the module soon.

Attached a few new images about the current patch.

Aron Novak’s picture

A few points after my review:

  • --- /dev/null
    +++ b/js/features.diff-formatters.navigator.js
    @@ -0,0 +1,15 @@
    +/**
    + * @file
    + * Documentation missing.
    + */

    It needs a proper head comment.

  • Do you think it's meaningful to make the list of formatters alterable (function features_diff_formatters($all = FALSE) {)? It might need to specify include files in the array and one (multiple?) callback per formatter and it would be more flexible in this case. It might worth consideration.
Sweetchuck’s picture

jsibley’s picture

Has this died? It looks like there hasn't been any update since the patch 2 years ago.