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

ISSUE-6566: Fix SVGs for special Arc lines #6571

Merged
merged 8 commits into from
Sep 7, 2020
Merged

ISSUE-6566: Fix SVGs for special Arc lines #6571

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6964c17
ISSUE-6566: Fix SVGs so for special Arc lines
gloriousjob Sep 4, 2020
ee48d93
ISSUE-6566: Fix lint issues
gloriousjob Sep 4, 2020
bd681d9
ISSUE-6566: Reduce variable
gloriousjob Sep 4, 2020
7baf58a
ISSUE-6566: Add test for weird arc values in SVG
gloriousjob Sep 4, 2020
59985ed
ISSUE-6566: Fix lint
gloriousjob Sep 4, 2020
b69f9c5
Add vscode to gitignore list
gloriousjob Sep 7, 2020
40447f3
Consolidate commaWsp
gloriousjob Sep 7, 2020
173e270
Simplify unit test
gloriousjob Sep 7, 2020
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
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
.DS_Store
*.iml
/.nyc_output/
/.vscode/
/node_modules/
/npm-debug.log
before_commit
Expand Down
1 change: 1 addition & 0 deletions HEADER.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ fabric.SHARED_ATTRIBUTES = [
*/
fabric.DPI = 96;
fabric.reNum = '(?:[-+]?(?:\\d+|\\d*\\.\\d+)(?:[eE][-+]?\\d+)?)';
fabric.commaWsp = '(?:\\s+,?\\s*|,\\s*)'
fabric.rePathCommand = /([-+]?((\d+\.\d+)|((\d+)|(\.\d+)))(?:[eE][-+]?\d+)?)/ig;
fabric.reNonWord = /[ \n\.,;!\?\-]/;
fabric.fontPaths = { };
Expand Down
2 changes: 1 addition & 1 deletion src/parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@
// == begin transform regexp
number = fabric.reNum,

commaWsp = '(?:\\s+,?\\s*|,\\s*)',
commaWsp = fabric.commaWsp,

skewX = '(?:(skewX)\\s*\\(\\s*(' + number + ')\\s*\\))',

Expand Down
28 changes: 22 additions & 6 deletions src/util/path.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -590,6 +590,12 @@
currentPath,
parsed,
re = fabric.rePathCommand,
rNumber = '[-+]?(?:\\d*\\.\\d+|\\d+\\.?)(?:[eE][-+]?\\d+)?\\s*',
rNumberCommaWsp = '(' + rNumber + ')' + fabric.commaWsp,
rFlagCommaWsp = '([01])' + fabric.commaWsp + '?',
rArcSeq = rNumberCommaWsp + '?' + rNumberCommaWsp + '?' + rNumberCommaWsp + rFlagCommaWsp + rFlagCommaWsp +
rNumberCommaWsp + '?(' + rNumber + ')',
regArcArgumentSequence = new RegExp(rArcSeq, 'g'),
match,
coordsStr,
// one of commands (m,M,l,L,q,Q,c,C,etc.) followed by non-command characters (i.e. command values)
Expand All @@ -605,11 +611,22 @@
coordsStr = currentPath.slice(1).trim();
coords.length = 0;

while ((match = re.exec(coordsStr))) {
coords.push(match[0]);
}
var command = currentPath.charAt(0);
coordsParsed = [command];

coordsParsed = [currentPath.charAt(0)];
if (command.toLowerCase() === 'a') {
// arcs have special flags that apparently don't require spaces so handle special
for (var args; (args = regArcArgumentSequence.exec(coordsStr));) {
for (var j = 1; j < args.length; j++) {
coords.push(args[j]);
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

can be this written like:

if (command.toLowerCase() === 'a') {
  // arcs have special flags that apparently don't require spaces so handle special
  coords = regArcArgumentSequence.exec(coordsStr);
}
else {
  coords = [];
  while ((match = re.exec(coordsStr))) {
     coords.push(match[0]);
  }
}

would it work?

Copy link
Member

Choose a reason for hiding this comment

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

line 613
https://github.com/fabricjs/fabric.js/pull/6571/files#diff-b4a17ab08e7f28e3bf83badb9236d01cR613
could go away

and so the pre declaration here would be just
https://github.com/fabricjs/fabric.js/pull/6571/files#diff-b4a17ab08e7f28e3bf83badb9236d01cR589

var coords,

thre is no point to reuse the same array this is not hot code to optimize.

Copy link
Contributor Author

@gloriousjob gloriousjob Sep 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This failed unit tests when I tried it. An arc can have multiple strings of numbers behind it so there needs to be a loop. Also, the exec returns an array that has [0] with a list of matches and then [1..n] with the matches (hence why j starts at 1). Maybe it can be reworked but I'm not sure there's a gain there.

Are you saying you don't like 'coords.length = 0' and would rather have 'coords = []' at 613? I didn't care for the former. Or are you saying you want 613 to be 'var coords = [];' and remove the top declaration? Or do you have other concerns as well?

else {
while ((match = re.exec(coordsStr))) {
coords.push(match[0]);
}
}

for (var j = 0, jlen = coords.length; j < jlen; j++) {
parsed = parseFloat(coords[j]);
Expand All @@ -618,8 +635,7 @@
}
}

var command = coordsParsed[0],
commandLength = commandLengths[command.toLowerCase()],
var commandLength = commandLengths[command.toLowerCase()],
repeatedCommand = repeatedCommands[command] || command;

if (coordsParsed.length - 1 > commandLength) {
Expand Down
4 changes: 4 additions & 0 deletions test/fixtures/f_blue.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
8 changes: 8 additions & 0 deletions test/unit/path_utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,14 @@
assert.deepEqual(command, expectedSimplified[index], 'should contain a subset of equivalent commands ' + index);
});
});
QUnit.test('fabric.util.parsePath can parse arcs correctly when no spaces between flags', function(assert) {
// eslint-disable-next-line max-len
var pathWithWeirdArc = 'a10.56 10.56 0 00-1.484-.133';
var expected = ['a', 10.56, 10.56, 0, 0, 0, -1.484, -0.133];
var parsed = fabric.util.parsePath(pathWithWeirdArc);
var command = parsed[0];
assert.deepEqual(command, expected, 'Arc should be parsed correctly.');
});
QUnit.test('fabric.util.getPathSegmentsInfo', function(assert) {
assert.ok(typeof fabric.util.getPathSegmentsInfo === 'function');
var parsed = fabric.util.makePathSimpler(fabric.util.parsePath(path));
Expand Down