Hi
-------------------------------------------------------------------------
Please read this mail, specially the part when I explain the difference
between getQueryURLParameters() and isClientWindowRenderModeEnabled().
Don't remove these methods!!!
-------------------------------------------------------------------------
AS>> Below we state that an "url" mode is supported. Which is correct?
The idea is different implementation can provide multiple modes. For example,
in MyFaces we have "url" and "client" mode, which is a variant of the hack
developed in MyFaces CODI. In that sense, if the param is not set or it has
"none" as value, ClientWindow stuff is disabled.
I suppose the "url" mode is the most common one (used in MyFaces Orchestra
as "conversationContext"), but by default ClientWindow stuff is disabled.
The problem here is each mode has its advantages/disadvantages. There is no
mode that can be perfect, so the idea in this API is cover a wide range of
possible algorithms.
EB>> Some volunteers were pushing for just "none". Some were pushing for
EB>> just "none" and "url". At this point, I'm going to choose "none" and
EB>> "url". I've updated the docs.
Ok.
AS>> What is the expected behavior when the end user creates a second browser
AS>> tab/window with an existing window id? For example, this can happen by:
AS>> - Bookmarking an url with a window id and opening that URL in
multiple tabs.
AS>> - Copying and pasting an url from one tab into a second tab.
AS>> - (On some browsers) Using the browser's "New Window" action.
AS>> For each of the above cases, each window/tab should be assigned its
AS>> own unique id, even though it would require additional work to determine
AS>> when a client window id has been copied across windows/tabs. Will JSF's
AS>> client window implementation handle this? If not, we run the risk of
AS>> severely hosing our end users
It depends on the mode selected.
AS>> What should the JSF implementation do with a value that it does not
AS>> recognize?
It assumes there is an attached ClientWindow implementation with that value
as identifier. In other words, Lifecycle.attachWindow() method will check
the param, take the value and create a proper ClientWindow instance.
AS>> + + public abstract Map<String, String>
getQueryURLParameters(FacesContext context);
AS>> Is the returned Map mutable? My thinking is that:
AS>> - The returned Map should be immutable. And...
AS>> - The method should be spec'ed to return a non-null value.
It is inmutable. I don't have any problem with return a non-null value.
The idea of the method is allow ClientWindow implementations to hook
query params into generated links. ClientWindow implementation can
choose between use it or not. For example, a ClientWindow implementation
could use an css class to recognize links and add the query param
on the client using javascript instead generate it on the server
(Mark Struberg has a project working under that idea).
AS>> An alternate approach would be to avoid returning a Map altogether and
AS>> instead ask the ClientWindow to tack its parameters onto a provided
AS>> base URL, similar to our other encode*URL APIs. This would simplify
AS>> life for client code (eg. no need to create an Iterator, iterate
AS>> over parameter names, check for already existing parameters, etc...)
The calculation for query params requires some string concatenation that is
best done in one place (inside ExternalContext). In this way, a lot less
objects needs to be created when calculating the link, so the extra work
will definitively pay off.
In that sense, the default implementation of ExternalContext.encodeXXX
check if client window API is enabled and if that so it scan this map
for query params and add them.
EB>> This was the API suggested by Leonardo as an extension mechanism. The
EB>> idea was to provide the method and specify the places where it must be
EB>> called so that custom ClientWindow implementations can rest assured that
EB>> the method will be called. Because we can't assert that the map is
EB>> immutable, I don't want to require it. Also, for performance I think
EB>> checking for a null return is quicker than an empty map.
The only one who can change that map is the ClientWindow implementation.
From the point of the caller (ExternalContext implementation) the map is
just for take the query params and add them to the generated url. No
add/remove/update operations are executed from that context.
AS>> Currently ClientWindow creation/initialization is buried in
AS>> LifecycleImpl.attachWindow(). This makes it difficult to provide custom
AS>> ClientWindow implementations, as it requires that you replace or wrap
AS>> the Lifecycle. It also assumes that you are only creating ClientWindows
AS>> from within the JSF lifecycle. Based on our experience with Trinidad's
AS>> window manager, we are likely going to want to create the ClientWindow
AS>> earlier (eg. from a filter).
In this version, LifecycleWrapper interface was added. I think the method
is ok, because it is the context where JSF runs the one who define how
ClientWindow api works. In that sense, ClientWindow implementations are just
a wrapper that provides the necessary info to JSF runtime about how to deal
with client windows, but you can still have something before to do the
real work.
AS>> Fortunately, there is a simple solution, which is to follow the typical
AS>> JSF factory pattern. We should introduce a ClientWindowFactory that is
AS>> responsible for vending ClientWindow instances. The factory can return
AS>> the appropriate type of ClientWindow implementation based on the client
AS>> window mode (eg. return a stub instance for "none", an url-based instance
AS>> for "url", or a 3rd party instance, eg. in the case of Trinidad.)
The same can be done with LifecycleWrapper/LifecycleFactory. The difference
is the logic related to enable/disable an specific client window mode can
be overriden using LifecycleWrapper. For example, you could have a
"client-url-mixed" mode that works different according to the browser
capabilities and so on.
AS>> This would allow us to kill off some unneeded complexity - eg. we could
AS>> kill ClientWindow.decode() - the factory would handle this instead.
I feel Client Window.decode() straightforward. This part is the central point
each ClientWindow implementation should override, so the factory cannot
really do it.
AS>> While we're at it, we could kill Lifecycle.attachWindow(), and instead
AS>> spec that the ClientWindow is set up during FacesContext creation -
AS>> eg. the FacesContextFacotry could set up the ClientWindow on the
AS>> ExternalContext before creating the FacesContext instance.
It is important to have a full FacesContext/ExternalContext instance before
attachWindow(). These two steps are different, because in a resource request,
you have FacesContext/ExternalContext, but you don't need to call
attachWindow(), because the served resource "usually" does not have state.
In MyFaces Orchestra, the code is inside FacesContextFactory, and in my
opinion that code doesn't look nice.
AS>> + + public static void
disableClientWindowRenderMode(FacesContext context) {
AS>>
AS>> I don't understand why these are static. This explanation:
AS>>
AS>> + * This is specified as a static method because callsites
need to access it
AS>> + * without having access to an actual {_at_code ClientWindow}
instance.</p>
AS>>
AS>> Is confusing.
There are three methods:
public static void disableClientWindowRenderMode(FacesContext context)
public static void enableClientWindowRenderMode(FacesContext context)
public static boolean isClientWindowRenderModeEnabled(FacesContext context)
The idea is have a general form to say ClientWindow implementation when
a fragment needs to include client window logic or not. For example, if
url mode is used, the query param is skipped from link generation in
ExternalContext.encodeXXX.
h:link and h:button has an attribute called disableClientWindow that call
these methods. The idea is component authors can control this flag and
deal with the necessary logic inside renderer classes.
The methods are static, because they are not supposed to change, after
all, those methods are just for control a flag.
AS>> If for some reason the ClientWindow is not available (eg. if
facesContext.getExternalContext().getClientWindow() reutrns null),
AS>> well, then... there is nothing to disable anyway, right?
AS>> At the moment the only place that Mojarra calls
AS>> ClientWindow.disableClientWindowRenderMode() is in
AS>> OutcomeTargetRenderer.getEncodedTargetURL(), which is called
AS>> during encoding. We clearly have access to a ClientWindow here.
AS>> What's the deal with making these methods static? From what I
AS>> can tell they should be instance methods.
There is no problem for me if they are not static and are inside
ClientWindow as normal methods.
AS>> Also, should we spec that when disabled,
AS>> ClientWindow.getQueryURLParameters() returns the empty collection?
AS>> If we did that, do we even need an isClientWindowRenderModeEnabled() API?
We need the methods, because not all client window modes requires
query params to work.
AS>> When it comes to API design, I am a minimalist. I also prefer
AS>> to avoid redundancy. Clients of the ClientWindow API only need to
AS>> know whether there are parameters or not, and don't need to be able
AS>> to distinguish between the various cases that might lead to an empty
AS>> parameter set. As such, getQueryURLParameters().isEmpty() is the
AS>> minimally sufficient solution. isClientWidnowRenderModeEnabled() is
AS>> redundant and doesn't add any especially useful information. As a
AS>> minimalist, I take pleasure in killing off seemingly pointless APIs. :-)
EB>> Again, I'm going to push back on that one.
There is no redundancy here. Whether use or not query params into its
logic is responsibility of the ClientWindow implementation, but it is not
a requeriment. So there are two different concepts here.
Please do not remove disableClientWindowRenderMode() related methods,
otherwise the trick used by Mark in real life cannot be ported to this
API.
regards,
Leonardo Uribe