-
1. Re: Contributing a Patch
nbelaevski Apr 2, 2009 6:15 PM (in response to klester)Hello Kevin,
Great, we will be glad to receive your contribution! Please create issue of type "Patch" in JIRA and attach necessary files there. Then link the newly created issue with the ones the patch is related to. Someone from the team will pick it up and check.
And yes, that's the right forum to ask development questions. -
2. Re: Contributing a Patch
klester Apr 2, 2009 9:43 PM (in response to klester)Thanks for the info, Nick.
After grabbing the latest code for one last test, the menu.js is no longer compiling for menu-components (unrelated to my changes). I'll make the patch as soon as the code compiles again.
I was going to work on issue https://jira.jboss.org/jira/browse/RF-3755 next since it's related to the other issue on which I worked. It doesn't look like I have to ability to assign this issue to myself, but I just wanted to let you know that I'll be working on it so that someone else doesn't grab it. Feel free to assign it to me if you like. -
3. Re: Contributing a Patch
jbalunas Apr 3, 2009 6:38 AM (in response to klester)I'll assign the issue to you, and thanks for the patch :-)
-Jay -
4. Re: Contributing a Patch
jbalunas Apr 3, 2009 6:40 AM (in response to klester)What's your username? I can't find it under klester.
Thanks,
Jay -
5. Re: Contributing a Patch
klester Apr 3, 2009 1:10 PM (in response to klester)I use the same 'klester' login for Jira (to watch certain issues, for example) and for this forum. Do I need to create a different Jira login or update my account somehow to be assigned bugs?
Kevin -
6. Re: Contributing a Patch
rruss Apr 3, 2009 2:46 PM (in response to klester)klester, it looks like you are not in a group that can be assigned jira's (Commiter).
-
7. Re: Contributing a Patch
klester Apr 3, 2009 3:34 PM (in response to klester)No prob - I figured that people were not automatically made committers. I'll just work on the issue unofficially.
The patch has been created and uploaded as the following issue https://jira.jboss.org/jira/browse/RF-6701. Let me know of any feedback/issues with the patch. -
8. Re: Contributing a Patch
klester Apr 4, 2009 10:09 PM (in response to klester)I just added a new patch that should be more consistent with how other components are disabled. You may ignore the first attachment in the issue I created.
In other news, I just saw issue https://jira.jboss.org/jira/browse/RF-6622 which is closely related to the changes I just made (adding new PickList attributes for "fastMoveControlsVisible" and "moveControlsVisible"). I was going to take a crack at this issue next, but I have a question on my proposed fix, first.
These attributes already exist in the ListShuttle component, and in that component the controls are conditionally rendered based on the returned value of the OrderingComponentRendererBase.ControlsHelper.isRendered method for the control's corresponding helper class, which looks at these attributes. So the plan was to simply implement this method for the PickList's helper classes. The issue, though, is that the ControlsHelper.isRendered method signature takes a FacesContext and a UIOrderingBaseComponent. Since the UIPickList does not extend from the UIOrderingBaseComponent, you can't simply call the isRendered method passing in the UIPickList.
So the question is how to handle this. Would it be better to
a). modify the ControlsHelper.isRendered() method signature to simply take a FacesContext and a UIComponent, and then have the implementing methods do the necessary casting? This would allow the PickList helper classes to implement the isRendered method, which would be consistent with the ListShuttle component. This would require changing all the implementing classes method signatures to match the new one.
b). ignore the isRendered method for PickList, and simply put the logic in the PickListRenderer to conditionally show/hide the controls based on the new attributes?
My preference would be option A since I think its the cleanest - but I wanted to get feedback in case I'm missing something.
This issue is currently assigned to Nick, so sorry if I'm stepping on any toes, I just thought that since I'm already looking at that part of the code I could just knock it out and save Nick some work.
Kevin -
9. Re: Contributing a Patch
ilya_shaikovsky Apr 6, 2009 7:06 AM (in response to klester)At first thanks a lot for you work. And about your questions I also think that a) option is preferable. Nick will also recheck this and update the thread if he will think that I'm mistaken.
-
10. Re: Contributing a Patch
jbalunas Apr 6, 2009 7:15 AM (in response to klester)Agree with Ilya - Thank you for you contributions :-)
At first glance I also agree with plan A). I'd wait on Nick's opinion on this as he knows this code best.
-Jay -
11. Re: Contributing a Patch
nbelaevski Apr 6, 2009 8:35 AM (in response to klester)Hello,
Thank you for participation!
Variant "a" is ok, however I see proper implemented isRendered() methods in PickListControlsHelper:public boolean isRendered(FacesContext context, UIOrderingBaseComponent list) { return ((UIListShuttle) list).isMoveControlsVisible(); }
so looks like the only thing necessary is adding these attributes into picklist.xml file. -
12. Re: Contributing a Patch
klester Apr 6, 2009 12:40 PM (in response to klester)Nick, unless I'm missing something, the isRendered method in the PickListControlsHelper cannot be used by UIPickList in its current form. This is because the method expects a UIOrderingBaseComponent as a parameter, and it uses the UIOrderingBaseComponent to determine if the component should be rendered. But UIPickList does not extend UIOrderingBaseComponent, only UIListShuttle and UIOrderingList do, so it doesn't look like UIPickList can use this method in its current form.
Hence I was going to change the isRendered's method signature to take a UIComponent, which is extended by UIPickList, UIListShuttle, and UIOrderingList so that they can all use the isRendered method.
Am I misunderstanding something that would enable this method to work as-is for the UIPickList? -
13. Re: Contributing a Patch
nbelaevski Apr 6, 2009 6:57 PM (in response to klester)You are right, UIPickList is not an instance of UIOrderingBaseComponent, I was confused by the helpers code in PickListRenderer. It's ok to change method signature to UIComponent; this should not break compatibility as these are our internal methods.
-
14. Re: Contributing a Patch
klester Apr 6, 2009 7:08 PM (in response to klester)Ok, I'll work on this tonight. Should I update my current patch in the 'patch' issue I created earlier, or would you recommend creating a new patch per issue?
Kevin