Hi
I have checked the documentation related to ClientWindow feature and I have
some observations to do.
I already did an implementation of the proposed api provided by the early draft
snapshot, which compared with the latest api only has minimal changes.
The code is here:
http://svn.apache.org/repos/asf/myfaces/core/branches/2.1.x-client-window/
I also did a small test case:
http://svn.apache.org/repos/asf/myfaces/core/branches/2.1.x-client-window/client-window-example/
After implement it, I notice two observations:
1. It is very useful to create a composite component that
enable/disable ClientWindow url mode. The
idea is write things like this:
<cw:disableClientWindow/>
<h:link outcome="helloWorld" value="helloWorld (do not
append url param and set target=''blank'')" target="_blank"/>
<cw:enableClientWindow/>
2. The solution implemented in myfaces has two modes:
- url
- client (or a port of MyFaces CODI solution, that uses a front
page to set the clientWindow on the
client side).
both solutions require ResponseStateManager.WINDOW_ID_URL_PARAM, but
the javadoc of the constant
says this:
"... The name of the URL query parameter that is only used if
ClientWindow.WINDOW_ID_MODE_PARAM_NAME is "url". The name of the
parameter is given by
the constant value of this field. The value of this parameter is the
return from ClientWindow.getId(). ..."
In this case, the lack of flexibility in this part becomes a problem.
It is possible to think about modes
that does not use the query param at all, others that use it and
others that requires two query params
(one for the request id, other for the windowId).
But the idea of have these methods in ClientWindow is very useful:
static void disableClientWindowUrlMode(FacesContext context)
static void enableClientWindowUrlMode(FacesContext context)
static boolean isClientWindowUrlModeEnabled(FacesContext context)
Because it allows to the RenderKit implementation or
ExternalContext.encodeXXX methods to work
together with the ClientWindow implementation and render the necessary
code, no matter if is just
add some query params or do something special (add a css class to
identify the links that requires
client window processing).
I think in this case we are mixing two different requeriments:
1. Provide a standard way to identify whether to apply the clientWindow mode or
not.
2. Provide a hook over generated urls to allow ClientWindow to
"inject" query params.
The solution proposed to this problem is add this method in ClientWindow
public Map<String, String> getQueryUrlParameters(FacesContext context)
by default it returns null, so we can delegate to the ClientWindow
implementation this detail. In this way,
alternate implementations can choose if they require to use query
param proposed by the spec "jfwid",
if the need something more or if it does not require it at all.
Additionally, we can and rename again
the methods in ClientWindow to something more neutral:
static void disableClientWindowRenderMode(FacesContext context)
static void enableClientWindowRenderMode(FacesContext context)
static boolean isClientWindowRenderModeEnabled(FacesContext context)
Please note the javadoc of ExternalContext.encodeActionURL(String
url) should be changed to
reflect these improvements.
With these changes, I think the solution looks great.
Suggestions are welcome.
regards,
Leonardo Uribe
2012/10/24 Werner Punz <werner.punz_at_gmail.com>:
> Hello after rereading the entire section I have to apologize, I was mislead
> but, i am somewhat clearer,
> sorry for my first mail which was caused a little bit by my misunderstanding
> probably also because
> I was already thinking this fixes the multiforms issue we have had since 2.0
> which it does not.
>
> I personally think for xml correctnes this thing works, but it does not work
> out for multiple form scenarii:
> Lets rereview some parts
>
> We have the change form <update id="javax.faces.ViewState"> to
>
> <update
> id="<VIEW_ROOT_CONTAINER_CLIENT_ID><SEP>javax.faces.ViewState<SEP><UNIQUE_PER_
> VIEW_NUMBER>">
> <![CDATA[...]]>
> </update>
>
> For XML correctness this is ok.
>
>>locate and update the submitting form's javax.faces.ViewState value with
>> the CDATA contents from
>> the response. <SEP>: is the currently configured (sidenote also the
>> render targets are updated as > fix for the multiple forms scenarii)
>
> So far so good this is the old behavior, as we have it, which is broken in
> itself, because the multiple forms scenarii break here.
> (only the issuing form is updated which causes an old viewstate in case of a
> second form)
>
> Then we have the section:
>
> Locate and update the javax.faces.ViewState value for all forms specified in
> the render target list.
>
> This fixes the multiple forms scenario in a way that you can add other forms
> to the render targets,
> this is quite unsatisfying because you have to specify render targets for
> every form you want to update.
> (I introduced in myfaces a config param no_portlet_environment (aka
> update_all_forms)
> to bypass the issue, because I got a lot of angry user mails regarding this
> problem
> who felt that the render target solution was unsatisfying)
> The reason for this was that we did not have any viewroot information at
> that stage in the viewstate part
> of the spec, this has changed.
>
> Now we have already the viewroot information and thus a dedicated
> information which section of the page
> is affected by the viewstate change, wouldnt it be simpler to update simply
> all the forms within the viewroot
> and maybe optionally the render targets? Or is there a reason why we still
> stick with the old broken
> behavior of not simply updating all forms under viewroot but just the
> issuing form?
> The question then also is do we still need the render target update as well?
>
> My personal preference to this section would be simply:
>
> <update
> id="<VIEW_ROOT_CONTAINER_CLIENT_ID><SEP>javax.faces.ViewState<SEP><UNIQUE_PER_VIEW_NUMBER>">
> <![CDATA[...]]>
> </update>
>
> locate and update the issuing form and all forms under
> VIEW_ROOT_CONTAINER_CLIENT_ID.
>
>
> and maybe
>
>
> Locate and update the javax.faces.ViewState value for all forms specified in
> the render target list.
>
> For backwards compatibility reasons.
> I am not sure whether this should be discussed in here or later after the
> first public draft, I just wanted
> to add this comment as a sidenote because I see still no fix for the
> multiform scenario here as it
> was introduced in 2.0 and then later somewhat a little bit fixed but not
> satisfyingly in 2.0a/2.1.
> And now that we have the viewroot information in place I guess it would be
> time to fix this as well.
>
>
> Werner
>
>
> Am 23.10.12 22:55, schrieb Edward Burns:
>
> On Mon, 01 Oct 2012 12:33:33 +0200, Werner Punz <werner.punz_at_gmail.com>
> said:
>
> WP> Ok I found another issue jsdoc section ViewState handling:
>
> If an |update| element is found in the response with an identifier
> containing |javax.faces.ViewState|:
> |<update
> id="<VIEW_ROOT_CONTAINER_CLIENT_ID><SEP>javax.faces.ViewState<SEP><UNIQUE_PER_VIEW_NUMBER>">
> <![CDATA[...]]>
> </update>|
> locate and update the submitting form's |javax.faces.ViewState| value
> with the |CDATA| contents from the response. <SEP>: is the currently
> configured |UINamingContainer.getSeparatorChar()|.
> <VIEW_ROOT_CONTAINER_CLIENT_ID> is the return from
> |UIViewRoot.getContainerClientId()| on the view from whence this state
> originated. <UNIQUE_PER_VIEW_NUMBER> is a number that must be unique
> within this view, but must not be included in the view state. This
> requirement is simply to satisfy XML correctness in parity with what
> is done in the corresponding non-partial JSF view. Locate and update
> the |javax.faces.ViewState| value for all forms specified in the
> |render| target list
>
> WP> Now if we introduce a viewroot container client id, as far as I
> WP> understood the original discussion, we do not need that anymore, we do
> WP> only update the forms within the given viewroot (this would solve the
> WP> multiple forms problem as well), maybe I misunderstood the discussion
>
> But we *have* introduced the VIEW_ROOT_CONTAINER_CLIENT_ID. There is no
> "if".
>
> "We do not need that anymore", I'm sorry, what do you mean by "that"?
>
> WP> From what I can gather here, the Viewroot is not really used and we
> WP> default back to the original broken implementation of using render
> WP> targets and origin forms in a multiple form scenario. The question is,
> WP> given the original discussion of introducing this mechanism to be xml
> WP> correct for multiple forms, do we still need the render target mechanism
> WP> here, wouldnt it be simpler just to update all forms within a given
> WP> ViewRoot?
>
> At this point, I'm not sure. Given where we are in the schedule, unless
> you feel that the current draft specification is broken, I'd really like
> to keep it as is.
>
> Ed
>
>