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.

Sign up for GitHub

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

Improve search in large diagrams #931

Merged
merged 5 commits into from
Sep 25, 2024
Merged

Improve search in large diagrams #931

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
58e3a20
fix(canvas): return copy of viewbox
philippfromme Sep 23, 2024
12fea64
fix(search-pad): results is an object, not an array
philippfromme Sep 23, 2024
9e36453
feat(search-pad): add custom marker to preselected element
philippfromme Sep 23, 2024
1f93932
chore(search-pad): do not set zoom to 1
philippfromme Sep 23, 2024
2612d06
chore(seach-pad): reduce padding
philippfromme Sep 23, 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
5 changes: 5 additions & 0 deletions assets/diagram-js.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@
--search-input-focus-background-color: var(--color-blue-205-100-95);
--search-result-hover-background-color: var(--color-grey-225-10-95);
--search-result-secondary-color: var(--color-grey-225-10-55);
--search-preselected-background-color: var(--color-blue-205-100-50-opacity-15);

--shape-attach-allowed-stroke-color: var(--color-blue-205-100-50);
--shape-connect-allowed-fill-color: var(--color-grey-225-10-97);
Expand Down Expand Up @@ -979,6 +980,10 @@ svg.new-parent {
background: var(--search-result-hover-background-color);
}

.djs-element.djs-search-preselected .djs-outline {
fill: var(--search-preselected-background-color) !important;
}

/**
* hidden styles
*/
Expand Down
2 changes: 1 addition & 1 deletion lib/core/Canvas.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could be a follow-up improvement.

Copy link
Member

Choose a reason for hiding this comment

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

Simple clone:

{ ...viewbox }

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

}

const viewport = this._viewport,
Expand Down
33 changes: 27 additions & 6 deletions lib/features/search-pad/SearchPad.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ import { isKey } from '../keyboard/KeyboardUtil';
* @typedef {import('./SearchPadProvider').Token} Token
*/

var SCROLL_TO_ELEMENT_PADDING = 300;

/**
* Provides searching infrastructure.
*
Expand All @@ -36,7 +38,7 @@ import { isKey } from '../keyboard/KeyboardUtil';
*/
export default function SearchPad(canvas, eventBus, selection, translate) {
this._open = false;
this._results = [];
this._results = {};
this._eventMaps = [];

this._cachedRootElement = null;
Expand Down Expand Up @@ -190,6 +192,9 @@ SearchPad.prototype._search = function(pattern) {
});

if (!searchResults.length) {
this._clearMarkers();
this._selection.select(null);

return;
}

