Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proof of concept: visualize hierarchical data #66479

Merged
merged 24 commits into from
Dec 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
95943cf
Visualize parent
oandregal Oct 24, 2024
65dd563
Sort by title asc
oandregal Nov 29, 2024
0fb533b
Return posts sorted by hierarchy
oandregal Nov 28, 2024
a058995
Visualize hierarchy
oandregal Dec 9, 2024
4ef527c
Add view config control to switch on/off hierarchical vizualization
oandregal Dec 3, 2024
ba3ce47
Revert "Add view config control to switch on/off hierarchical vizuali…
oandregal Dec 12, 2024
1fb12e3
Control automatically when hierarchical sort is active
oandregal Dec 12, 2024
404d764
Display parent fields when hierarchy is active
oandregal Dec 12, 2024
3e8946e
Revert "Display parent fields when hierarchy is active"
oandregal Dec 12, 2024
3c753ac
Hide parent field by default
oandregal Dec 12, 2024
6983694
Rename 'init' to 'get_instance' to reflect singleton pattern
mcsf Dec 16, 2024
1c10d7e
GutenbergHierarchicalSortTest: satisfy linter
mcsf Dec 16, 2024
c04da3d
Gutenberg_Hierarchical_Sort::get_ancestor: Refactor using `??`
mcsf Dec 16, 2024
affe071
Gutenberg_Hierarchical_Sort::sort: Refactor for clarity
mcsf Dec 16, 2024
ef44147
Rename hierchicalSort to showLevels
oandregal Dec 17, 2024
e6c708e
Set showLevels internally using onChangeView instead of effect
oandregal Dec 17, 2024
e53e69b
Do not provide a default implementation for getItemLevel
oandregal Dec 17, 2024
da863ad
Remove unnecessary change
oandregal Dec 17, 2024
307944d
Cover against empty post parent
oandregal Dec 17, 2024
33c093f
Clarify PHPDoc and tests
oandregal Dec 17, 2024
c1961db
Update comment
oandregal Dec 17, 2024
bf876a8
Make PHP linter happy
oandregal Dec 17, 2024
9cd80ee
Add backport changelog file
oandregal Dec 17, 2024
28e8449
Limit to 9 items
oandregal Dec 18, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions backport-changelog/6.8/8014.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
https://github.com/WordPress/wordpress-develop/pull/8014

* https://github.com/WordPress/gutenberg/pull/66479
205 changes: 205 additions & 0 deletions lib/compat/wordpress-6.8/class-gutenberg-hierarchical-sort.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,205 @@
<?php

/**
* Modifies the Post controller endpoint to support orderby_hierarchy.
*
* @package gutenberg
* @since 6.8.0
*/

