On Tue, May 5, 2009 at 11:45 AM, Partha Aji <p...@redhat.com> wrote:
[snip]
>> Can we get a unit test for the comparator? Sure it looks like a trivial
>> class,
>> but let's test it so that we don't break it.
>
> This is not a new add, we have been using DynamicComparator class in the
> List Tag over the past few years. All I did was add another constructor...
> Sure can add a unit test..
Great thx. sorry didn't realize we aleady had the class.
[snip]
>> why are we asking an Action for a snippet? I've never seen us make
>> dependencies
>> between actions. Seems like this should be in a Manager, a Helper class or
>> just
>> simply operate on the RequestContext directly i.e. (CobblerSnippet)
>> ctx.getParam("cobbler.snippet")
>> something along those lines. I'm not a big fan of the DeleteAction
>> calling back on the
>> DetailsAction. I'm also going to assume that these Actions are NOT
>> maintaining state because
>> that would be a no-no.
>
> Here is the issue. Name is changeable even on the edit page for a
> CobblerSnippet. So this changes the logic a little bit..
> For our logic to work we need to preseve the OldName as a formVar and then
> do things like if !oldName.equals(name) change the name of the snippet and
> if there is an error complain and load the original snippet using the
> oldName and not the new one (for that doesn;t exist as yet..)
>
> Also this is in Action becasue its using Reuqest and RequestCOntext.. I
> think of manager layer that deals with logic that is entirely based on non
> UI objects so that one can use it in XMLRPC layer also.. To me
> RequestContexts being used in Manager is a code smell. In this case
> getSnippet() method uses request heavily. Initially it was a private method
> in this class but then I had to copy paste the same exact method over to the
> Delete Confirm page (since I had to load a snippet and show its contents)
> and at that point was uing the same request parameters. So I said lets have
> the loading logic on one action. To mehaving a static method in a Action is
> acceptable as long as it deals with UI objects + and does no state member
> variable magic (as in only deals with objects passed to it).. One argument
> that could ve been made was that this should be a default access method and
> not a public static, I am going to change to default/package acceess ...
Thanks for the detailed explanation. I'm ok with your change then.
[snip]
> No form vars, they are calling a general list page.. That change was
> intentional.
Good wanted to make sure it was intentional.
>> createMode? hrm that smells fishy
>
> Same action is used for both creating a snippet and loading an existing
> snippet.. Will think on how to fix this..
I guess it's ok. I know I did a similar approach on the EditChannelAction
or ChannelEditAction, /me can't remember the name.
>> Isn't there something in BeanUtils that can do this type of comparator? If
>> you
>> already looked there, then my apologies.
>
> The does exist a bean comparator, but not in the version of bean utils we
> have ..
> http://commons.apache.org/beanutils/commons-beanutils-1.7.0/docs/bean-collections/org/apache/commons/beanutils/BeanComparator.html
>
> Again as I mentioned above this class DynamicComparator has been in use for
> a couple of year in our list tag. I didn't want to invent a new way to do
> this..
Ok fair enough, I'm good with this. Thx again for the education.
[snip]
>> Did you intend to remove the WARNING above? seems way out of context for
>> this commit.
>>
>> The rest of the string cleanup looked good.
>
> There were servel duplicate keys called Warning. But I think I removed the
> wrong one.. Will fix this .. Thanks..
:)
>
>>
>>> diff --git a/java/code/src/com/redhat/rhn/frontend/taglibs/NoteTag.java
>>> b/java/code/src/com/redhat/rhn/frontend/taglibs/NoteTag.java
>>> new file mode 100644
>>> index 0000000..ddd4788
>>> --- /dev/null
>>> +++ b/java/code/src/com/redhat/rhn/frontend/taglibs/NoteTag.java
>>> @@ -0,0 +1,33 @@
>>> +/**
>>> + * Copyright (c) 2009 Red Hat, Inc.
>>> + *
>>> + * This software is licensed to you under the GNU General Public
>>> License,
>>> + * version 2 (GPLv2). There is NO WARRANTY for this software, express or
>>> + * implied, including the implied warranties of MERCHANTABILITY or
>>> FITNESS
>>> + * FOR A PARTICULAR PURPOSE. You should have received a copy of GPLv2
>>> + * along with this software; if not, see
>>> + * http://www.gnu.org/licenses/old-licenses/gpl-2.0.txt.
>>> + *
>>> + * Red Hat trademarks are not licensed under GPLv2. No permission is
>>> + * granted to use or replicate Red Hat trademarks that are incorporated
>>> + * in this software or its documentation.
>>> + */
>>> +package com.redhat.rhn.frontend.taglibs;
>>> +
>>> +
>>> +/**
>>> + * NoteTag
>>> + * @version $Rev$
>>> + */
>>> +public class NoteTag extends ToolTipTag {
>>> +
>>> + /**
>>> + * Comment for <code>serialVersionUID</code>
>>> + */
>>> + private static final long serialVersionUID = -7743415103174952344L;
>>> +
>>> + @Override
>>> + protected String geTypeKey() {
>>
>> geTypeKey? Did you mean getTypeKey?
>>
>>> + return "Note";
>>> + }
>>> +}
>>
>>
>> What's this for? some better javadoc would be useful and where is the unit
>> test?
>> Why can't folks use <rhn:tooltip type="NOTE" ... > instead of using
>> another tag?
>
> I was kinda confused myself on the better approach
> <rhn:note key ="...."/>
> vs
> <rhn:tooltip key ="...." type = "Note"/>
> thing is I think 'Warning:' , 'Tip:' and 'Note:' IMHO covers all the cases
> we'd ever want to use.. so why the extra text :)...
>
> I am ok with a vote on this...
So you mean rhn:warning, rhn:tip and rhn:note are the 3 we need so avoid
using the type="TYPE"? I guess that makes sense. Just figured why create
extra classes when the one suits our needs :) not really that serious
a problem just thought it wasn't that necessary.
>
>>
>>> diff --git
>>> a/java/code/src/com/redhat/rhn/frontend/taglibs/ToolTipTag.java
>>> b/java/code/src/com/redhat/rhn/frontend/taglibs/ToolTipTag.java
>>> index 4a4e284..9190e57 100644
>>> --- a/java/code/src/com/redhat/rhn/frontend/taglibs/ToolTipTag.java
>>> +++ b/java/code/src/com/redhat/rhn/frontend/taglibs/ToolTipTag.java
>>> @@ -39,7 +39,7 @@ public class ToolTipTag extends TagSupport {
>>> private static final long serialVersionUID = 7202597580376186072L;
>>>
>>> private String key;
>>> - private String text;
>>> + private String typeKey = "Tip";
>>
>> Should these be constants? If not why not just add a default ctor
>> public ToolTipTag() {
>> key = null;
>> text = null;
>> typeKey = "Tip";
>> }
>>
>> I know we do this in other places, just wondering if you meant it to
>> be a constant
>> or not.
>
> Well if you do a <rhn:tooltip, it uses the above class..
>
> Me is confused with the question " Should these be constants? "
I saw a string being defined and thought you might have wanted
private static final string TYPEKEY = "Tip";
but you didn't intend it to be a constant, you really are just defaulting
it to the value of "Tip" and letting the setter take care of it.
> I mean they being set when the class is instantiated anyway.. I don;t see
> difference in them being in the constructor vs being set when they are
> member variables..
Yeah that's fine. Ignore.
>> should the class be a parameter to the tag? or do we always want it to
>> be small-text?
>
> Yes thats the intent of the tooltip tag..
Ok, I'm fine with it.
>>
>>> writer.write(strong.render());
>>> if (!StringUtils.isBlank(key)) {
>>> writer.write(ls.getMessage(key));
>>> @@ -82,7 +82,10 @@ public class ToolTipTag extends TagSupport {
>>> }
>>>
>>> protected String geTypeKey() {
>>> - return "Tip";
>>> + if (StringUtils.isBlank(typeKey)) {
>>> + return "Tip";
>>> + }
>>> + return typeKey;
>>
>> I think the setTypeKey should check the incoming value and set it to
>> Tip if it is
>> blank. Also what if I set it to 'feefifoefum' what will this tag do?
>>
>> unit test for this class?
>>
> ***feefifoefum***
>
> May be I 'll just remove type of tool tip so you are forced to use
> rhn:tooltip or rhn:warning or rhn:note and not add your own..
Yep, if we are going to have concrete tags as you stated earlier,
then it doesn't make sense to have tooltip be 'flexible'.
[snip]
>> WHEW! that was one long commit :)
>> jesus
>
> Thanks for reviewing.. Will add the mods as suggested :)
No prob, good work and thanks again for the explanations.
jesus
_______________________________________________
Spacewalk-devel mailing list
Spacewalk-devel@redhat.com
https://www.redhat.com/mailman/listinfo/spacewalk-devel
