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

fix(context-pad): do not re-open for targets that were removed #893

Merged
merged 1 commit into from
May 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
40 changes: 27 additions & 13 deletions lib/features/context-pad/ContextPad.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@ import {
every,
forEach,
isArray,
isFunction,
some
isFunction
} from 'min-dash';

import {
Expand All @@ -29,6 +28,7 @@ import { isConnection } from '../../util/ModelUtil';
* @typedef {import('../../util/Types').RectTRBL} RectTRBL
*
* @typedef {import('../../core/Canvas').default} Canvas
* @typedef {import('../../core/ElementRegistry').default} ElementRegistry
* @typedef {import('../../core/EventBus').default} EventBus
*
* @typedef {import('./ContextPadProvider').default} ContextPadProvider
Expand All @@ -53,11 +53,13 @@ var HOVER_DELAY = 300;
* to a diagram element.
*
* @param {Canvas} canvas
* @param {ElementRegistry} elementRegistry
* @param {EventBus} eventBus
*/
export default function ContextPad(canvas, eventBus) {
export default function ContextPad(canvas, elementRegistry, eventBus) {

this._canvas = canvas;
this._elementRegistry = elementRegistry;
this._eventBus = eventBus;

this._current = null;
Expand All @@ -67,6 +69,7 @@ export default function ContextPad(canvas, eventBus) {

ContextPad.$inject = [
'canvas',
'elementRegistry',
'eventBus'
];

Expand Down Expand Up @@ -102,18 +105,29 @@ ContextPad.prototype._init = function() {
return;
}

var currentTarget = current.target;
var { target } = current;

var currentChanged = some(
isArray(currentTarget) ? currentTarget : [ currentTarget ],
function(element) {
return includes(elements, element);
}
);
var targets = isArray(target) ? target : [ target ];

var targetsChanged = targets.filter(function(element) {
return includes(elements, element);
});

if (targetsChanged.length) {

// re-open if elements in current selection changed
if (currentChanged) {
self.open(currentTarget, true);
// (1) close
self.close();

var targetsNew = targets.filter(function(element) {
return self._elementRegistry.get(element.id);
});

if (targetsNew.length) {

// (2) re-open with new targets being all previous targets that still
// exist
self._updateAndOpen(targetsNew.length > 1 ? targetsNew : targetsNew[ 0 ]);
}
}
});

Expand Down
75 changes: 67 additions & 8 deletions test/spec/features/context-pad/ContextPadSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,7 @@ describe('features/context-pad', function() {
}));


it('open and hidden', inject(function(canvas, contextPad, eventBus) {
it('open and hidden', inject(function(canvas, contextPad) {

// given
var shape = { id: 's1', width: 100, height: 100, x: 10, y: 10 };
Expand All @@ -407,7 +407,7 @@ describe('features/context-pad', function() {
}));


it('closed', inject(function(canvas, contextPad, eventBus) {
it('closed', inject(function(canvas, contextPad) {

// given
var shape = { id: 's1', width: 100, height: 100, x: 10, y: 10 };
Expand Down Expand Up @@ -575,8 +575,7 @@ describe('features/context-pad', function() {
contextPad.close();

// then
expect(overlays.get({ element: shape })).to.have.length(0);
expect(!!contextPad.isOpen()).to.be.false;
expect(contextPad.isOpen()).to.be.false;
}));


Expand Down Expand Up @@ -654,10 +653,20 @@ describe('features/context-pad', function() {
});
}

function remove(elements) {
getDiagramJS().invoke(function(canvas) {
(isArray(elements) ? elements : [ elements ]).forEach(function(element) {
canvas.removeShape(element);
});
});

change(elements);
Copy link
Member

@nikku nikku May 7, 2024

Choose a reason for hiding this comment

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

Do we have this behavior covered in ChangeSupport specs?

I'd be afraid that we otherwise mock something that we don't even guarantee.

Copy link
Contributor Author

@philippfromme philippfromme May 7, 2024

Choose a reason for hiding this comment

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

elements.changed is always fired even when an element was deleted (using a command). GraphicsFactory then updates all parents. Since deleted elements don't have parents anymore they're ignored. shape.changed and connection.changed aren't fired when elements are deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We did handle delete explicitly in the context pad previously but changed it when the multi-element context pad was introduced.

Copy link
Member

Choose a reason for hiding this comment

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

Great explaination, thanks a lot. 🏅

Copy link
Member

Choose a reason for hiding this comment

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

Great catch (with this PR).

}


describe('should handle changed', function() {

it('single target', inject(function(eventBus, canvas, contextPad) {
it('single target', inject(function(canvas, contextPad) {

// given
var shape_1 = { id: 's1', width: 100, height: 100, x: 10, y: 10 };
Expand All @@ -678,7 +687,7 @@ describe('features/context-pad', function() {
}));


it('multiple targets', inject(function(eventBus, canvas, contextPad) {
it('multiple targets', inject(function(canvas, contextPad) {

// given
var shape_1 = { id: 's1', width: 100, height: 100, x: 10, y: 10 };
Expand All @@ -703,9 +712,59 @@ describe('features/context-pad', function() {
});


describe('should handle removed', function() {

it('some removed', inject(function(canvas, contextPad) {

// given
var shape_1 = { id: 's1', width: 100, height: 100, x: 10, y: 10 };
var shape_2 = { id: 's2', width: 50, height: 50, x: 210, y: 10 };

canvas.addShape(shape_1);
canvas.addShape(shape_2);

// assume
contextPad.open([ shape_1, shape_2 ]);

// then
expectOpened([ shape_1, shape_2 ]);

// when
remove(shape_1);

// then
expectOpened(shape_2);
}));


it('all removed', inject(function(canvas, contextPad) {

// given
var shape_1 = { id: 's1', width: 100, height: 100, x: 10, y: 10 };
var shape_2 = { id: 's2', width: 50, height: 50, x: 210, y: 10 };

canvas.addShape(shape_1);
canvas.addShape(shape_2);

// assume
contextPad.open([ shape_1, shape_2 ]);

// then
expectOpened([ shape_1, shape_2 ]);

// when
remove([ shape_1, shape_2 ]);

// then
expectClosed();
}));

});


describe('should ignore unrelated', function() {

it('single target', inject(function(eventBus, canvas, contextPad) {
it('single target', inject(function(canvas, contextPad) {

// given
var shape_1 = { id: 's1', width: 100, height: 100, x: 10, y: 10 };
Expand All @@ -728,7 +787,7 @@ describe('features/context-pad', function() {
}));


it('multiple targets', inject(function(eventBus, canvas, contextPad) {
it('multiple targets', inject(function(canvas, contextPad) {

// given
var shape_1 = { id: 's1', width: 100, height: 100, x: 10, y: 10 };
Expand Down