Re: svn commit: r1177251 - in /xmlgraphics/fop/trunk/src/java/org/apache/fop: fo/flow/BasicLink.java layoutmgr/inline/InlineLayoutManager.java pdf/PDFFactory.java

[Glenn Adams <gl...@skynav.com>]

i just checked this change against the complex script branch, and it's ok
(doesn't regress); so if you want to leave it in, i won't object, though, in
general, i prefer the "if it ain't broke, don't fix it" rule;

i would also note that in the complex script branch i had previously
redefined BidiOverride to inherit from Inline in order to reuse a portion of
its implementation; i would not want to redefine it again merely to follow
the same path you have chosen for BasicLink;

also, just to note further, your change seems to have introduced a new
findbugs warning; perhaps you can fix:

 CodeWarning UPMPrivate method
org.apache.fop.layoutmgr.inline.InlineLayoutManager.getInlineFO() is never
called

Bug type UPM_UNCALLED_PRIVATE_METHOD (click for
details)<file:///Users/glenn/Work/fop/build/report_findbugs.html#UPM_UNCALLED_PRIVATE_METHOD>

In class org.apache.fop.layoutmgr.inline.InlineLayoutManager
In method org.apache.fop.layoutmgr.inline.InlineLayoutManager.getInlineFO()
At InlineLayoutManager.java:[line 111]


On Fri, Sep 30, 2011 at 10:54 PM, Glenn Adams <gl...@skynav.com> wrote:

> let's step back a minute; what was the problem you were trying to solve?
> what was broken? how did your change fix it?
>
> if you made this change just because you think instanceof Inline should
> return false on a BasicLink, then this change would seem gratuitous
>
> it is wholly reasonable that BasicLink may share the implementation of
> Inline as previously held; your change required you to copy/paste existing
> code from Inline into BasicLink and to alter InlineLayoutManager for no
> purpose other than accommodating your change
>
> IMO you should revert the change
>
> On Fri, Sep 30, 2011 at 9:18 PM, Peter Hancock <pe...@gmail.com>wrote:
>
>> By ancestor I refer to the relationship with respect to the fo:
>> element hierarchy: Since the definition of fo:basic-link does not
>> depend upon fo:inline, we cannot conclude that fo:basic-link is an
>> fo:inline.
>>
>> The parameter entity "%inline;" refers to inline-level fo elements,
>> fo:inline and fo:basic-link being members, and this is now reflected
>> on the FOP FO object hierarchy, where Inline and BasicLink extend
>> InlineLevel
>>
>> Have I understood the recommendation correctly, or have I missed anything?
>>
>> On Fri, Sep 30, 2011 at 1:18 PM, Glenn Adams <gl...@skynav.com> wrote:
>> > i'm not sure what you mean by 'ancestor', since containment relation is
>> not
>> > at issue here;
>> > your argument is counter to the definition of the parameter entity
>> %inline;
>> > defined in XSL 1.1 Section 6.2
>> >
>> > The parameter entity, "%inline;" in the content models below, contains
>> the
>> > following formatting objects:
>> >
>> >      bidi-override
>> >      character
>> >      external-graphic
>> >      instream-foreign-object
>> >      inline
>> >      inline-container
>> >      leader
>> >      page-number
>> >      page-number-citation
>> >      page-number-citation-last
>> >      scaling-value-citation
>> >      basic-link
>> >      multi-toggle
>> >      index-page-citation-list
>> >
>> > i believe you should first restore the previous state of affairs, and
>> then,
>> > if you wish to continue making the case that it is not inline, take it
>> up
>> > with the group and get consensus before making what appears to be a
>> possibly
>> > unjustified architectural change
>> >
>> > On Fri, Sep 30, 2011 at 5:31 PM, Peter Hancock <peter.hancock@gmail.com
>> >
>> > wrote:
>> >>
>> >> While fo:basic-link and fo:inline are both inline level elements, one
>> >> is not the ancestor of the other and so FOP's model of the FO elements
>> >> should reflect this, I believe.
>> >>
>> >> On Fri, Sep 30, 2011 at 8:43 AM, Glenn Adams <gl...@skynav.com> wrote:
>> >> > if I recall, I need this inheritance (from Inline) to work in the
>> >> > complex
>> >> > script branch as well
>> >> >
>> >> > On Fri, Sep 30, 2011 at 3:12 PM, Simon Pepping <
>> spepping@leverkruid.eu>
>> >> > wrote:
>> >> >>
>> >> >> On Thu, Sep 29, 2011 at 10:18:54AM -0000, phancock@apache.orgwrote:
>> >> >> > Author: phancock
>> >> >> > Date: Thu Sep 29 10:18:53 2011
>> >> >> > New Revision: 1177251
>> >> >> >
>> >> >> > URL: http://svn.apache.org/viewvc?rev=1177251&view=rev
>> >> >> > Log:
>> >> >> > Fix FO tree hierarchy: BasicLink shouldn't inherit from Inline
>> >> >>
>> >> >> Why? A basic-link is an inline object which generates inline areas.
>> >> >>
>> >> >> Simon
>> >> >
>> >> >
>> >
>> >
>>
>
>