WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 9 months ago

Last modified 9 months ago

#24908 closed enhancement (fixed)

Proposed new filters/hooks for revisions

Reported by: adamsilverstein Owned by: johnbillion
Milestone: 4.1 Priority: normal
Severity: normal Version: 4.1
Component: Revisions Keywords: has-patch commit
Focuses: Cc:

Description

I would like to propose a few simple filters for the revisions and diff engine in core. in particular, I am trying to create a plugin that re-introduces features tried and dropped from (or present before) the 3.6 version of revisions.

There are two features that need simple hooks to work; without them i will be forced to bundle a modified version of the revisions/diff system or something else kludgy with my plugin, and that seems ugly.

The two features are a WYSI(closer to)WYG and a single column instead of split column view for diffs.

I am attaching a patch that introduces the filters I need, not sure i'm doing it right or used appropriate names for the filters?

The plugin (warning, work in progress) is posted here: https://github.com/adamsilverstein/RevisionsPlus

Attachments (5)

24908.diff (2.1 KB) - added by adamsilverstein 2 years ago.
introduce filters for diff html processing and show_split_view option
24908.2.diff (2.2 KB) - added by adamsilverstein 2 years ago.
add context to the process_revision_diff_html filter
24908.3.diff (3.0 KB) - added by adamsilverstein 22 months ago.
add hook docs, adjusted filter name
24908.4.diff (3.4 KB) - added by adamsilverstein 10 months ago.
updates as per suggestions
24908.5.diff (3.9 KB) - added by johnbillion 9 months ago.

Download all attachments as: .zip

Change History (27)

@adamsilverstein2 years ago

introduce filters for diff html processing and show_split_view option

comment:1 follow-up: @DrewAPicture2 years ago

  • Cc xoodrew@… added

+1 for some filters here.

We should probably add a context arg to the htmlspecialchars filters denoting each method, such as added, deleted, or context since we're using the same filter hook in three different places.

comment:2 in reply to: ↑ 1 ; follow-up: @adamsilverstein2 years ago

Replying to DrewAPicture:

+1 for some filters here.

We should probably add a context arg to the htmlspecialchars filters denoting each method, such as added, deleted, or context since we're using the same filter hook in three different places.

good idea! i was wondering about that, generally the three calls are for the same purpose, but i could definitely see plugin developers adding some specific processing for just the added, deleted or unchanged lines.

there are also probably other spots that could use filters, i will keep looking;

comment:3 in reply to: ↑ 2 ; follow-up: @DrewAPicture2 years ago

Replying to adamsilverstein:

... generally the three calls are for the same purpose, but i could definitely see plugin developers adding some specific processing for just the added, deleted or unchanged lines.

It's more in the vein of "most people would probably change the filter output the same for all three, but being able to filter based on context is a nice-to-have".

@adamsilverstein2 years ago

add context to the process_revision_diff_html filter

comment:4 in reply to: ↑ 3 @adamsilverstein2 years ago

Replying to DrewAPicture:

Replying to adamsilverstein:

... generally the three calls are for the same purpose, but i could definitely see plugin developers adding some specific processing for just the added, deleted or unchanged lines.

It's more in the vein of "most people would probably change the filter output the same for all three, but being able to filter based on context is a nice-to-have".

good suggestion, i can already think of how i would use it! added in 24908.2.diff​.

@adamsilverstein22 months ago

add hook docs, adjusted filter name

comment:6 @adamsilverstein22 months ago

24908.3.diff - refreshed, add hook docs, adjusted filter name

comment:7 @DrewAPicture22 months ago

Inline docs notes on 24908.3.diff:
Overall:

  • There should be an empty line after the @since lines
  • No leading slash on file paths in the duplicate comments

show_revisions_split_view filter:

  • Use a docs-specific variable to represent the boolean in the parameter line. Maybe $bool or something
  • Specify the default at the end of the parameter description: "Default true."
  • Wrap the long description after "(two columns)"

process_text_diff_html filter:

  • There are three parameters but only two are documented
  • The long description is super cryptic. I had to read it a couple of times to get it. Consider using as little jargon as possible. Also, the various contexts need more explanation.

comment:8 @adamsilverstein22 months ago

Thanks for the feedback, will revise.

comment:9 @adamsilverstein20 months ago

  • Summary changed from Proposed new filters for revisions to Proposed new filters/hooks for revisions

comment:10 @johnbillion10 months ago

  • Keywords needs-refresh added; has-patch dev-feedback removed
  • Milestone changed from Awaiting Review to 4.1

comment:11 @johnbillion10 months ago

@adamsilverstein If you can get those docs tweaks in, plus the change to the first filter so it filters the whole args array instead of just the show_split_view argument, then I'll get this in.

@adamsilverstein10 months ago

updates as per suggestions

comment:12 @adamsilverstein10 months ago

  • Keywords has-patch dev-feedback added; needs-refresh removed

In 24908.4.diff:

  • Update docblock headers as per @drewapicture's suggestions
  • Pass the entire options array more clearly to the renamed show_revisions_split_view filter, although show_split_view is the only option currently passed

comment:13 @slackbot10 months ago

This ticket was mentioned in Slack in #core by adamsilverstein. View the logs.

comment:14 @slackbot10 months ago

This ticket was mentioned in Slack in #core by adamsilverstein. View the logs.

comment:15 follow-up: @ericlewis10 months ago

I think we should change 'context' to 'unchanged' here:

$line = apply_filters( 'process_text_diff_html', htmlspecialchars( $line ), $line, 'context' ); 

otherwise this looks rad.

comment:16 in reply to: ↑ 15 @adamsilverstein10 months ago

Sounds good to me. I used 'context' to match the internal naming, but 'unchanged' makes way more sense...

Replying to ericlewis:

I think we should change 'context' to 'unchanged' here:

$line = apply_filters( 'process_text_diff_html', htmlspecialchars( $line ), $line, 'context' ); 

otherwise this looks rad.

@johnbillion9 months ago

comment:17 @johnbillion9 months ago

  • Keywords commit added; dev-feedback removed

24908.5.diff is a final pass at this.

  • Use revision_text_diff_options as the text diff options filter name
  • Add some more context parameters to the above
  • Use unchanged instead of context for the process_text_diff_html filter context, along with added and deleted

comment:18 @johnbillion9 months ago

  • Owner set to johnbillion
  • Resolution set to fixed
  • Status changed from new to closed

In 30396:

Introduce two new filters to the post revisions screen:

  • process_text_diff_html for contextually filtering a diffed line. Allows for the line to be processed in a different manner to the default htmlspecialchars.
  • revision_text_diff_options for filtering the options passed to wp_text_diff() when viewing a post revision.

Fixes #24908
Props adamsilverstein

comment:19 @DrewAPicture9 months ago

In 30497:

Minor syntactical adjustments to the inline documentation for the revision_text_diff_options hook.

Converts backticked-inline-code to inline @see tags (for the full benefit of Code Reference automagical behavior).

See [30396]. See #24908.

comment:20 @johnbillion9 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Version set to trunk

r30396 erroneously changed the default value of the show_split_view argument to false which means all revisions are currently shown inline instead of side by side.

comment:21 follow-up: @johnbillion9 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 30739:

Correct the default value of the show_split_view argument for revisions so they are correctly displayed side by side ins
tead of inline.

Fixes #24908

comment:22 in reply to: ↑ 21 @adamsilverstein9 months ago

Replying to johnbillion:

In 30739:

Correct the default value of the show_split_view argument for revisions so they are correctly displayed side by side ins
tead of inline.

Fixes #24908

Good catch! Thanks.

Note: See TracTickets for help on using tickets.