Expand Down Expand Up @@ -258,12 +263,22 @@ SearchPad.prototype._scrollToNode = function(node) {
SearchPad.prototype._clearResults = function() {
domClear(this._resultsContainer);

this._results = [];
this._results = {};

this._eventBus.fire('searchPad.cleared');
};


/**
* Clears all markers.
*/
SearchPad.prototype._clearMarkers = function() {
for (var id in this._results) {
this._canvas.removeMarker(this._results[id].element, 'djs-search-preselected');
}
};


/**
* Get currently selected result.
*
Expand Down Expand Up @@ -380,6 +395,8 @@ SearchPad.prototype.close = function(restoreCached = true) {
domClasses(this._canvas.getContainer()).remove('djs-search-open');
domClasses(this._container).remove('open');

this._clearMarkers();

this._clearResults();

this._searchInput.value = '';
Expand Down Expand Up @@ -418,6 +435,8 @@ SearchPad.prototype._preselect = function(node) {
return;
}

this._clearMarkers();

// removing preselection from current node
if (selectedNode) {
domClasses(selectedNode).remove(SearchPad.RESULT_SELECTED_CLASS);
Expand All @@ -428,14 +447,14 @@ SearchPad.prototype._preselect = function(node) {

domClasses(node).add(SearchPad.RESULT_SELECTED_CLASS);

this._canvas.zoom(1);

this._canvas.scrollToElement(element, {
top: 300
top: SCROLL_TO_ELEMENT_PADDING
});

this._selection.select(element);

this._canvas.addMarker(element, 'djs-search-preselected');

this._eventBus.fire('searchPad.preselected', element);
};

Expand All @@ -454,7 +473,9 @@ SearchPad.prototype._select = function(node) {

this.close(false);

this._canvas.scrollToElement(element, { top: 400 });
this._canvas.scrollToElement(element, {
top: SCROLL_TO_ELEMENT_PADDING
});

this._selection.select(element);

Expand Down
19 changes: 19 additions & 0 deletions test/spec/core/CanvasSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -867,6 +867,25 @@ describe('Canvas', function() {
}));


it('should return copy of viewbox', inject(function(canvas) {

// given
canvas.addShape({ id: 's0', x: 0, y: 0, width: 300, height: 300 });

var shape = canvas.addShape({ id: 's1', x: 10000, y: 0, width: 300, height: 300 });

var viewbox = canvas.viewbox(),
viewboxStringified = JSON.stringify(viewbox);

// when
canvas.scrollToElement(shape);

// then
expect(JSON.stringify(viewbox)).to.eql(viewboxStringified);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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());

?

expect(JSON.stringify(canvas.viewbox())).not.to.eql(viewboxStringified);
}));


it('should provide default viewbox / overflowing diagram', inject(function(canvas) {

// given
Expand Down
32 changes: 12 additions & 20 deletions test/spec/features/search-pad/SearchPadSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,7 @@ describe('features/searchPad', function() {
}));


it('should preselect first result', inject(function(canvas) {
it('should preselect first result', inject(function(canvas, selection) {

// when
typeText(input_node, 'two');
Expand All @@ -371,10 +371,12 @@ describe('features/searchPad', function() {
var result_nodes = domQueryAll(SearchPad.RESULT_SELECTOR, canvas.getContainer());
expect(domClasses(result_nodes[0]).has(SearchPad.RESULT_SELECTED_CLASS)).to.be.true;
expect(capturedEvents).to.eql([ EVENTS.opened, EVENTS.preselected ]);
expect(selection.isSelected(elements.two.a)).to.be.true;
expect(domClasses(canvas.getGraphics(elements.two.a)).has('djs-search-preselected')).to.be.true;
}));


it('should select result on enter', function() {
it('should select result on enter', inject(function(canvas, selection) {

// given
typeText(input_node, 'two');
Expand All @@ -389,10 +391,13 @@ describe('features/searchPad', function() {
EVENTS.closed,
EVENTS.selected
]);
});

expect(selection.isSelected(elements.two.a)).to.be.true;
expect(domClasses(canvas.getGraphics(elements.two.a)).has('djs-search-preselected')).to.be.false;
}));


it('should reset selection on escape without enter', inject(function(selection) {
it('should reset selection on escape without enter', inject(function(canvas, selection) {

// given
selection.select(elements.one.a);
Expand All @@ -406,6 +411,8 @@ describe('features/searchPad', function() {

// then
expect(selection.isSelected(elements.one.a)).to.be.true;

expect(domClasses(canvas.getGraphics(elements.two.a)).has('djs-search-preselected')).to.be.false;
}));


Expand All @@ -431,22 +438,7 @@ describe('features/searchPad', function() {
// then
var newViewbox = canvas.viewbox();
expect(newViewbox).to.have.property('x', -100);
expect(newViewbox).to.have.property('y', -400);
}));


it('select set zoom level to 1', inject(function(canvas) {

// given
canvas.zoom(0.4);

typeText(input_node, 'one');

// when
triggerKeyEvent(input_node, 'keyup', 'Enter');

// then
expect(canvas.zoom()).to.equal(1);
expect(newViewbox).to.have.property('y', -300);
}));


Expand Down
Loading