Hi all,
I have rewritten most of the style handling code in style.h and style.cpp to use real C++ classes. I would like you all to review the code and give feedback. The code is available at:
lp:~inkscape.dev/inkscape/style
Background:
I have found it quite difficult to add new properties to the Inkscape code as I try to implement SVG 2 features. The code in style.cpp is a convoluted mess. To add a new property, one needs to find a similar, already existing property and search through the code for it to find all the places where one should add code for the new property. This is quite error prone and time consuming.
Following the advice in the book "Refactoring, Improving the Design of Existing Code" (thanks to the Inkscape fund for a copy), I started to refactor the code bit by bit, while adding a bunch of unit tests to style-testing.h.
Scope:
I've limited my changes to the following files: style.h style.cpp style-internal.h style-internal.cpp, style-enums.h (plus minor changes to color.h, id-class.cpp, Makefile_insert). There are opportunities for further refactoring but they require modifying code in other files, something I wanted to avoid doing until I got the basic refactoring done.
Advantages:
It is now quite straight forward to add a new property, especially if it fits one of the pre-existing models. It can be as easy as adding the property to the SPStyle class declaration in style.h, initializing it in SPStyle::SPstyle, storing its address in the std::vector in SPStyle::SPStyle, and adding an entry in the switch statement in SPStyle::read_if_unset (this last requirement, could possibly be eliminated).
Not only will it be easier to add new properties, but it will also be easy to add new functionality. For example, it will be easy to add a variable to keep track of where a property is set (in a style sheet, in an attribute statement, or in a style-attribute), and write out the property in the same place. This is a requirement if we are ever to include stylesheet editing in Inkscape.
Disadvantages:
The code could be slower as there is more overhead in function calls. I've tried it out with a large file (the Tango icons) and don't see any measurable slow down.
The code does use more memory. It appears to use about 4k bytes more per object (this is really hard to measure accurately). One thing to think about in the code review is how memory use can be reduced.
(One possible place memory might be reduced is that it appears that in the rendering of Inkscape's icons, the instances of SPStyle that are created are not released. This means that around 4000 objects are being kept around.)
Status:
The code is mostly done. Here is a list of possible things to be worked on:
Short term (all internal to the SPStyle and SPIxxx classes):
* Finish SPColor class and use where appropriate ('color' and 'text-decoration-color').
* Memory use issues:
Remove unneeded destructors. Check variable order (to insure minimal memory use).
* Class Documentation
* Move SPIPaint/SPIFilter HREF functions into classes
* Find out why SPStyle::read() requires call to clear().
* Investigate using std::map instead of std::vector to allow removal of switch in SPStyle::read_if_unset(). A hash is already being used to look up the property so it is a more a question of memory usage than efficiency.
Medium term (requires changes external to style code):
* Update rest of code to use new SPStyle member functions.
* Restructure 'text' (i.e. font info) to match other properties
* Restructure SPIPaint/SPIColor to match other properties
* Convert text-decoration-style to SPIEnum
* Move sp_css_attr_xxxx functions to separate file.
* Correct quoting in 'font-family'
* Proper 'font' class
* SPIColor: Enum for color format: hsl(), named color.
Long term:
* Keep track of where a property is set (style-sheet, attribute, style attribute) and reuse on write.
* Proper 'ex' height handling
* CSS3 Color (HSL) + named colors (i.e. no longer convert red to #ff0000)
* CSS4 Color
* Proper handling of SPIFilter::cascade() and SPIFilter::merge()
* Dash array with units.
* Release SPStyle instances after icon generation.
* Check for code duplication between SPStyle classes and XML style handling.
* Add the ability to choose shorthand or longhand property formats.
Tav