See the attached header file for the first draft of what I think the interface for the new text layout engine should look like. The intended audience is developers, there's nothing to see here for people who don't enjoy reading source code.
Comments are encouraged on design, comprehensibility, features, style and smackdowns.
A few things to talk about, in no particular order:
1) Do we disapprove of public member variables, even where there's no disadvantages to using them?
2) The coding style guide doesn't mention the how the names of enumerations should be formatted.
3) I implement parse_svg_length_list() because the old flow_src did. Should this really belong closer to the XML layer?
4) Likewise the xml:space attribute. The difficulty with moving this is that you could cause roundtripping problems. Can we even do roundtripping?
5) Lots of private members are missing still, because I don't know what they're going to be yet.
6) The appearance of this document doesn't mean I've decided whether to wrap the existing code, switch over the users, then write my own innards, or to do it all in one big (destructive) commit. The latter is quite a lot less work, but the former will likely be broken for shorter intervals (although maybe more of them).
7..n) There are more things to discuss in the attachment, under the heading "Comments", near the top.
That's it, I think. I'll remember something else as soon as I click 'send', of course.
Richard.
On Fri, 2005-02-18 at 20:04, Richard Hughes wrote:
A few things to talk about, in no particular order:
- Do we disapprove of public member variables, even where there's no
disadvantages to using them?
There are always disadvantages in the long term (wrt maintainence at least), for most classes.
Modifications to public member variable declarations unavoidably create widespread ripples during refactoring, whereas with methods you can always write wrappers to preserve the old interface temporarily.
You should always plan on your classes getting refactored someday; no matter how well-designed they are, requirements will always change.
I learned this lesson the hard way in Inkscape. Wrapping existing public member variables in accessor methods was the ONLY way I could successfully perform many of the major refactorings I've done in our codebase.
Otherwise, I was forced to perform far too much radical change, all at once. Trying to bite off too much at once almost always meant having to scrap what I was doing and start over again.
That's not to say public member variables are _always_ bad. Basically, classes fall into three categories:
1) "interface" classes, which have mostly abstract or very thin wrapper methods, and no data to speak of at all.
2) "implementation" classes, whose member variables ought to be private or (occasionally) protected.
3) "data" classes, whose member variables can and ought to be public; these classes hold locally useful data and should only have trivial methods if any (often a constructor to prefill/initialize fields is desirable).
The latter type of class ("data") are used mostly for local, internal things.
In reality things aren't quite so cut-and-dried, but that is at least the Platonic ideal I would like us to strive for.
- The coding style guide doesn't mention the how the names of
enumerations should be formatted.
The enumeration type name should be formatted the same as class names, the names of individual enumeration values ought to be all caps and use underscores for word separators, as I believe the coding style document specifies for constants.
- Likewise the xml:space attribute. The difficulty with moving this
is that you could cause roundtripping problems. Can we even do roundtripping?
Ideally we should be able to. I'm not sure how well we currently do in that regard though.
-mental
I've just finished reading it - very impressive, thanks! It has everything I need (well, probably something is missing still, but I can't find anything offhand). If such a class exists and works as advertised, I will have no problem quickly switching at least SPText with children and SPTextPath to it. In fact, for some time we could maintain both old and new implementations and switch between them for testing vs. actual work, until the new one is mature enough.
With regard to planning, we're now early in the release cycle, and this one is going to be a long one, so you have quite some time to work on this if you start now (several months). I think it will be OK to still have no official textflow support in 0.42, however the regular text and text-on-path must work at least we well as now. So, for now you can focus on these simpler cases (i.e. without wrap_inside_shapes) to finish them for the next release, and leave the flowtext for later.
On Sat, 19 Feb 2005 01:04:49 +0000, Richard Hughes <richard@...724...> wrote:
- I implement parse_svg_length_list() because the old flow_src did.
Should this really belong closer to the XML layer?
We have it in sp_svg_length_list_read
- The appearance of this document doesn't mean I've decided whether
to wrap the existing code, switch over the users, then write my own innards, or to do it all in one big (destructive) commit. The latter is quite a lot less work, but the former will likely be broken for shorter intervals (although maybe more of them).
I think we can go for the latter, but with a period of both implementations being available to switch between, as per above.
Just finished an initial review of your actual header file. I love your documentation.
Nothing jumps out at me as being particularly wrong; I'd feel a bit better if e.g. std::vector<WrapShape> were a typedef, and wrapInsideShapes() were an accessor method that returned a reference to a _wrap_inside_shapes, but other than that I don't have any quibbles at the moment.
Good job. I look forward to seeing this implemented...
-mental
On Saturday 19 February 2005 18:11, MenTaLguY wrote:
Just finished an initial review of your actual header file. I love your documentation.
Nothing jumps out at me as being particularly wrong; I'd feel a bit better if e.g. std::vector<WrapShape> were a typedef, and wrapInsideShapes() were an accessor method that returned a reference to a _wrap_inside_shapes, but other than that I don't have any quibbles at the moment.
Good job. I look forward to seeing this implemented...
-mental
I saw the mention of floats and doubles in this thread. We have found using doubles only will give you a significat performance increase, esp across architechtures, and you can ignore rounding issues between floating pt types.
Craig Scribus Team
On Sat, 19 Feb 2005 18:37:31 +0100, Craig Bradney <cbradney@...242...> wrote:
I saw the mention of floats and doubles in this thread. We have found using doubles only will give you a significat performance increase, esp across architechtures, and you can ignore rounding issues between floating pt types.
I agree that it's better to use doubles throughout, as we do in the rest of the code. I don't think using floats will buy us memory savings worth speaking about, while the cost in code maintainability and perhaps, as Craig suggests, even performance may be significant.
participants (4)
-
bulia byak -
Craig Bradney -
MenTaLguY -
Richard Hughes