You are viewing a plain text version of this content. The canonical link for it is here.
Posted to fop-dev@xmlgraphics.apache.org by bu...@apache.org on 2004/01/26 12:32:22 UTC
DO NOT REPLY [Bug 26434] New: -
[PATCH] unnesting Property.Maker and rollling datatypes into thier properties.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://nagoya.apache.org/bugzilla/show_bug.cgi?id=26434>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND
INSERTED IN THE BUG DATABASE.
http://nagoya.apache.org/bugzilla/show_bug.cgi?id=26434
[PATCH] unnesting Property.Maker and rollling datatypes into thier properties.
Summary: [PATCH] unnesting Property.Maker and rollling datatypes
into thier properties.
Product: Fop
Version: 1.0dev
Platform: Other
OS/Version: Other
Status: NEW
Severity: Normal
Priority: Other
Component: general
AssignedTo: fop-dev@xml.apache.org
ReportedBy: bckfnn@worldonline.dk
This patch is intended as inspiration and as an example of the discussion found
here:
http://marc.theaimsgroup.com/?l=fop-dev&m=107511296910230&w=2
The patch includes the following:
- unnests the Property.Maker classes.
- moves the PropertyMakers into properties
- Rolls the datatypes into the property classes.
- Moves the property classes into datatypes.
The patch moves files around into new packages but I have not been able to get
a 'delete file' into the patch, so there is some files that must be manually
deleted after applying the patch:
src/java/org/apache/fop/datatypes/AutoLength.java
src/java/org/apache/fop/datatypes/ColorType.java
src/java/org/apache/fop/datatypes/CondLength.java
src/java/org/apache/fop/datatypes/FixedLength.java
src/java/org/apache/fop/datatypes/Keep.java
src/java/org/apache/fop/datatypes/Length.java
src/java/org/apache/fop/datatypes/LengthPair.java
src/java/org/apache/fop/datatypes/LengthRange.java
src/java/org/apache/fop/datatypes/MixedLength.java
src/java/org/apache/fop/datatypes/PercentLength.java
src/java/org/apache/fop/datatypes/Space.java
src/java/org/apache/fop/fo/BorderWidthPropertyMaker.java
src/java/org/apache/fop/fo/CharacterProperty.java
src/java/org/apache/fop/fo/ColorTypeProperty.java
src/java/org/apache/fop/fo/CompoundPropertyMaker.java
src/java/org/apache/fop/fo/CondLengthProperty.java
src/java/org/apache/fop/fo/CorrespondingPropertyMaker.java
src/java/org/apache/fop/fo/DimensionPropertyMaker.java
src/java/org/apache/fop/fo/EnumProperty.java
src/java/org/apache/fop/fo/IndentPropertyMaker.java
src/java/org/apache/fop/fo/KeepProperty.java
src/java/org/apache/fop/fo/LengthPairProperty.java
src/java/org/apache/fop/fo/LengthProperty.java
src/java/org/apache/fop/fo/LengthRangeProperty.java
src/java/org/apache/fop/fo/LineHeightPropertyMaker.java
src/java/org/apache/fop/fo/ListProperty.java
src/java/org/apache/fop/fo/NumberProperty.java
src/java/org/apache/fop/fo/SpaceProperty.java
src/java/org/apache/fop/fo/StringProperty.java
Re: [PATCH] unnesting Property.Maker and rollling datatypes into
thier properties.
Posted by Finn Bock <bc...@worldonline.dk>.
[Simon Pepping]
> I have the following considerations:
> ...
> 3. In code where the datatype aspect is used, the code may become less
> logical. This happens in the parsers and in the RTF renderer. An
> example from render/rtf/TextAttributesConverter.java:
>
> // Cell background color
> ColorTypeProperty colorTypeProp = (ColorTypeProperty)propertyList.get(Constants.PR_COLOR);
> if (colorTypeProp != null) {
> ColorTypeProperty colorType = colorTypeProp.getColorType();
> if (colorType != null) {
> if (colorType.getAlpha() != 0
> || colorType.getRed() != 0
> || colorType.getGreen() != 0
> || colorType.getBlue() != 0) {
> rtfAttr.set(
> RtfText.ATTR_FONT_COLOR,
> convertFOPColorToRTF(colorType));
> }
>
> Here colorTypeProp and colorType denote the same, and the code is
> not quite logical. That is because the method getColorType now more
> acts as an assertion. In the new situation it could be made more
> logical as follows:
>
> // Cell background color
> ColorTypeProperty colorType = (ColorTypeProperty)propertyList.get(Constants.PR_COLOR);
> if (colorType != null && colorType.getColorType() != null) {
> if (colorType.getAlpha() != 0
> || colorType.getRed() != 0
> || colorType.getGreen() != 0
> || colorType.getBlue() != 0) {
> rtfAttr.set(
> RtfText.ATTR_FONT_COLOR,
> convertFOPColorToRTF(colorType));
> }
Yes, I hadn't though of that, perhaps because such constructs are rare
in layoutmgr and area renderers. But I think it should be rewritten to:
// Cell background color
ColorTypeProperty colorType =
propertyList.get(Constants.PR_COLOR).getColorType();
if (colorType.getAlpha() != 0
|| colorType.getRed() != 0
|| colorType.getGreen() != 0
|| colorType.getBlue() != 0) {
rtfAttr.set(RtfText.ATTR_FONT_COLOR,
convertFOPColorToRTF(colorType));
}
without the cast and without testing for a null value because there is
*always* some computed value for PR_COLOR and it is *always* a
colorType. Oh, and the comment should be fixed as well <wink>.
> All in all I think that this change simplifies the code, and would be
> a good change.
Thanks.
> Allow me to make some notes:
>
> 1. Would it not be a good idea to move Property.java from fo to
> properties?
Yes, I agree with you and Glen on this.
> 2. TableColLength and LinearCombinationLength do not have the word
> 'Property' in their name. I would advocate making these names
> consistent with the others.
Perhaps, I don't have strong feelings about naming.
> 3. Should ToBeImplemented.java also be removed?
>
> A lot of good work.
Thanks, but eclipse did all the work for me. It's a great tool for this
kind of refactoring.
regards,
finn
Re: [PATCH] unnesting Property.Maker and rollling datatypes into
thier properties.
Posted by "Peter B. West" <pb...@powerup.com.au>.
Simon Pepping wrote:
>
> I have the following considerations:
>
> 1. The old situation has pure datatypes, which in theory may be reused
> in other situations. In practice, these datatypes are very much
> bound to properties, so that reuse is not realistic, and does not
> happen in FOP code. Combining the notions of datatype and property
> is more tuned to FOP's situation.
Alt-design has completely separate properties and data-types. Instances
of datatypes contain int references to the property on which they were
defined.
>
> 2. Even in the old situation the separation between datatypes and
> properties is not complete. Compound datatypes contain properties.
Alt-design has no compound properties.
Peter
--
Peter B. West <http://www.powerup.com.au/~pbwest/resume.html>
Re: [PATCH] unnesting Property.Maker and rollling datatypes into thier properties.
Posted by Glen Mazza <gr...@yahoo.com>.
--- Finn Bock <bc...@worldonline.dk> wrote:
> I like it. How does this sound:
>
> - Unnest Property.Maker to PropertyMaker and put it
> in fo.properties.
> - Roll the datatypes classes into their property
> class and move the
> property class to fo.properties (but without
> unnesting their Maker
> class because the remaining nested Maker classes
> are really trivial).
> - Move the handcoded maker classes to fo.properties.
>
> regards,
> finn
>
Fortrinlig Tak nemlig jeres arbejde her ovre!
(= Excellent! Thanks for your work here.)
Glen (who recommends we stay with English, despite the
availability of machine translation... ;)
__________________________________
Do you Yahoo!?
Yahoo! SiteBuilder - Free web site building tool. Try it!
http://webhosting.yahoo.com/ps/sb/
Re: [PATCH] unnesting Property.Maker and rollling datatypes into
thier properties.
Posted by Finn Bock <bc...@worldonline.dk>.
[Glen Mazza]
> Another option, Finn, is to move all the Property
> subclasses to fo.properties (even if they're alongside
> the makers, nested or unnested), after thinking about
> it, I think that will be a little bit clearer than
> having them in the datatype package. Comments?
I like it. How does this sound:
- Unnest Property.Maker to PropertyMaker and put it in fo.properties.
- Roll the datatypes classes into their property class and move the
property class to fo.properties (but without unnesting their Maker
class because the remaining nested Maker classes are really trivial).
- Move the handcoded maker classes to fo.properties.
regards,
finn
RE: [PATCH] unnesting Property.Maker and rollling datatypes into thier properties.
Posted by Glen Mazza <gr...@yahoo.com>.
--- "Andreas L. Delmelle" <a_...@pandora.be>
wrote:
> > -----Original Message-----
> > From: Simon Pepping
> [mailto:spepping@leverkruid.nl]
> >
> <snip />
> > All in all I think that this change simplifies the
> code, and would be
> > a good change.
> >
> > Allow me to make some notes:
> >
> > 1. Would it not be a good idea to move
> Property.java from fo to
> > properties?
> >
>
> A question that was on the tip of my tongue too...
> I'd think: not only
> Property.java, but all related Maker-classes as
> well.
>
Another option, Finn, is to move all the Property
subclasses to fo.properties (even if they're alongside
the makers, nested or unnested), after thinking about
it, I think that will be a little bit clearer than
having them in the datatype package. Comments?
Thanks,
Glen
__________________________________
Do you Yahoo!?
Yahoo! SiteBuilder - Free web site building tool. Try it!
http://webhosting.yahoo.com/ps/sb/
RE: [PATCH] unnesting Property.Maker and rollling datatypes into thier properties.
Posted by "Andreas L. Delmelle" <a_...@pandora.be>.
> -----Original Message-----
> From: Simon Pepping [mailto:spepping@leverkruid.nl]
>
<snip />
> All in all I think that this change simplifies the code, and would be
> a good change.
>
> Allow me to make some notes:
>
> 1. Would it not be a good idea to move Property.java from fo to
> properties?
>
A question that was on the tip of my tongue too... I'd think: not only
Property.java, but all related Maker-classes as well.
<snip />
> 3. Should ToBeImplemented.java also be removed?
In the long run, that *should* be the objective indeed. For the moment, I'd
leave that right where it is, until we're sure we've no more refs to it
hidden somewhere else in the code.
Cheers,
Andreas
Re: [PATCH] unnesting Property.Maker and rollling datatypes into thier properties.
Posted by Simon Pepping <sp...@leverkruid.nl>.
On Mon, Jan 26, 2004 at 11:32:22AM -0000, bugzilla@apache.org wrote:
>
> This patch is intended as inspiration and as an example of the discussion found
> here:
>
> http://marc.theaimsgroup.com/?l=fop-dev&m=107511296910230&w=2
>
> The patch includes the following:
>
> - unnests the Property.Maker classes.
> - moves the PropertyMakers into properties
> - Rolls the datatypes into the property classes.
> - Moves the property classes into datatypes.
I have the following considerations:
1. The old situation has pure datatypes, which in theory may be reused
in other situations. In practice, these datatypes are very much
bound to properties, so that reuse is not realistic, and does not
happen in FOP code. Combining the notions of datatype and property
is more tuned to FOP's situation.
2. Even in the old situation the separation between datatypes and
properties is not complete. Compound datatypes contain properties.
3. In code where the datatype aspect is used, the code may become less
logical. This happens in the parsers and in the RTF renderer. An
example from render/rtf/TextAttributesConverter.java:
// Cell background color
ColorTypeProperty colorTypeProp = (ColorTypeProperty)propertyList.get(Constants.PR_COLOR);
if (colorTypeProp != null) {
ColorTypeProperty colorType = colorTypeProp.getColorType();
if (colorType != null) {
if (colorType.getAlpha() != 0
|| colorType.getRed() != 0
|| colorType.getGreen() != 0
|| colorType.getBlue() != 0) {
rtfAttr.set(
RtfText.ATTR_FONT_COLOR,
convertFOPColorToRTF(colorType));
}
Here colorTypeProp and colorType denote the same, and the code is
not quite logical. That is because the method getColorType now more
acts as an assertion. In the new situation it could be made more
logical as follows:
// Cell background color
ColorTypeProperty colorType = (ColorTypeProperty)propertyList.get(Constants.PR_COLOR);
if (colorType != null && colorType.getColorType() != null) {
if (colorType.getAlpha() != 0
|| colorType.getRed() != 0
|| colorType.getGreen() != 0
|| colorType.getBlue() != 0) {
rtfAttr.set(
RtfText.ATTR_FONT_COLOR,
convertFOPColorToRTF(colorType));
}
Therefore, in the new situation people might want to do some
rewriting.
All in all I think that this change simplifies the code, and would be
a good change.
Allow me to make some notes:
1. Would it not be a good idea to move Property.java from fo to
properties?
2. TableColLength and LinearCombinationLength do not have the word
'Property' in their name. I would advocate making these names
consistent with the others.
3. Should ToBeImplemented.java also be removed?
A lot of good work.
Regards,
Simon Pepping
--
Simon Pepping
home page: http://www.leverkruid.nl