RE: [VOTE] Remove Visitor Patterns from AbstractRenderer.java
-Original Message- From: Glen Mazza [mailto:[EMAIL PROTECTED] This simplification, even if temporary, drops the average IQ needed to understand the Renderer classes perhaps 30 points, more into my range*, hopefully opening the door for more developers to start filling out these renderers. Glen *That of a house plant. Give yourself some credit, man! Besides that, higher IQ only means 'faster understanding under the same circumstances' and not 'more understanding, period'... background knowledge remains the more fundamental prerequisite. Anyway, I can see now that the 'simplification' in question at least offers the benefit of needing a re-implementation of text-justification (among others) which will fit more harmoniously into the redesigned API, instead of merely copying the Maintenance way of doing things and having to 'rape' the new design because we so rabidly want to keep that part working as it did before. It just might prove more worthwhile to be forced to re-think the process of justification in terms of the 1.0 API... let's hope so. Cheers, Andreas
Re: [VOTE] Remove Visitor Patterns from AbstractRenderer.java
Glen Mazza wrote: That hasn't changed; for 1.0 that work is still done in the renderers (as opposed to 0.20.x which had extensive rendering business logic in the layout objects, the 1.0 InlineArea.renders() just made a one-line command to a renderInlineArea() method in the renderer objects). The problem is that text justification and leader expansion is a layout task, not a renderer task. And text justification can't be done during the normal layout pass because of page number references, it has to bedeferred until the references are resolved. There is a reason why the maintenance code is as it is. J.Pietschmann
Re: [VOTE] Remove Visitor Patterns from AbstractRenderer.java
--- J.Pietschmann [EMAIL PROTECTED] wrote: The problem is that text justification and leader expansion is a layout task, not a renderer task. Absolutely, much of the logic needs to be in the layout objects, which interface with the Renderer classes, indeed activate them. That's fine, normal processing, makes rendering easier. (In 0.20.5, LineArea is a layout package object, but in 1.0 the InlineArea subclasses are Area objects, not LM package ones.) And text justification can't be done during the normal layout pass because of page number references, it has to bedeferred until the references are resolved. Either the LM classes do this processing, or the Renderers do it. Either way is fine for me--I'd just rather keep the *Area* objects inert--but if they *do* do the processing, have them do it, not just a one-line immediate bounce-back to now-gotta-make-public LM or Renderer methods. This is torture trying to trace. There is a reason why the maintenance code is as it is. We can always go back when needed/beneficial. Right now, I'm trying to fix the layout bugs (where our example FOs render fine in 0.20.x but not-so-fine in 1.0--I'm currently focusing on the extra-space issue in 1.0) and the bouncebacks between classes is causing my brain to overheat. This simplification, even if temporary, drops the average IQ needed to understand the Renderer classes perhaps 30 points, more into my range*, hopefully opening the door for more developers to start filling out these renderers. Glen *That of a house plant.
RE: [VOTE] Remove Visitor Patterns from AbstractRenderer.java
-Original Message- From: J.Pietschmann [mailto:[EMAIL PROTECTED] snip / BTW I don't think it's good style do ignore a veto and commit a change even before the discussion is resolved. One of my points also: quote The 'more correct' design, I believe, ... after which he can _buy_ _something_ /quote Probably hid that too well ;) Cheers, Andreas
Re: [VOTE] Remove Visitor Patterns from AbstractRenderer.java
--- J.Pietschmann [EMAIL PROTECTED] wrote: Question: in the maintenance branch, leader expansion and text justification is done just before a line is rendered in the LineArea.render() method. The reason for this is that it's only the renderer which knows when page number references are resolved. How will this fit in now? That hasn't changed; for 1.0 that work is still done in the renderers (as opposed to 0.20.x which had extensive rendering business logic in the layout objects, the 1.0 InlineArea.renders() just made a one-line command to a renderInlineArea() method in the renderer objects). But if more accessors are needed in the InlineArea subclasses, for the renderers to retrieve information about its state, that would appear to be a clean solution. (I don't see any more needed however.) BTW I don't think it's good style do ignore a veto and commit a change even before the discussion is resolved. J.Pietschmann I didn't realize your -1 was a veto. I just counted it as a no vote. (I thought you needed to explicitly say veto[1], but I now see that -1 by itself always means veto.[2]) Anyway, far from ignoring your vote, I spent considerable time Tuesday evening researching and responding to the two technical concerns you gave for your vote (bad OO design and locks in renderer design) and I believe I had responded sufficiently for both of those concerns to be met. Do we need more discussion on this?--I'd rather get back into fixing layout/rendering bugs again! Thanks, Glen [1] http://marc.theaimsgroup.com/?l=fop-devm=104565027606931w=2 [2] http://incubator.apache.org/learn/voting.html
RE: [VOTE] Remove Visitor Patterns from AbstractRenderer.java
-Original Message- From: Glen Mazza [mailto:[EMAIL PROTECTED] Taking your pros into account: on 1.) +1 Now that you mention it, I have a vague suspicion that this is somehow related to the bug WRT basic-links I pointed out earlier (for which I was going to send some test file, but I'll put this off for now, and see whether the change solves this, and if so, what part is solved. If we decide to go back at some later point, we might then be able to prevent it from happening again) on 2.) +0 I see myself as unable to judge whether 2) would be good or bad, but I'm inclined to believe the latter (see below). However, FOP is an integrated product, for which one would ideally have to be able to write new renderers without too much hassle. If you think your proposal would encourage this, I see no reason for a -1. In your reply to Joerg's objection, your arguments would certainly make sense in certain circumstances, only the analogy being drawn concerns me a bit: snip / This is not a very good OO design in three ways (comparing buyWhatGlenWants(this) vs. getCapital()): 1.) The private functions buy in class Glen now have to be made public. (Likewise, in AbstractRenderer, all of the renderInline need to move from protected to public to allow for such external calls from the InlineAreas.) (This doesn't occur with getCapital().) Does any package other than the renderers really *need* access to the renderInlineXXX methods? It seems to depend on whether one wants to be able to have: a) an InlineArea that 'knows how to render itself' or b) an InlineArea that 'knows how to provide data about itself to the renderer when asked for it' The logic is not meant to be 'the AbstractRenderer asks the InlineArea to render itself', but rather, 'the AbstractRenderer asks the InlineArea for the specs necessary to render it'. In order to be able to respond to such a call, the InlineArea inevitably gets coupled to the different possible renderers --and the degree of complexity of this coupling is solely determined by the extent up to which the different renderers share a common interface. 2.) The Country classes get coupled with objects not relevant for them (here, class Glen), and hence become less pluggable. (Again, this doesn't occur with getCapital().) In our code, the InlineArea subclasses end up being unnecessarily tied to the renderers. See above: I think 'unnecessarily' should be 'inevitably'. I agree 100% on the point of keeping the tie transparent/as simple as possible, but the fact of being tied itself obviously cannot be avoided. It would make the AbstractRenderer... too abstract IMHO. If the InlineArea knows what to do to render itself, you wouldn't need all the different renderers, would you? --or at least, you wouldn't really need an 'abstract' renderer if the areas are going to be talking the 'concrete' renderers directly... 3.) The business logic for what Glen wants to buy while in Canada is being maintained in the wrong class. Should Glen choose to buy something else in Canada, class Canada needs updating. (On the What about the consideration that class Canada provides for a getAvailableProducts(), or what if 'buying' means something else in the US than it does in Canada? OK, bad example maybe, --try 'buying in the US vs. buying in India', might get us closer --but anyway, whether your given example proves your point rests upon these suppositions (amongst others): - Glen knows what buying means in all possible countries - Glen knows what products are available in all possible countries, or expects that all products he needs are available in all countries he is going to buy from/in Would these hold for the relationship between AbstractRenderer-InlineArea? The 'more correct' design, I believe, would be for Glen to study the Country to find out what buying means over there, what products are available etc., after which he can _buy_ _something_ so a signature like Glen.buy( Country country, Product product, Prereqs[] prereqs ) [where prereqs refers to 'the prerequisites for buying in the country specified in the first argument'] would be more in its place, I presume. Upon making that call, the Country --or whatever instance is used as a salesman - can then prepare whatever is necessary to complete the transaction ( check whether the product is available, and if so, see if Glen has taken all necessary steps that permit him to buy the product he wants...) [Joerg: ] But this is what keeps the renderers pluggable. If these methods are removed, every renderer must follow the same design. I don't understand this point completely (using the country example above, everyone would need to buy hockey skates, but by moving the logic down to the person, class Joerg can buy something else.) But at any You don't have to move the logic down to the person in order to let them buy something else (and perhaps it is even better not to do so). I think the
RE: [VOTE] Remove Visitor Patterns from AbstractRenderer.java
--- Andreas L. Delmelle [EMAIL PROTECTED] wrote: Does any package other than the renderers really *need* access to the renderInlineXXX methods? No, they're now protected--furthermore, whether or not such renderInlineXXX methods even exist within a Renderer is now up to the renderer (the change allowed me to remove these methods from the Renderer interface.) Just FYI, we have two types of renderers--Structure Renderers (MIF, RTF) and formatting renderers (those needing an Area Tree--PDF, PS, PCL, etc.) For the formatting renderers, they all descend from AbstractRenderer and therefore follow its general design. But the nice thing about our system is that they don't have to follow AbstractRenderer. AR implements an interface called Renderer, which is the bare minimum of methods needed to implement a FOP-compatible renderer. Since InlineAreas never call renderInlineXXX anymore, I pulled it out of the Renderer interface. Now, a renderer can create methods of its choosing for implementing inline area rendering--so a Renderer implementor now has more design options. It seems to depend on whether one wants to be able to have: a) an InlineArea that 'knows how to render itself' or No, the wrong direction. A renderer knows how to render an InlineArea. The InlineArea would need to know too much of the innards of the Renderers--as well as its present state of that renderer's processing--for that to happen. b) an InlineArea that 'knows how to provide data about itself to the renderer when asked for it' The logic is not meant to be 'the AbstractRenderer asks the InlineArea to render itself', but rather, 'the AbstractRenderer asks the InlineArea for the specs necessary to render it'. Exactly! Renderers get specs from the Area objects, and use that data to render the object. Rendering is done by renderers. In order to be able to respond to such a call, the InlineArea inevitably gets coupled to the different possible renderers Actually no--The Renderers get coupled to the InlineArea, as well as every other type of Area object, which is fine. Working with InlineAreas, querying their traits, rendering them, etc., is their job. But the InlineAreas aren't coupled to the Renderers anymore--they don't make a reference anymore to Render objects in their code. InlineArea ia.getTrait(); within the renderer code for example: Renderer knows about IA but IA is now oblivious that it's being used by a Renderer object. Glen
RE: [VOTE] Remove Visitor Patterns from AbstractRenderer.java
-Original Message- From: Glen Mazza [mailto:[EMAIL PROTECTED] snip / But the nice thing about our system is that they don't have to follow AbstractRenderer. AR implements an interface called Renderer, which is the bare minimum of methods needed to implement a FOP-compatible renderer. Since InlineAreas never call renderInlineXXX anymore, I pulled it out of the Renderer interface. Now, a renderer can create methods of its choosing for implementing inline area rendering--so a Renderer implementor now has more design options. You've convinced me fully. Thanks for the enlightenment. snip / In order to be able to respond to such a call, the InlineArea inevitably gets coupled to the different possible renderers Actually no--The Renderers get coupled to the InlineArea, as well as every other type of Area object, which is fine. Working with InlineAreas, querying their traits, rendering them, etc., is their job. But the InlineAreas aren't coupled to the Renderers anymore--they don't make a reference anymore to Render objects in their code. InlineArea ia.getTrait(); within the renderer code for example: Renderer knows about IA but IA is now oblivious that it's being used by a Renderer object. Aah! So the references were more like 'remnants' of the previous design, then? (Apart from that, I was paying too little attention to the distinction between Renderer-AbstractRenderer...) Nice cleanup IAC. Cheers, Andreas
Re: [VOTE] Remove Visitor Patterns from AbstractRenderer.java
Glen Mazza wrote: But the InlineAreas aren't coupled to the Renderers anymore--they don't make a reference anymore to Render objects in their code. Question: in the maintenance branch, leader expansion and text justification is done just before a line is rendered in the LineArea.render() method. The reason for this is that it's only the renderer which knows when page number references are resolved. How will this fit in now? BTW I don't think it's good style do ignore a veto and commit a change even before the discussion is resolved. J.Pietschmann
Re: [VOTE] Remove Visitor Patterns from AbstractRenderer.java
Glen Mazza wrote:
Team,
To simplify the Area Tree--Renderer interaction somewhat, making this
section of the code easier to follow, I'd like to make two changes to
the code:
1.) Remove the serveVisitor() methods in AbstractRenderer.java [1], and
return to what we were using last year, that of having the InlineArea
call the render() methods. This will allow us to drop the
area.inline.InlineAreaVisitor interface[2] as well as the sundry
acceptVisitor methods in the inlineArea subclasses[3, for one example
of the 8-9 places this occurs].
Currently developing with them around IMO makes comprehension, coding
and debugging more difficult. They also add an additional level of
indirection in the code where there is enough complexity already. If
there's a need for these patterns in the future, e.g., for more Area
Tree pluggability, we can bring them back, but upon actual demand and
after things are finished in our coding.
Anyway, here's my +1 for this change.
Victor was the one who introduced the serveVisitor stuff to allow AT and
FO Tree separation. Whilst I respected his valiant efforts to achieve
this for the purpose of allowing pluggable layouts, I believe it was a
bridge too far for a project as complex as FOP. And since hes not here now:
heres my +1 too.
snip/
2.) (This I'm less sure on) After reverting, I'd like to remove the
render functions within the InlineArea objects in favor of direct
function calls within AbstractRenderer:
i.e., move from here in render.AbstractRenderer:
protected void renderLineArea(LineArea line) {
List children = line.getInlineAreas();
for (int count = 0; count children.size(); count++) {
InlineArea inline = (InlineArea) children.get(count);
inline.render(this);
}
}
to here:
protected void renderLineArea(LineArea line) {
List children = line.getInlineAreas();
for (int count = 0; count children.size(); count++) {
InlineArea inline = (InlineArea) children.get(count);
renderInlineArea(inline); // may need expansion, with
instanceof() operators.
}
}
This will result in allowing us to remove several render() functions
from the InlineArea objects, and remove the bounce-back between
Renderers and Area objects, further simplifying the coding. The
drawback to this method is that we may need some instanceof() operators
in order to choose the proper render method. (Different InlineAreas
currently have different rendering functions.)
I'm almost +1 on this, but am awaiting more comments on the performance
issues involved. Performance is the my only concern with this change
(this section of the code is very highly called)--I otherwise like the
simplification that this change will offer.
I like the sound of this, and as recent discussion indicated, instanceof
has a rather unfair press. Im all for simplifying the complex code
inside FOP, so
+1
Chris
Re: [VOTE] Remove Visitor Patterns from AbstractRenderer.java
+0 if you feel this simplifies things. On 22.02.2004 23:41:03 Glen Mazza wrote: Team, To simplify the Area Tree--Renderer interaction somewhat, making this section of the code easier to follow, I'd like to make two changes to the code: 1.) Remove the serveVisitor() methods... snip/ - 2.) (This I'm less sure on) After reverting, I'd like to remove the... snip/ Jeremias Maerki
Re: [VOTE] Remove Visitor Patterns from AbstractRenderer.java
Glen Mazza wrote: 1.) Remove the serveVisitor() +0 2.) (This I'm less sure on) After reverting, I'd like to remove the render functions within the InlineArea objects in favor of direct function calls within AbstractRenderer: -1 Remember one of the three basic OO principles: use virtual methods instead of switch according to a class marker. and remove the bounce-back between Renderers and Area objects, further simplifying the coding. But this is what keeps the renderers pluggable. If these methods are removed, every renderer must follow the same design. J.Pietschmann
Re: [VOTE] Remove Visitor Patterns from AbstractRenderer.java
2.) (This I'm less sure on) After reverting, I'd like to remove the
render functions within the InlineArea objects in favor of direct
function calls within AbstractRenderer:
-1
Remember one of the three basic OO principles: use
virtual methods instead of switch according to a
class marker.
Thanks, I just studied this concept:
http://www.adtmag.com/java/articleold.asp?id=1475, and I agree with it,
but I decided to go ahead with the change here because this seems to be
a different scenario.
This principle would be 100% applicable if, for example, within a Class
Glen, I have a function getCapital(country) as follows:
class Glen { // bad
getCapital (Country country) {
if (country instanceof UnitedStates) return Washington;
elsif (country instanceof Canada) return Ottawa;
}
}
Absolutely, 100%, it should be like this instead:
getCapital(Country country) {
return country.getCapital();
}
But if the function is haveGlenBuySomething(Country country) this
functionality, from an OO perspective, really should be maintained in
the Glen class, not the various Country classes:
class Glen { // good
haveGlenBuySomething(Country country) {
if (country instanceof UnitedStates) buyCowboyHat();
elsif (country instanceof Canada) buyHockeySkates();
elsif (country instanceof Mexico) buySombrero();
}
private buyCowboyHat() { ... }
private buyHockeySkates() { ... }
private buySombrero() { ... }
}
*not*
country.buyWhatGlenWants(this); // bad
class Canada extends Country {
buyWhatGlenWants(Glen glen) { // bad
glen.buyHockeySkates();
}
}
This is not a very good OO design in three ways (comparing
buyWhatGlenWants(this) vs. getCapital()):
1.) The private functions buy in class Glen now have to be made
public. (Likewise, in AbstractRenderer, all of the renderInline
need to move from protected to public to allow for such external calls
from the InlineAreas.) (This doesn't occur with getCapital().)
2.) The Country classes get coupled with objects not relevant for them
(here, class Glen), and hence become less pluggable. (Again, this
doesn't occur with getCapital().) In our code, the InlineArea
subclasses end up being unnecessarily tied to the renderers.
3.) The business logic for what Glen wants to buy while in Canada is
being maintained in the wrong class. Should Glen choose to buy
something else in Canada, class Canada needs updating. (On the
contrary, for getCapital(), the country subclass is exactly the right
object to update when its capital changes.)
and remove the bounce-back between Renderers and Area objects,
further simplifying the coding.
But this is what keeps the renderers pluggable. If these
methods are removed, every renderer must follow the same
design.
I don't understand this point completely (using the country example
above, everyone would need to buy hockey skates, but by moving the logic
down to the person, class Joerg can buy something else.) But at any
rate, the Area Tree-needing renderers all descend from AbstractRenderer
whether or not this change is made, so they will still follow more or
less the same design. But if there's a demand for this pluggability in
the future, and this change will provide it, we can easily go back to it.
At any rate, I think the new code is cleaner and easier to follow now
(and besides, we have plenty of instanceof's in AbstractRenderer
elsewhere anyway!)
Thanks,
Glen
[VOTE] Remove Visitor Patterns from AbstractRenderer.java
Team,
To simplify the Area Tree--Renderer interaction somewhat, making this
section of the code easier to follow, I'd like to make two changes to
the code:
1.) Remove the serveVisitor() methods in AbstractRenderer.java [1], and
return to what we were using last year, that of having the InlineArea
call the render() methods. This will allow us to drop the
area.inline.InlineAreaVisitor interface[2] as well as the sundry
acceptVisitor methods in the inlineArea subclasses[3, for one example
of the 8-9 places this occurs].
Currently developing with them around IMO makes comprehension, coding
and debugging more difficult. They also add an additional level of
indirection in the code where there is enough complexity already. If
there's a need for these patterns in the future, e.g., for more Area
Tree pluggability, we can bring them back, but upon actual demand and
after things are finished in our coding.
Anyway, here's my +1 for this change.
[1]
http://cvs.apache.org/viewcvs.cgi/xml-fop/src/java/org/apache/fop/render/AbstractRenderer.java?r1=1.12r2=1.13diff_format=h
[2]
http://cvs.apache.org/viewcvs.cgi/xml-fop/src/java/org/apache/fop/area/inline/InlineAreaVisitor.java?diff_format=hrev=1.2view=auto
[3]
http://cvs.apache.org/viewcvs.cgi/xml-fop/src/java/org/apache/fop/area/inline/Character.java?r1=1.1r2=1.2
-
2.) (This I'm less sure on) After reverting, I'd like to remove the
render functions within the InlineArea objects in favor of direct
function calls within AbstractRenderer:
i.e., move from here in render.AbstractRenderer:
protected void renderLineArea(LineArea line) {
List children = line.getInlineAreas();
for (int count = 0; count children.size(); count++) {
InlineArea inline = (InlineArea) children.get(count);
inline.render(this);
}
}
to here:
protected void renderLineArea(LineArea line) {
List children = line.getInlineAreas();
for (int count = 0; count children.size(); count++) {
InlineArea inline = (InlineArea) children.get(count);
renderInlineArea(inline); // may need expansion, with
instanceof() operators.
}
}
This will result in allowing us to remove several render() functions
from the InlineArea objects, and remove the bounce-back between
Renderers and Area objects, further simplifying the coding. The
drawback to this method is that we may need some instanceof() operators
in order to choose the proper render method. (Different InlineAreas
currently have different rendering functions.)
I'm almost +1 on this, but am awaiting more comments on the performance
issues involved. Performance is the my only concern with this change
(this section of the code is very highly called)--I otherwise like the
simplification that this change will offer.
Thanks,
Glen
