-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Hi Ate,
thanks for your effort to comment the Fixme's and Todo's. I forgot one
issue, sorry. There are a lot of Java Doc Todo's in the code. We should
make an issue for this too.
Torsten
Ate Douma schrieb:
| Hi Torsten,
|
| Sorry for the delayed response but very much thanks for taking care of
this.
|
| I've looked at the TODOs and FIXMEs lists you created and have some
comments on them below.
|
| I'll skip the "old" JSR-168/Pluto 1.x FIXME issues for now. Those need
to be reviewed and processed too of course but I'd like to concentrate
on the new, JSR-286 related issues first.
| Maybe Craig, Elliot or active Pluto community members can also take a
look at the "old" Pluto 1.x TODOs and FIXMEs and qualify them as to be
resolved or removed?
|
| Starting then with the FIXMElist.
|
| - public Profile DummyCCPPProfileServiceImpl.getCCPPProfile(...). line #19
| I think the FIXME from that method can be removed. As this is A
DummyCCPPProfileServiceImpl, it should not be expected it to return a
"real" Profile,
| so returning a DummyProfile instance is perfectly fine in my view.
+1
|
| - public static EventProvider
EventProviderImpl.getEventProviderImpl(), line #134
| That method doesn't seem to be used anywhere, so my first reaction is:
can't it simply be removed from the PortalCallbackService interface and thus
| also from this class?
I can check it and if it not used i delete it if that is ok
|
| - public boolean SupportedModesServiceImpl.isPortletManagedMode(...),
line #236
| This empty method isn't (yet) called anywhere in Pluto, but should be
properly implemented when Pluto (or for instance Jetspeed) would like to
support it.
| Its optional, but I think we should properly implement this service as
well as SupportedWindowStatesService would be properly implemented
before the Pluto 2.0 (container) release.
|
| - PortletURLTag286.doStartTag(), line #50
| //FIXME:implement escapeXml-container-runtime-option check!
| This definitely looks like a required feature to me which even
should be implemented for the RI, no?
this have to be implemented in the RI, I will do it.
|
| - pluto-testsuite/src/main/webapp/WEB-INF/portlet.xml, #19
| The schema location
+1
| of the 2.0.xsd at uni-jena.de definitely needs to be removed.
|
| For the TODOlist it was a bit more work to evaluate these as its not
always clear what is "new" or "old" stuff.
| Anyway, I had a go at it and these are my preliminary comments.
|
| - BaseURLImpl, line #404
| //TODO:This two methods should be deleted, when the CACHABILITY
parameter gets his own prefix
| Can you elaborate on this and how/when this is supposed to happen? I
really have no idea.
I have stored the parameter for the Cachability in the (for URL which
are generated in the resourceServing lifecycle) URL as normal render
parameter. I think we should move it to a parameter with his own prefix
in the URL, with this point we move the functionality for the request
for Cachability-level to the PortletURLProvider. We should discuss this.
In my opinion this isn't critical. Pluto don't supports any form of
Cachability.
|
| - CacheControlImpl, line 26
| TODO: Move CachControlImpl from inner Class to this implementation
| Seems like a simple action to me (concerning
MimeResponseImpl.CacheControlImpl inner class), +1
+1
|
| - other TODOs
| I didn't see anything critical at first review, maybe others can
take a look too?
|
| In general, there are a quite a few places where exceptions are
"swallowed", albeit logged (thanks for replacing printStackTrace calls
with at least proper logging Torsten). We probably should create an
issue to review each of those and see if better/more appropriate
handling is possible or even required.
+1
|
| So, overall, it doesn't look so bad anymore.
| The only required issue for a RI release POV might be the handling of
the escapeXml-container-runtime-option in PortletURLTag286.doStartTag()
|
| The other issues should be handled before we can start thinking of a
Pluto 2.0 release.
|
| And, another requirement for we can actually release Pluto 2.0 is
making sure the container actually is embeddable again by Jetspeed-2 at
least.
| I already know there's a lot to discuss *and* to do before that will
be possible again...
|
| Regards,
|
| Ate
|
|
| Torsten Dettborn wrote:
|> Hi,
|>
|> I have attached the TODO list. I changed the print stack messages to
log messages in the last commit, sorry i forgot it to say.
|>
|> Torsten
|>
|> Ate Douma schrieb:
|>> Hi Torsten,
|>>
|>> Sorry for not following up but I was away for a JSR-301 F2F.
|>>
|>> You proposed earlier to provide a new patch to restore the unresolved
|> TODO and FIXMEs, but rolling back and only applying the real changes
should be fine as well.
|>>
|>> And we definitely need a proper list of outstanding issues, especially
|> those related to the JSR-286 RI, so if you can provide that based on
your own information *as well* as the remaining TODO and FIXMEs that
would be nice.
|>>
|>> Regards,
|>>
|>> Ate
|>>
|>> Torsten Dettborn wrote:
|>> Hi Ate,
|>>
|>> because the discussion about this has stopped, here my plan, (I need
half a day and than I'm ready)
|>>
|>> I roll the patch back, send a new patch without deleting undone TODO
and Fixme and send a list for this items.
|>> For this list we have to compare it with the jira issues.
|>>
|>> Hopefully this is ok for everybody
|>>
|>> Torsten
|>>
|>> [EMAIL PROTECTED] schrieb:
|>> | I'm currently not able to check on these changes as I'm at a
JSR-301 F2F
|>> | and can't get my laptop to connect to the internet (I'm typing this
|>> | response at a public booth) which it looks like will remain so the
next 2
|>> | days :(
|>> |
|>> | Anyway, if this is all true, I fully agree these changes need to
be rolled
|>> | back. Its clear there are areas in Pluto 2.0 trunk which needs
fixing and
|>> | implementing before it can be considered usable and more or less
complete.
|>> | Simply ripping out FIXME and TODO comment won't bring us any closer to
|>> | that, on the contrary...
|>> |
|>> | My suggestion is th following:
|>> | - rollback these changes
|>> | - make a list of all *must* have issues/FIXMEs
|>> | - make a list of all *nice* to have TODOs
|>> | - if need be, discuss these on the dev list first
|>> | - create JIRA issues for all the above, making (at least) the
first list
|>> | items required for the 2.0 container release
|>> |
|>> | Regards,
|>> |
|>> | Ate
|>> |
|>> |> Torsten,
|>> |>
|>> |> I very much appreciated the work you and your group has done to
create
|>> |> Pluto 2.0, but I was very disappointed in many of the commits you did
|>> |> today. Rather than fixing most of the TODOs and FIXMEs, you just
erased
|>> |> them. Some of the TODOs and FIXMEs are inconsequential and not
necessary
|>> |> and should be erased and some FIXMEs are not that important and
should be
|>> |> changed to TODOs. But many of these should have been fixed before the
|>> |> FIXME
|>> |> or TODO was removed.
|>> |>
|>> |> Here are a few specific comments:
|>> |> 1. Many of the TODOs are those automatically added by Eclipse and are
|>> |> labelled "Auto-generated catch block" and include a
printStackTrace() call
|>> |> to stdout. This should have been changed to log to Pluto's log file
|>> |> including a relevant message and the exception so the stack trace
will
|>> |> appear in the Pluto log. Consideration should also be made to
rethrow the
|>> |> exception so it is propagated up the stack trace to the user.
|>> |> 2. The FIXME in SupportedModesServiceImpl.isPortletManagedMode()
refers to
|>> |> the fact that this method is not properly implemented and had
notes on how
|>> |> to implement the method. You just erased the FIXME and notes
rather than
|>> |> fixing the method as detailed in the FIXME comment.
|>> |> 3. In PortletURLTag286.doStartTag(), rather than implementing the
FIXME
|>> |> suggestion, you just erased the FIXME and the suggested code that was
|>> |> commented out.
|>> |>
|>> |> I think the commits where FIXMEs and TODOs are just erased should be
|>> |> rolled
|>> |> back except in those cases where the FIXME or TODO does not add
value or
|>> |> the issue pointed out by the FIXME or TODO was fixed. Does
anybody else
|>> |> have this opinion or am I making too much out of nothing here?
|>> |>
|>> |
|>> |
|>>
|>> >
|>
|>
|
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFHmdSc0Ji0BqEIlIURAkabAKCKb1qpYiWTrmTF47sY1hV2J08hmwCfbmjN
uF7NN81P9nzG0Lq4vT0A0xo=
=vxNF
-----END PGP SIGNATURE-----
