Skip to content
Open
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
41 changes: 39 additions & 2 deletions ASCIIToSVG.php
Original file line number Diff line number Diff line change
Expand Up @@ -273,17 +273,19 @@ class SVGGroup {
private $curGroup;
private $groupStack;
private $options;
private $index;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Please remove this, more information in a later comment.


public function __construct() {
$this->groups = array();
$this->groupStack = array();
$this->options = array();
$this->index = 0;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Please remove this, more information in a later comment.

}

public function getGroup($groupName) {
return $this->groups[$groupName];
}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Please remove this whitespace change.

public function pushGroup($groupName) {
if (!isset($this->groups[$groupName])) {
$this->groups[$groupName] = array();
Expand All @@ -304,10 +306,44 @@ public function popGroup() {
$this->curGroup = array_pop($this->groupStack);
}

/*
AAVZZ
Since SVG has no notion of layers, we emulate them by changing the order
of boxes. This involves tagging and reordering. Tagging is done in `addObject',
reordering in `rearrangeBoxes'. We tag a box with a layer prefix, like `a' (default),
`b', `c' and so on. So, the sequence numbers will be something like
a0, a1, b2, a3, a4, b5, c6 ...
Once all the boxes are parsed, we need to rearrange them like
a0, a1, a3, a4, b2, b5, c6 ...
and change the indices to be numeic (we rely on them being numeric!!)
XXX Some text lines belonging to overlying box are placed in underlying box
XXX and do not show up. Further investigation is necessary to place them correctly.
XXX This issue is not introduced with this patch.
*/

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Can you please make this comment block follow the style of other comment blocks in this file? For example:

/* 
 * Since SVG has no notion of layers...

Please remove your github username from the comment. It isn't informative to the code or the comment and this ends up being distracting when reading the code.

Please only use one instance of XXX in the comment. E.g.

 * XXX: Some text lines ...
 * and do not show up...
 */

If you intend on solving this problem, please feel free to change the XXX: to TODO(aavzz):

Please also fix the contents of this comment based on the discussion about indexing below.


Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Please remove this line, to follow style with other comments on class methods in this code.

public function addObject($o) {
$this->groups[$this->curGroup][] = $o;
if ($this->curGroup == "boxes") {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Although this is probably only useful for layering boxes, I'd recommend removing this conditional such that it may be possible to annotate lines and text in the future, and have those layered appropriately.

$layer = 'a';
if ($o->getOption('a2s:layer')) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I would recommend renaming this to a2s:z-index so that it feels more like specifying a CSS property and so that we can better define the stacking behavior (more on that in the next comment). Can you please document this option in the README as part of this PR?

$layer = $o->getOption('a2s:layer');
}
$i = $layer . $this->index++;
$this->groups[$this->curGroup][$i] = $o;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The code above ends up calling getOption twice and uses $this->curGroup when it is already known to be 'boxes' based on the conditional. (That's another reason to remove the condition.)

Another thing worth thinking about: if you do not layer all of your objects, you start to need to come up with creative names for layers since they are sorted lexicographically. If I want to stack my boxes below the default layer, I need to use uppercase names, some kinds of punctuation, or numbers for the start of the layer name. If I want to stack my boxes above the default layer, I can't use those things for layer names.

I would recommend that we use a deterministic ordering, hence switching to a2s:z-index. Simply assign a number. Higher numbers mean closer to the front, lower numbers mean further to the back. This also removes the need to introduce the $index variable.

This can all be expressed in a single line of code which can replace most of this change.

$this->groups[$this->curGroup][$o->getOption('a2s:z-index') ?: 0];

}
else {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Please remove this else clause based on the above comments.

$this->groups[$this->curGroup][] = $o;
}
}

public function rearrangeBoxes() {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I don't think this needs to be a public method. In particular, you can perform the sort just-in-time before you render the group. Please move the layer sorting logic into the render method of this class.

$newboxes = array();
ksort($this->groups["boxes"], SORT_NATURAL);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Per the PHP manual, array sorting functions are in-place. As such, the loop and re-assignment below are not necessary.

Since this will be moving into render, and since we should be switching to a numeric z-index, I would recommend changing this line to:

ksort($this->groups[$groupName], SORT_NUMERIC);

foreach($this->groups["boxes"] as $index => $value) {
$newboxes[] = $value;
}
$this->groups["boxes"]=$newboxes;
}

public function setOption($opt, $val) {
$this->options[$this->curGroup][$opt] = $val;
}
Expand Down Expand Up @@ -1480,6 +1516,7 @@ private function parseBoxes() {

/* Anything after this is not a subgroup */
$this->svgObjects->popGroup();
$this->svgObjects->rearrangeBoxes();

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Please remove this line as the logic performed by this call should instead just be performed by SVGGroup::render.

}

/*
Expand Down