Improve search in large diagrams #931

philippfromme · 2024-09-23T12:32:46Z

Proposed Changes

do not set zoom to 1
reduce padding below search
add custom marker to preselected element

Related to bpmn-io/bpmn-js#2233
Related to camunda/camunda-modeler#4538

Checklist

To ensure you provided everything we need to look at your PR:

Brief textual description of the changes present
Visual demo attached
Steps to try out present, i.e. using the @bpmn-io/sr tool
Related issue linked via Closes {LINK_TO_ISSUE} or Related to {LINK_TO_ISSUE}

Returning reference to viewbox can lead to undesired effects.

Related to camunda/camunda-modeler#4538

Related to bpmn-io/bpmn-js#2233

nikku · 2024-09-25T16:43:26Z

Try out via

npx @bpmn-io/sr bpmn-io/bpmn-js -l bpmn-io/diagram-js#search-improvements-next

nikku

Works well. Thanks for following up.

nikku · 2024-09-25T16:50:25Z

lib/core/Canvas.js

@@ -1153,7 +1153,7 @@ Canvas.prototype._viewboxChanged = function() {
 Canvas.prototype.viewbox = function(box) {

  if (box === undefined && this._cachedViewbox) {
-    return this._cachedViewbox;
+    return JSON.parse(JSON.stringify(this._cachedViewbox));


What is the issue here, could you elaborate? To me this looks like a hack. The cached viewbox, after all, should not be fiddled with.

The problem is when you return a reference to the viewbox to the caller of viewbox() they will assume that it's not going to be mutated. This is an issue I ran into. I called viewbox() to then used the viewbox at a later point just to realize it had been changed.

BTW there is a dedicated API for deep cloning: https://developer.mozilla.org/en-US/docs/Web/API/Window/structuredClone

Could be a follow-up improvement.

Simple clone:

{ ...viewbox }

If this needs to return a copy, a shallow clone won't work due to nested objects (inner and friends).

I'll need to figure why this needs a copy in the first place. We always construct a fresh viewbox., and the cached viewbox is not to be fiddled with.

@philippfromme Still looking for a concrete scenario where users would mutate the viewbox, and would love to see that as a test.

To prevent mutation (which does not make sense anyway), we can freeze the object, too.

nikku · 2024-09-25T16:51:45Z

test/spec/core/CanvasSpec.js

+        canvas.scrollToElement(shape);
+
+        // then
+        expect(JSON.stringify(viewbox)).to.eql(viewboxStringified);


I don't understand the test either 🙈. If we want the to verify "no identity", why don't we test for "not equal"?

I'm testing whether the viewbox I got from calling viewbox() has been changed. If you remove the JSON.parse(JSON.stringify()) part you'll see the test failing.

A much simpler test would be:

// mind the "strict object equality" expect(canvas.viewbox()).not.to.equal(canvas.viewbox())

But

// mind the loose object comparison expect(canvas.viewbox()).to.eql(canvas.viewbox());

?

nikku · 2024-09-26T09:02:53Z

@GeethaParthasarathy FYI :)

# Changes Siehe Changelog: https://github.com/bpmn-io/diagram-js/blob/develop/CHANGELOG.md ## 15.2.4 * `FIX`: canvas `autoFocus` must explicitly be enabled ([bpmn-io#956](bpmn-io#956)) * `FIX`: properly integrate `zoomscroll` with canvas focus ([bpmn-io#956](bpmn-io#956)) * `FIX`: properly integrate `movecanvas` with canvas focus ([bpmn-io#956](bpmn-io#956)) ## 15.2.3 * `FIX`: adjust search to prioritize start of word and exact matches ([bpmn-io#953](bpmn-io#953)) * `FIX`: ignore whitespace when searching ([bpmn-io#954](bpmn-io#954)) ## 15.2.2 * `FIX`: correct `Keyboard#bind` and config types ([bpmn-io#948](bpmn-io#948)) ## 15.2.1 * `FIX`: limit overly permissive regex ([bpmn-io#949](bpmn-io#949)) ## 15.2.0 * `FIX`: clear selection when opening search pad ([bpmn-io#947](bpmn-io#947)) * `FIX`: correct dangling selection after search pad interaction ([bpmn-io#947](bpmn-io#947)) * `CHORE`: simplify search pad pre-selection behavior ([bpmn-io#947](bpmn-io#947)) ## 15.1.0 * `FEAT`: integrate `popup-menu` with `search` ([bpmn-io#932](bpmn-io#932)) * `FEAT`: recognize modern `search` tokens in `search-pad` ([bpmn-io#932](bpmn-io#932)) * `FEAT`: improve `search` types ([bpmn-io#932](bpmn-io#932)) * `FIX`: correctly handle duplicate entries and whitespace in `search` ([bpmn-io#932](bpmn-io#932)) * `FIX`: find `search` terms across all keys ([bpmn-io#932](bpmn-io#932)) * `FIX`: `search` always returns tokens for matched items ([bpmn-io#932](bpmn-io#932)) ## 15.0.0 * `FEAT`: make canvas browser selectable ([bpmn-io#659](bpmn-io#659)) * `FEAT`: make keyboard binding implicit ([bpmn-io#661](bpmn-io#661)) * `FEAT`: make multi-selection outline an outline concern ([bpmn-io#944](bpmn-io#944)) ### Breaking Changes * `Keyboard` binding target can no longer be chosen. Configure keyboard binding via the `keyboard.bind` configuration and rely on keybindings to work if the canvas has browser focus. ([bpmn-io#661](bpmn-io#661)) * The `Canvas` is now a focusable component, that is recognized accordingly by the browser, with all benefits for UX and interaction. Components that pull focus from the `Canvas` during modeling must ensure to restore the focus (if intended), via `Canvas#restoreFocus`. ([bpmn-io#661](bpmn-io#661)) * The `selection` feature does not provide visual outline by default anymore. Use the `outline` feature to re-enable it. ([bpmn-io#944](bpmn-io#944)) ## 14.11.3 * `CHORE`: simplify viewbox cloning ([bpmn-io#935](bpmn-io#935)) ## 14.11.2 * `FIX`: restore search result highlight ([bpmn-io#931](bpmn-io#931)) * `FIX`: correct search result highlight not being removed ([bpmn-io#931](bpmn-io#931)) * `FIX`: do not change zoom when search openes ([bpmn-io#931](bpmn-io#931)) ## 14.11.1 _Partially reverts v14.11.0._ * `FIX`: revert `search` integration into popup menu ## 14.11.0 * `FEAT`: add `search` utility * `FEAT`: sort popup entry search results semantically ([bpmn-io#916](bpmn-io#916)) ## 14.10.0 * `FEAT`: align search styling with other popups ([bpmn-io#913](bpmn-io#913)) * `CHORE`: use existing outline in search ([bpmn-io#913](bpmn-io#913)) * `FIX`: only commit search viewport changes on `ENTER` ([bpmn-io#913](bpmn-io#913)) ## 14.9.0 * `CHORE`: export types compatible with `verbatimModuleSyntax` ([bpmn-io#927](bpmn-io#927), [bpmn-io#864](bpmn-io#864)) * `CHORE`: re-compute context pad position next frame ([bpmn-io#920](bpmn-io#920))

bpmn-io-tasks bot added the needs review Review pending label Sep 23, 2024

bpmn-io-tasks bot assigned philippfromme Sep 23, 2024

philippfromme force-pushed the search-improvements-next branch from 56a2ca0 to 16f13c2 Compare September 23, 2024 12:33

philippfromme requested review from nikku and barmac September 23, 2024 12:34

philippfromme mentioned this pull request Sep 24, 2024

When you have a search term that does not resolve it still highlights the element that was previously searched upon. camunda/camunda-modeler#4538

Closed

philippfromme force-pushed the search-improvements-next branch from 16f13c2 to fa36b90 Compare September 24, 2024 09:55

philippfromme added 5 commits September 25, 2024 18:42

fix(canvas): return copy of viewbox

58e3a20

Returning reference to viewbox can lead to undesired effects.

fix(search-pad): results is an object, not an array

12fea64

feat(search-pad): add custom marker to preselected element

9e36453

Related to camunda/camunda-modeler#4538

chore(search-pad): do not set zoom to 1

1f93932

chore(seach-pad): reduce padding

2612d06

Related to bpmn-io/bpmn-js#2233

nikku force-pushed the search-improvements-next branch from fa36b90 to 2612d06 Compare September 25, 2024 16:42

nikku requested a review from lmbateman September 25, 2024 16:42

nikku approved these changes Sep 25, 2024

View reviewed changes

nikku merged commit 430bc0a into develop Sep 25, 2024
8 of 10 checks passed

bpmn-io-tasks bot removed the needs review Review pending label Sep 25, 2024

nikku deleted the search-improvements-next branch September 25, 2024 16:47

nikku reviewed Sep 25, 2024

View reviewed changes

nikku requested review from nikku and GeethaParthasarathy September 26, 2024 09:02

bpmn-io-tasks bot added the needs review Review pending label Sep 26, 2024

This was referenced Sep 27, 2024

Searching makes diagram shift down in the canvas camunda/camunda-modeler#4547

Closed

chore: simplify viewbox cloning #935

Merged

jarekdanielak mentioned this pull request Oct 29, 2024

Release camunda-modeler v5.29.0 camunda/camunda-modeler#4594

Closed

38 tasks

ElRaptorus mentioned this pull request Jan 9, 2025

Bump to 15.2.4 5minds/diagram-js#6

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve search in large diagrams #931

Improve search in large diagrams #931

philippfromme commented Sep 23, 2024 •

edited by nikku

Loading

nikku commented Sep 25, 2024

nikku left a comment

nikku Sep 25, 2024

philippfromme Sep 27, 2024

barmac Sep 27, 2024

philippfromme Sep 27, 2024

nikku Sep 27, 2024

barmac Sep 27, 2024

nikku Sep 27, 2024

nikku Sep 25, 2024

philippfromme Sep 27, 2024

nikku Sep 27, 2024

nikku commented Sep 26, 2024

Improve search in large diagrams #931

Improve search in large diagrams #931

Conversation

philippfromme commented Sep 23, 2024 • edited by nikku Loading

Proposed Changes

Checklist

nikku commented Sep 25, 2024

nikku left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikku commented Sep 26, 2024

philippfromme commented Sep 23, 2024 •

edited by nikku

Loading