Refactor class hierarchy, markup and ports

[ialokim]

This PR tries to simplify some of the core codebase by moving to the standard jointJS solution for ports, markup, initialization and styling (via attrs) and by using a slightly clearer gate hierarchy.

I'm aware this branch contains a lot of changed lines, but most of the changes are connected and could not be committed separately (e.g. addPort only works after the constructor has already been called). I will try to give a quick overview over the changes here, feel free to ask if something is unclear.

Fixes #13 by switching to the standard jointJS port solutions and a second move forwards towards #2 as several properties can now be changed including the bits and (at least some of) the gates should react on it properly.

• added a new gates.json file to examples to be able to see all ports in one paper
• constructor is replaced by initialize to be able to call addPort() while initializing gates (this is also the recommended way for backbone.js)
• this.listenTo(this, ...) calls have been replaced by this.on(...) (see backbone.js)
• as ports are no longer needed to be added to the markup inside the constructor, markup is moved to be a prototype property as recommended by jointJS and is thus shared between different instances of the same cell. Also markup has been migrated to use JSON markup style and is extended by sub-celltypes.
• the attrs are changed to use mostly relative dimensions (using jointJS ref* special attributes) to account for size changes automatically
• each cell type has been added respective default attributes s.th. every cell can be instantiated with no further arguments
• some styling which does not change during the cell lifetime has been moved from css to attrs
• each cell type claims now on which of its properties it currently does not react on changing (unsupportedPropChanges) and prints a warning to the console if one of these properties changes
• basic gates (e.g. arithmetic or logic ones) already successfully set their port bits on a bits property change (using setPortBits(), but still print a warning as connected wires are currently not handled

The last two points could be addressed in upcoming PRs.

Changes for specific celltypes:

• Gate:
  • added default port attributes for basic ports styling as well as default port markup
  • each Gate has two port groups by default with specific styling (attrs): in (left) and out (right)
  • a Gate has always a label (markup and attrs) and reacts on label property change displaying the new label
  • addWire has been replaced by overriding addPort (the proper inputSignals/outputSignals are set within resetPortBits). Also addLabelledWire has be incorporated into addPort by always adding an optional portlabel.
  • consequently, addPorts, removePort and removePorts are also overridden
• Box:
  • is now a parent cell type for every cell that is displayed using a box and thus adds a rect to its markup with default styling (all rects now also use strokeWidth: 2 - I'm aware this is a visual appearance change, but it looked nicer to me. If you object to this change, it would be quite easy to make it look again as before)
  • defines a common markup for a zoom tooltip and a path for the clk input symbol
  • the box resizing function has been generalized s.th. sub cell types are able to activate it (autoResizeBox) and influence the calculated width using calculateBoxWidth
• Arith is a new super cell type which defines common markup, attrs and slightly changed port groups (to cope with the circular shape) for all arithmetic cells
• GateSVG is another new super cell type with common markup, attrs and slightly changed port groups (to cope with the svg images which don't cover the whole rectangle) for all logic cells currently displayed using an external svg image
• GenMux
  • adds a new port group for the select input (on the top side)
  • the checkmark is now drawn using a path directly on the port instead of the mux' markup (the same decor is used to display the clock symbol on other cells' ports)

I would really appreciate if you could have a look at these changes, leave your comments, perhaps find problems or propose further changes.

[tilk]

Wow, that's a lot of changes. I'm currently busy with other things (including teaching a course utilizing DigitalJS), but I'll try to review this as soon as possible.

[tilk]

I'm completely fine with wider lines for boxes, but there is a visual change which looks bad: the symbol on arithmetic nodes are (at least in my browser) not centered anymore.
I tried linking digitaljs_online to your version, and most things seem to work fine. One problem I noticed is with memories. In RAM example, the asynchronous read port has a clock input. In the ROM example, somehow another port is generated. I didn't have time to go over your code yet, could you look into that in the meantime?

[tilk]

In general I like the changes - they remove much of the ugliness of the previous solution. Good work! I'd still like to look for possible issues before merging.

[ialokim]

I'm completely fine with wider lines for boxes, but there is a visual change which looks bad: the symbol on arithmetic nodes are (at least in my browser) not centered anymore.

That's strange, on mine (Firefox) they are. Which browser are you using?

I tried linking digitaljs_online to your version, and most things seem to work fine. One problem I noticed is with memories. In RAM example, the asynchronous read port has a clock input. In the ROM example, somehow another port is generated. I didn't have time to go over your code yet, could you look into that in the meantime?

These memory effects are gone after changing the defaults as you mentioned.

[tilk]

That's strange, on mine (Firefox) they are. Which browser are you using?

Firefox 76 on Linux. Here is a screenshot:

[tilk]

These memory effects are gone after changing the defaults as you mentioned.

Confirmed. Now my best example (the riscv_simple core) runs OK with your version, without apparent correctness or performance issues. I will merge when the visual problem is fixed.

[ialokim]

Firefox 68.8 ESR on Linux here, looks normal for me.

Could you post the corresponding SVG code and check whether any further CSS is applied? Here's mine:

<text class="oper" id="v-319" font-size="12pt" xml:space="preserve" y="0.8em" fill="black" transform="matrix(1,0,0,1,13.5,12.699999809265137)">
    <tspan class="v-line" dy="0">+</tspan>
</text>

[tilk]

My SVG code:

<text class="oper" id="v-271" font-size="12pt" xml:space="preserve" y="0.8em" fill="black" transform="matrix(1,0,0,1,13.399999618530273,13.699999809265137)">
    <tspan class="v-line" dy="0">+</tspan>
</text>

I see nothing weird in the CSS.

I just checked Chrome and the issue is there too. And it's not just digitaljs_online - I see the same issue in the stand-alone tests.

[ialokim]

I've tried to change the Alignment attributes back to textAnchor attributes. Could you check whether this fixes the issues in your browsers? If so, I would like to replace all text alignment attributes by text anchors before merging.

My svg now looks like:

<text class="oper" id="v-319" font-size="12pt" xml:space="preserve" fill="black" text-anchor="middle" transform="matrix(1,0,0,1,20,20)">
    <tspan class="v-line" dy="0.3em">+</tspan>
</text>

[tilk]

The issue is now fixed, on both browsers.

Now I noticed a minor layout issue - see screenshot:

I think it's that because the line for mux selection port is longer in your code.

[ialokim]

The issue is now fixed, on both browsers.

Great to hear! I'll change other occurrences then, too.

minor layout issue

I'll look into it.

There's another issue: port bits are not displayed initially. It should be sort of a one-liner, but just for the record.

[ialokim]

Would you prefer additional commits or a force-pushed version of this branch?

[tilk]

I think having a history of fixes here is not really helpful, so force push is probably OK. I could then merge the commits as they are without squashing.

[ialokim]

Another question: are tooltips shown as expected in your browsers? I was unsure if namespaceURI would be the proper way to reference http://www.w3.org/1999/xhtml.

[tilk]

The tooltips seem to work fine in both Firefox and Chrome.
By the way, it's nice you cleaned them up a little.

[ialokim]

minor layout issue

Unfortunately, this seems to be more difficult than it might appear at first sight. The layout is computed using the model's size which is just the height of the Mux element itself. This can't be changed as it would alter the ports positioning too. The only way I see for now is to set a higher nodeSep for separation of all gates together. However, this results in some ugly extra spacing as you can see below:

See also clientIO/joint#1292

Edit: thanks to a quick answer over at jointJS' github, I was able to fix the problem. See

[ialokim]

I've force-pushed the branch with the necessary fixes. If you don't find any further issues, it would be ready to be merged from my side.

[tilk]

Now that the port bit widths are back, they have a display problem too - if the number is two digit long, the number doesn't fit.

[ialokim]

if the number is two digit long, the number doesn't fit.

Good catch, I've made the port wires 5pt longer each s.th. two digits fit well. Three digits would still not fit, but I guess we could ignore this case for reasonable circuits?

[tilk]

but I guess we could ignore this case for reasonable circuits?
In current version three digits also don't fit, so at least it's not a regression now ;)

I'll play around with this and see if I can find more issues. If not, I think this is ready to merge.