class Gutenberg_Hierarchical_Sort {
private static $post_ids = array();
private static $levels = array();
private static $instance;

public static function get_instance() {
if ( null === self::$instance ) {
self::$instance = new self();
}

return self::$instance;
}

public function run( $args ) {
$new_args = array_merge(
$args,
array(
'fields' => 'id=>parent',
'posts_per_page' => -1,
)
);
$query = new WP_Query( $new_args );
$posts = $query->posts;
$result = self::sort( $posts );

self::$post_ids = $result['post_ids'];
self::$levels = $result['levels'];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we using static variables instead of returning the results instead? Personally I don't like this kind of caching. Actually, it's really not clear to me why this whole class exists. It can be just a function no (or a couple)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only reason for this class to exist is to make this PR performant. Given this PR uses two different filters, we'd have to calculate the info twice if we don't have a way to hold the results in the interim. The implementation of the backport PR would be a bit different that this: it can just modify the get_items method directly, so this can be simplified to be a simple function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More context: I used the filter approach in this PR instead of modifying the get_items method directly because I wanted reviewers to understand what changed. If this modified the get_items method it'd have been hundreds of lines of (untouched) code with a few tweaks in between.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate the review-friendly approach, especially when approaching something new to DataViews like hierarchy. The current approach with the singleton class isn't perfect, but I think it's acceptable for now. Thinking about Riad's question, what crossed my mind was to use global functions with an explicit cache:

// In rest_page_query filter
$cache_key        = serialize( $request->get_params() );
$result           = get_hierarchical_sort( $args, $cache_key );
$args['post__in'] = $result['post_ids'];

// In rest_prepare_page filter
//
// The `null` argument combines with the cache key to signal that the function
// should retrieve the previously calculated value.
$cache_key               = serialize( $request->get_params() );
$result                  = get_hierarchical_sort( null, $cache_key );
$response->data['level'] = $result['levels'];

But, honestly, this comes with other problems (a false impression that nothing could have mutated in between the two function calls?) and I don't think we should invest more time in anything other than "the real thing", as André points out, i.e. get_items. Until then, this class is fine. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't like the class to be honest. The same reason I don't like the current theme_json class. We know by experience now that this architecture created a lot of bugs (outdates styles...). I don't think using classes properties to cache things like that is good.

You call a run function with args but the cache is actually independent of the "args", so one calling the run function later with another set of args will conflict with the initial call.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What kind of performance penalty are we talking about if we remove all caching?

If indeed there is a penalty to avoid, how palatable would something like this be to both of you? (This works in my local Pages view.)

diff --git a/lib/compat/wordpress-6.8/class-gutenberg-hierarchical-sort.php b/lib/compat/wordpress-6.8/class-gutenberg-hierarchical-sort.php
index f61002f435a..7d9d9b9c22e 100644
--- a/lib/compat/wordpress-6.8/class-gutenberg-hierarchical-sort.php
+++ b/lib/compat/wordpress-6.8/class-gutenberg-hierarchical-sort.php
@@ -8,19 +8,7 @@
  */
 
 class Gutenberg_Hierarchical_Sort {
-	private static $post_ids = array();
-	private static $levels   = array();
-	private static $instance;
-
-	public static function get_instance() {
-		if ( null === self::$instance ) {
-			self::$instance = new self();
-		}
-
-		return self::$instance;
-	}
-
-	public function run( $args ) {
+	public static function run( $args ) {
 		$new_args = array_merge(
 			$args,
 			array(
@@ -32,8 +20,7 @@ class Gutenberg_Hierarchical_Sort {
 		$posts    = $query->posts;
 		$result   = self::sort( $posts );
 
-		self::$post_ids = $result['post_ids'];
-		self::$levels   = $result['levels'];
+		return $result;
 	}
 
 	/**
@@ -146,14 +133,6 @@ class Gutenberg_Hierarchical_Sort {
 			}
 		}
 	}
-
-	public static function get_post_ids() {
-		return self::$post_ids;
-	}
-
-	public static function get_levels() {
-		return self::$levels;
-	}
 }
 
 add_filter(
@@ -171,17 +150,23 @@ add_filter(
 add_filter(
 	'rest_page_query',
 	function ( $args, $request ) {
+		global $_gutenberg_hierarchical_sort_cache;
+
 		if ( ! Gutenberg_Hierarchical_Sort::is_eligible( $request ) ) {
 			return $args;
 		}
 
-		$hs = Gutenberg_Hierarchical_Sort::get_instance();
-		$hs->run( $args );
+		$result = Gutenberg_Hierarchical_Sort::run( $args );
 
 		// Reconfigure the args to display only the ids in the list.
-		$args['post__in'] = $hs->get_post_ids();
+		$args['post__in'] = $result['post_ids'];
 		$args['orderby']  = 'post__in';
 
+		$_gutenberg_hierarchical_sort_cache[] = array(
+			'request' => $request,
+			'result'  => $result,
+		);
+
 		return $args;
 	},
 	10,
@@ -195,8 +180,13 @@ add_filter(
 			return $response;
 		}
 
-		$hs                      = Gutenberg_Hierarchical_Sort::get_instance();
-		$response->data['level'] = $hs->get_levels()[ $post->ID ];
+		global $_gutenberg_hierarchical_sort_cache;
+		foreach ( $_gutenberg_hierarchical_sort_cache as $tuple ) {
+			if ( $request === $tuple['request'] ) {
+				$levels                  = $tuple['result']['levels'];
+				$response->data['level'] = $levels[ $post->ID ];
+			}
+		}
 
 		return $response;
 	},

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@youknowriad @mcsf I'd like to land the backport PR and it doesn't use filters or the static cache in the HierarchicalSort class. WordPress/wordpress-develop#8014

Note the HierarchicalSort class still exist and exposes two public static methods. I think this is nice to prevent polluting the namespace with intermediate helper functions. If that's a blocker for you, I'm happy to make everything functions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note the HierarchicalSort class still exist and exposes two public static methods. I think this is nice to prevent polluting the namespace with intermediate helper functions. If that's a blocker for you, I'm happy to make everything functions.

Using a class is not a blocker, but I would love for us to have a system in place for when to use functions and when to use classes rather than each one coming up with its own approach to things and second guessing ourselves.

}

/**
* Check if the request is eligible for hierarchical sorting.
*
* @param array $request The request data.
*
* @return bool Return true if the request is eligible for hierarchical sorting.
*/
public static function is_eligible( $request ) {
if ( ! isset( $request['orderby_hierarchy'] ) || true !== $request['orderby_hierarchy'] ) {
return false;
}

return true;
}

public static function get_ancestor( $post_id ) {
return get_post( $post_id )->post_parent ?? 0;
oandregal marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Sort posts by hierarchy.
*
* Takes an array of posts and sorts them based on their parent-child relationships.
* It also tracks the level depth of each post in the hierarchy.
*
* Example input:
* ```
* [
* ['ID' => 4, 'post_parent' => 2],
* ['ID' => 2, 'post_parent' => 0],
* ['ID' => 3, 'post_parent' => 2],
* ]
* ```
*
* Example output:
* ```
* [
* 'post_ids' => [2, 4, 3],
* 'levels' => [0, 1, 1]
* ]
* ```
*
* @param array $posts Array of post objects containing ID and post_parent properties.
*
* @return array {
* Sorted post IDs and their hierarchical levels
*
* @type array $post_ids Array of post IDs
* @type array $levels Array of levels for the corresponding post ID in the same index
* }
*/
public static function sort( $posts ) {
/*
* Arrange pages in two arrays:
*
* - $top_level: posts whose parent is 0
* - $children: post ID as the key and an array of children post IDs as the value.
* Example: $children[10][] contains all sub-pages whose parent is 10.
*
* Additionally, keep track of the levels of each post in $levels.
* Example: $levels[10] = 0 means the post ID is a top-level page.
*
*/
$top_level = array();
$children = array();
foreach ( $posts as $post ) {
if ( empty( $post->post_parent ) ) {
$top_level[] = $post->ID;
} else {
$children[ $post->post_parent ][] = $post->ID;
mcsf marked this conversation as resolved.
Show resolved Hide resolved
}
}

$ids = array();
$levels = array();
self::add_hierarchical_ids( $ids, $levels, 0, $top_level, $children );

// Process remaining children.
if ( ! empty( $children ) ) {
foreach ( $children as $parent_id => $child_ids ) {
$level = 0;
$ancestor = $parent_id;
while ( 0 !== $ancestor ) {
++$level;
$ancestor = self::get_ancestor( $ancestor );
}
self::add_hierarchical_ids( $ids, $levels, $level, $child_ids, $children );
}
}

return array(
'post_ids' => $ids,
'levels' => $levels,
);
}

private static function add_hierarchical_ids( &$ids, &$levels, $level, $to_process, $children ) {
foreach ( $to_process as $id ) {
if ( in_array( $id, $ids, true ) ) {
continue;
}
$ids[] = $id;
$levels[ $id ] = $level;

if ( isset( $children[ $id ] ) ) {
self::add_hierarchical_ids( $ids, $levels, $level + 1, $children[ $id ], $children );
unset( $children[ $id ] );
}
}
}

public static function get_post_ids() {
return self::$post_ids;
}

public static function get_levels() {
return self::$levels;
}
}

add_filter(
'rest_page_collection_params',
function ( $params ) {
$params['orderby_hierarchy'] = array(
'description' => 'Sort pages by hierarchy.',
'type' => 'boolean',
'default' => false,
);
return $params;
}
);

add_filter(
'rest_page_query',
function ( $args, $request ) {
if ( ! Gutenberg_Hierarchical_Sort::is_eligible( $request ) ) {
return $args;
}

$hs = Gutenberg_Hierarchical_Sort::get_instance();
$hs->run( $args );

// Reconfigure the args to display only the ids in the list.
$args['post__in'] = $hs->get_post_ids();
$args['orderby'] = 'post__in';

return $args;
},
10,
2
);

add_filter(
'rest_prepare_page',
function ( $response, $post, $request ) {
if ( ! Gutenberg_Hierarchical_Sort::is_eligible( $request ) ) {
return $response;
}

$hs = Gutenberg_Hierarchical_Sort::get_instance();
$response->data['level'] = $hs->get_levels()[ $post->ID ];

return $response;
},
10,
3
);
1 change: 1 addition & 0 deletions lib/load.php
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ function gutenberg_is_experiment_enabled( $name ) {
require __DIR__ . '/compat/wordpress-6.8/block-comments.php';
require __DIR__ . '/compat/wordpress-6.8/class-gutenberg-rest-comment-controller-6-8.php';
require __DIR__ . '/compat/wordpress-6.8/class-gutenberg-rest-post-types-controller-6-8.php';
require __DIR__ . '/compat/wordpress-6.8/class-gutenberg-hierarchical-sort.php';
require __DIR__ . '/compat/wordpress-6.8/rest-api.php';

// Plugin specific code.
Expand Down
8 changes: 6 additions & 2 deletions packages/dataviews/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,13 @@

- Fixed commonjs export ([#67962](https://github.com/WordPress/gutenberg/pull/67962))

### Features

- Add support for hierarchical visualization of data. `DataViews` gets a new prop `getItemLevel` that should return the hierarchical level of the item. The view can use `view.showLevels` to display the levels. It's up to the consumer data source to prepare this information.

## 4.10.0 (2024-12-11)

## Breaking Changes
### Breaking Changes

- Support showing or hiding title, media and description fields ([#67477](https://github.com/WordPress/gutenberg/pull/67477)).
- Unify the `title`, `media` and `description` fields for the different layouts. So instead of the previous `view.layout.mediaField`, `view.layout.primaryField` and `view.layout.columnFields`, all the layouts now support these three fields with the following config ([#67477](https://github.com/WordPress/gutenberg/pull/67477)):
Expand All @@ -23,7 +27,7 @@ const view = {
};
```

## Internal
### Internal

- Upgraded `@ariakit/react` (v0.4.13) and `@ariakit/test` (v0.4.5) ([#65907](https://github.com/WordPress/gutenberg/pull/65907)).
- Upgraded `@ariakit/react` (v0.4.15) and `@ariakit/test` (v0.4.7) ([#67404](https://github.com/WordPress/gutenberg/pull/67404)).
Expand Down
20 changes: 17 additions & 3 deletions packages/dataviews/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,19 @@ Example:
}
```

#### `getItemLevel`: `function`

A function that receives an item and returns its hierarchical level. It's optional, but this property must be passed for DataViews to display the hierarchical levels of the data if `view.showLevels` is true.

Example:

```js
// Example implementation
{
getItemLevel={ ( item ) => item.level }
}
```

#### `fields`: `Object[]`

The fields describe the visible items for each record in the dataset and how they behave (how to sort them, display them, etc.). See "Fields API" for a description of every property.
Expand Down Expand Up @@ -191,6 +204,7 @@ Properties:
- `showTitle`: Whether the title should be shown in the UI. `true` by default.
- `showMedia`: Whether the media should be shown in the UI. `true` by default.
- `showDescription`: Whether the description should be shown in the UI. `true` by default.
- `showLevels`: Whether to display the hierarchical levels for the data. `false` by default. See related `getItemLevel` DataView prop.
- `fields`: a list of remaining field `id` that are visible in the UI and the specific order in which they are displayed.
- `layout`: config that is specific to a particular layout type.

Expand Down Expand Up @@ -302,8 +316,8 @@ const actions = [
RenderModal: ( { items, closeModal, onActionPerformed } ) => (
<div>
<p>Are you sure you want to delete { items.length } item(s)?</p>
<Button
variant="primary"
<Button
variant="primary"
onClick={() => {
console.log( 'Deleting items:', items );
onActionPerformed();
Expand Down Expand Up @@ -520,7 +534,7 @@ The unique identifier of the action.
- Required
- Example: `move-to-trash`

### `label`
### `label`

The user facing description of the action.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ type DataViewsContextType< Item > = {
openedFilter: string | null;
setOpenedFilter: ( openedFilter: string | null ) => void;
getItemId: ( item: Item ) => string;
getItemLevel?: ( item: Item ) => number;
onClickItem?: ( item: Item ) => void;
isItemClickable: ( item: Item ) => boolean;
};
Expand Down
2 changes: 2 additions & 0 deletions packages/dataviews/src/components/dataviews-layout/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export default function DataViewsLayout() {
data,
fields,
getItemId,
getItemLevel,
isLoading,
view,
onChangeView,
Expand All @@ -40,6 +41,7 @@ export default function DataViewsLayout() {
data={ data }
fields={ fields }
getItemId={ getItemId }
getItemLevel={ getItemLevel }
isLoading={ isLoading }
onChangeView={ onChangeView }
onChangeSelection={ onChangeSelection }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ function SortFieldControl() {
direction: view?.sort?.direction || 'desc',
field: value,
},
showLevels: false,
} );
} }
/>
Expand Down Expand Up @@ -194,6 +195,7 @@ function SortDirectionControl() {
)?.id ||
'',
},
showLevels: false,
} );
return;
}
Expand Down
3 changes: 3 additions & 0 deletions packages/dataviews/src/components/dataviews/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ type DataViewsProps< Item > = {
onClickItem?: ( item: Item ) => void;
isItemClickable?: ( item: Item ) => boolean;
header?: ReactNode;
getItemLevel?: ( item: Item ) => number;
} & ( Item extends ItemWithId
? { getItemId?: ( item: Item ) => string }
: { getItemId: ( item: Item ) => string } );
Expand All @@ -64,6 +65,7 @@ export default function DataViews< Item >( {
actions = EMPTY_ARRAY,
data,
getItemId = defaultGetItemId,
getItemLevel,
isLoading = false,
paginationInfo,
defaultLayouts,
Expand Down Expand Up @@ -115,6 +117,7 @@ export default function DataViews< Item >( {
openedFilter,
setOpenedFilter,
getItemId,
getItemLevel,
isItemClickable,
onClickItem,
} }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ const _HeaderMenu = forwardRef( function HeaderMenu< Item >(
field: fieldId,
direction,
},
showLevels: false,
} );
} }
>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,15 @@ import getClickableItemProps from '../utils/get-clickable-item-props';

function ColumnPrimary< Item >( {
item,
level,
titleField,
mediaField,
descriptionField,
onClickItem,
isItemClickable,
}: {
item: Item;
level?: number;
titleField?: NormalizedField< Item >;
mediaField?: NormalizedField< Item >;
descriptionField?: NormalizedField< Item >;
Expand All @@ -44,6 +46,11 @@ function ColumnPrimary< Item >( {
<VStack spacing={ 0 }>
{ titleField && (
<div { ...clickableProps }>
{ level !== undefined && (
<span className="dataviews-view-table__level">
{ '—'.repeat( level ) }&nbsp;
</span>
) }
<titleField.render item={ item } />
</div>
) }
Expand Down
Loading
Loading