Add an option to selection objects only when 100% intersect #4508
Conversation
|
In the future this could easily be replaced by
|
src/canvas.class.js
Outdated
| @@ -178,6 +178,13 @@ | |||
| */ | |||
| selectionLineWidth: 1, | |||
|
|
|||
| /** | |||
| * Selection only shapes that intersect to 100% | |||
There was a problem hiding this comment.
Select only shapes that are fully contained in the dragged selection rectangle.
src/mixins/canvas_grouping.mixin.js
Outdated
| currentObject.containsPoint(selectionX1Y1) || | ||
| currentObject.containsPoint(selectionX2Y2) | ||
| (!this.selectionFullyContained && currentObject.containsPoint(selectionX1Y1)) || | ||
| (!this.selectionFullyContained && currentObject.containsPoint(selectionX2Y2)) |
There was a problem hiding this comment.
please put !this.selectionFullyContained in a var with a shorter name and with the negated name, helps reading. like var allowIntersect = !this.selectionFullyContained helps minification too. this.selectionFullyContained does not get minified.
|
this is usefull, but you have to write tests. |
|
If you look a bit around i think this was also surfaced in an issue if i remember correctly. |
|
I m a bit busy today, i ll leave an example of a test that may work, you can then for another from it. |
|
about tests: first you need to install fabric. this is the expected output. After you start from this point, since this is a selection test and fall in the Canvas case, check this file: at line 450 you find: QUnit.test('_collectObjects collects object contained in area', function(assert) {
var rect1 = new fabric.Rect({ width: 10, height: 10, top: 0, left: 0 });
var rect2 = new fabric.Rect({ width: 10, height: 10, top: 0, left: 10 });
var rect3 = new fabric.Rect({ width: 10, height: 10, top: 10, left: 0 });
var rect4 = new fabric.Rect({ width: 10, height: 10, top: 10, left: 10 });
canvas.add(rect1, rect2, rect3, rect4);
canvas._groupSelector = {
top: 15,
left: 15,
ex: 1,
ey: 1
};
var collected = canvas._collectObjects();
assert.equal(collected.length, 4, 'a rect that contains all objects collects them all');
assert.equal(collected[3], rect1, 'contains rect1 as last object');
assert.equal(collected[2], rect2, 'contains rect2');
assert.equal(collected[1], rect3, 'contains rect3');
assert.equal(collected[0], rect4, 'contains rect4 as first object');
});
QUnit.test('_collectObjects do not collects object if area is outside', function(assert) {
var rect1 = new fabric.Rect({ width: 10, height: 10, top: 0, left: 0 });
var rect2 = new fabric.Rect({ width: 10, height: 10, top: 0, left: 10 });
var rect3 = new fabric.Rect({ width: 10, height: 10, top: 10, left: 0 });
var rect4 = new fabric.Rect({ width: 10, height: 10, top: 10, left: 10 });
canvas.add(rect1, rect2, rect3, rect4);
canvas._groupSelector = {
top: 1,
left: 1,
ex: 24,
ey: 24
};
var collected = canvas._collectObjects();
assert.equal(collected.length, 0, 'a rect outside objects do not collect any of them');
});There are 4 existing tests that creates 4 rects ( for simplicity ) and that create a fake _groupSelector for testing purposes. What you have to do is test that with false those test do not change. Give tests a relevant title and also give asserts a string to identify them when they fail. run
|
|
ok added. but JQunit is really not my favorite to work with :( I tried to encapsulate my tests inside a module to add its own before/after but then the all canvas tests beforeEach/afterEach were not triggered anymore 😞 |
|
i do not like JQunit as well. i find jest or better in running, writing, stubbing / mocking But i learned it last year, and no one is going to rewrite this test suite probably |
…#4508) * Add an option to selection objects only when 100% intersect * Better description * Cache property negation * Add unit tests * Remove boundary tests, not specific to this option
Because today selection works as soon as an object intersects with the selection rect. but some use cases require a distance of 100% before a selection can happen.
A common pattern for these kind of option (like jquery.ui selectable) is a distance ratio but the tools classes around intersection computing does not yet return distance of intersection in % so a simple boolean to act like 0 or 100 is fine, non breaking (because false by default) and open the path to a future possibility to a distance ratio.
I'm just not sure about the naming, open to any suggestion on it.
close #3102