Merge pull request #3136 from perspective-dev/fix-datagrid-sort-jump

Fix datagrid scroll position shift when sorting

Author: texodus

packages/viewer-datagrid/src/ts/data_listener/index.ts

@@ -24,7 +24,7 @@ import type {
     Schema,
 } from "../types.js";
 import type { CellScalar, DataResponse } from "regular-table/dist/esm/types.js";
-import { ViewWindow } from "@perspective-dev/client";
+import { ViewConfig, ViewWindow } from "@perspective-dev/client";
 
 interface ColumnData {
     __ROW_PATH__?: unknown[][];
@@ -192,8 +192,10 @@ export function createDataListener(
             column_paths.push(path);
         }
 
+        const is_dim_call = x1 - x0 > 0 && y1 - y0 > 0;
+
         // Only update the last state if this is not a "phantom" call.
-        if (x1 - x0 > 0 && y1 - y0 > 0) {
+        if (is_dim_call) {
             this.last_column_paths = last_column_paths;
             this.last_meta = last_meta;
             this.last_ids = last_ids;
@@ -223,7 +225,9 @@ export function createDataListener(
             ),
         ) as (string | HTMLElement)[][];
 
-        const num_row_headers = row_headers[0]?.length;
+        const num_row_headers = !is_dim_call
+            ? row_header_depth(this._config)
+            : row_headers[0]?.length;
 
         const result: DataResponse = {
             num_column_headers:
@@ -244,3 +248,15 @@ export function createDataListener(
         return result;
     };
 }
+
+function row_header_depth(config: ViewConfig) {
+    if (config.group_rollup_mode === "flat") {
+        return config.group_by.length;
+    } else if (config.group_rollup_mode === "total") {
+        return 0;
+    } else if (config.group_by.length === 0) {
+        return 0;
+    } else {
+        return config.group_by.length + 1;
+    }
+}

packages/viewer-datagrid/src/ts/event_handlers/sort.ts

@@ -28,13 +28,13 @@ const ROW_SORT_ORDER: SortRotationOrder = {
 
 const ROW_COL_SORT_ORDER: SortRotationOrder = {
     desc: "asc",
-    asc: "col desc",
+    asc: undefined,
     "desc abs": "asc abs",
-    "asc abs": "col desc abs",
-    "col desc": "col asc",
-    "col asc": undefined,
-    "col desc abs": "col asc abs",
-    "col asc abs": undefined,
+    "asc abs": undefined,
+    // "col desc": "col asc",
+    // "col asc": undefined,
+    // "col desc abs": "col asc abs",
+    // "col asc abs": undefined,
 };
 
 export async function sortHandler(
@@ -49,7 +49,7 @@ export async function sortHandler(
     const column_name = meta.column_header[this._config.split_by.length];
     const sort_method =
         event.ctrlKey ||
-        (event as MouseEvent & { metaKet?: boolean }).metaKet ||
+        (event as MouseEvent & { metaKey?: boolean }).metaKey ||
         event.altKey
             ? append_sort
             : override_sort;
@@ -97,6 +97,7 @@ export function override_sort(
             return sort ? [sort] : [];
         }
     }
+
     return [[column_name, abs ? "desc abs" : "desc"]];
 }
 
@@ -111,8 +112,10 @@ export function create_sort(
     const inc_sort_dir: SortDir | undefined = sort_dir
         ? order[sort_dir]
         : "desc";
+
     if (inc_sort_dir) {
         return [column_name, inc_sort_dir];
     }
+
     return undefined;
 }

packages/viewer-datagrid/src/ts/model/create.ts

@@ -87,21 +87,21 @@ export async function createModel(
         }
 
         let split_by_changed = old.split_by.length !== config.split_by.length;
-        if (split_by_changed) {
+        if (!split_by_changed) {
             for (const lvl in old.split_by) {
                 split_by_changed ||= config.split_by[lvl] !== old.split_by[lvl];
             }
         }
 
         let columns_changed = old.columns.length !== config.columns.length;
-        if (columns_changed) {
+        if (!columns_changed) {
             for (const lvl in old.columns) {
                 columns_changed ||= config.columns[lvl] !== old.columns[lvl];
             }
         }
 
         let filter_changed = old.filter.length !== config.filter.length;
-        if (filter_changed) {
+        if (!filter_changed) {
             for (const lvl in old.filter) {
                 for (const i in config.filter[lvl]) {
                     filter_changed ||=
@@ -112,7 +112,7 @@ export async function createModel(
         }
 
         let sort_changed = old.sort.length !== config.sort.length;
-        if (sort_changed) {
+        if (!sort_changed) {
             for (const lvl in old.sort) {
                 for (const i in config.sort[lvl]) {
                     sort_changed ||=

rust/perspective-js/test/js/group_rollup_mode.spec.js

@@ -585,6 +585,23 @@ const data = {
                 table.delete();
             });
 
+            test("with split_by schema", async function () {
+                const table = await perspective.table(data);
+                const view = await table.view({
+                    split_by: ["z"],
+                    group_rollup_mode: "total",
+                });
+                const schema = await view.schema();
+                expect(schema).toStrictEqual({
+                    w: "float",
+                    x: "integer",
+                    y: "integer",
+                    z: "integer",
+                });
+                view.delete();
+                table.delete();
+            });
+
             test("updates after table.update()", async function () {
                 const table = await perspective.table(data);
                 const view = await table.view({

rust/perspective-server/cpp/perspective/src/cpp/view.cpp

@@ -461,7 +461,7 @@ View<CTX_T>::schema() const {
         std::string type_string = dtype_to_str(types[agg_name]);
         new_schema[agg_name] = type_string;
 
-        if ((!m_row_pivots.empty() || m_view_config->is_total_only()) && !is_column_only()) {
+        if ((!m_row_pivots.empty() || m_view_config->is_total_only()) && (!is_column_only() || m_view_config->is_total_only())) {
             new_schema[agg_name] =
                 _map_aggregate_types(agg_name, new_schema[agg_name]);
         }
@@ -540,7 +540,7 @@ View<CTX_T>::expression_schema() const {
         const std::string& expression_alias = expr->get_expression_alias();
         new_schema[expression_alias] = dtype_to_str(expr->get_dtype());
 
-        if ((!m_row_pivots.empty() || m_view_config->is_total_only()) && !is_column_only()) {
+        if ((!m_row_pivots.empty() || m_view_config->is_total_only()) && (!is_column_only() || m_view_config->is_total_only())) {
             new_schema[expression_alias] = _map_aggregate_types(
                 expression_alias, new_schema[expression_alias]
             );