Update ASCIIToSVG.php#11
Conversation
Add more fine-grained control over which box is rendered above and which one is rendered below.
dhobsd
left a comment
dhobsd
left a comment
There was a problem hiding this comment.
Hi aavzz, thanks so much for this contribution! It's a good idea. I have several comments that I'd like to see addressed before merging. Many of them are stylistic, but there are some functional changes that I think would be very helpful as well.
| 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. | ||
| */ |
There was a problem hiding this comment.
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.
| $this->groups[$this->curGroup][] = $o; | ||
| if ($this->curGroup == "boxes") { | ||
| $layer = 'a'; | ||
| if ($o->getOption('a2s:layer')) { |
There was a problem hiding this comment.
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?
| $i = $layer . $this->index++; | ||
| $this->groups[$this->curGroup][$i] = $o; | ||
| } | ||
| else { |
There was a problem hiding this comment.
Please remove this else clause based on the above comments.
| $layer = $o->getOption('a2s:layer'); | ||
| } | ||
| $i = $layer . $this->index++; | ||
| $this->groups[$this->curGroup][$i] = $o; |
There was a problem hiding this comment.
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];
|
|
||
| public function addObject($o) { | ||
| $this->groups[$this->curGroup][] = $o; | ||
| if ($this->curGroup == "boxes") { |
There was a problem hiding this comment.
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.
| private $curGroup; | ||
| private $groupStack; | ||
| private $options; | ||
| private $index; |
There was a problem hiding this comment.
Please remove this, more information in a later comment.
| $this->groups = array(); | ||
| $this->groupStack = array(); | ||
| $this->options = array(); | ||
| $this->index = 0; |
There was a problem hiding this comment.
Please remove this, more information in a later comment.
| return $this->groups[$groupName]; | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Please remove this whitespace change.
| XXX and do not show up. Further investigation is necessary to place them correctly. | ||
| XXX This issue is not introduced with this patch. | ||
| */ | ||
|
|
There was a problem hiding this comment.
Please remove this line, to follow style with other comments on class methods in this code.
|
|
||
| public function rearrangeBoxes() { | ||
| $newboxes = array(); | ||
| ksort($this->groups["boxes"], SORT_NATURAL); |
There was a problem hiding this comment.
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);
2 participants
Add more fine-grained control over which box is rendered above and which one is rendered below.