1 2 3 Previous Next 42 Replies Latest reply on May 27, 2009 6:43 PM by jbalunas

    Contributing a Patch

      Hi,

      I'm a fan of RichFaces, and would like to contribute where I can. I found a few simple Jira issues that I can handle, and have a fix for issue
      https://jira.jboss.org/jira/browse/RF-2357 ready for review. I didn't see a description of exactly how to contribute patches, so can someone please let me know the steps needed to submit a patch?

      In addition, if I have questions on how best to implement a fix for a Jira issue, is this forum the right place to discuss potential fixes?

      Thanks
      Kevin

        • 1. Re: Contributing a Patch
          nbelaevski

          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

            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

              I'll assign the issue to you, and thanks for the patch :-)

              -Jay

              • 4. Re: Contributing a Patch
                jbalunas

                What's your username? I can't find it under klester.

                Thanks,
                Jay

                • 5. Re: Contributing a Patch

                  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

                    klester, it looks like you are not in a group that can be assigned jira's (Commiter).

                    • 7. Re: Contributing a Patch

                      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

                        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

                          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

                            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

                              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

                                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

                                  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

                                    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

                                    1 2 3 Previous Next