Hi Arjun, please find below some comments
> > Arjun Nair reassigned XX-5552: > ------------------------------ > > Assignee: George Niculae (was: Arjun > Nair) > > The patch looks good and adds some much needed, and overdue > functionality to the phonebook. There some minor changes I > would like to address. > > As Paul pointed out, the firstname/lastname in > SearchConstants.properties & SearchConstants.java > inconsistency Yup, something that should be removed > > PhonebookDataSource now defines public static Strings such > as FIRST_NAME. Hence, there is no need for a second > definition inside UserPhonebookSearch. This also applies to > entries inside attributes FIELDS_GENERAL, FIELDS_HOME, and > FIELDS_OFFICE in the Details class. I tried this - somehow if I use the PhonebookDataSource defined constants in UserPhonebookSearch the phonebook grid is not displayed anymore. I am going to investigate why. > > We should try and keep HTML out of the java files. In the > AvatarSection, we could probably instead use an Img, and a > Label widget, nested inside a VLayout. Yup, I forgot to change this > > In the AvatarSection class, you need to explicitly check > for null entries when you do a > record.getAttributeAsString(PhonebookDataSource.FIRST_NAME). > Otherwise, on a null entry, the first name will be printed > as "null" in the UI. OK > > IMO, the function 'private static JSONObject > buildJsonRequest(ValuesManager form)' should be implemented > as a part of PhonebookDataSource, in order to keep that > clear Model, View seperation. What do you think? > > On that same note, although it's not a part of this patch, > we should be overriding the functions 'protected Object > transformRequest(DSRequest request)' and 'protected void > transformResponse(DSResponse response, DSRequest request, > Object data)' inside the PhonebookDataSource class > definition itself. If needed, we can override > transformResponse a second time inside UserPhonebookSearch > to set the sizeLabel. I am going to move this logic in data source, where it belongs. > > We should perform some validation before saving the data > (at the minimum check to make sure we are not saving a null > entry - make it mandatory to have at least a first/last name > + phone/email). This should ideally be done both on the gwt > side and the rest side. Will add the validation > > Rather than calculating and adding the Avatar URL in the > UserPhonebookSearchResource class, we could do it in the > AddressBookEntry class. See: > http://thread.gmane.org/gmane.comp.voip.sipx.devel/20148 OK > > Create a private static class DetailsSection, that extends > SectionStackSection. Create the editBUtton, addButton, and > removeButton inside this class. > > Finally, > > Rather than creating a section stack with the listGrid at > the top and the detailsSection at the bottom, we can use the > new ListGrid row expansion feature in 2.0 to implement the > details section. We should be able to do this by overriding > the getExpansionComponent() function in ListGrid to return > the DetailsSection, and setting ListGrid.canExpand to true. > See: > http://www.smartclient..com/smartgwt/showcase/#grid_nested_form_new_category Great! I was looking for such a feature in 1.3, good that we have it now with 2.0 - Clearly it needs to be done this way. > George, let me know if you've got your hands full with > another issue. I can make these changes myself, and take the > patch in. It'd be nice to have this in the mainline soon. I will focus on that one and defer the current work since I really want to see it in. Meanwhile maybe you can took a look on http://track.sipfoundry.org/browse/XX-7304 - a rebase for XX-5552 is needed if gmail import makes it in mainline. Thanks, George _______________________________________________ sipx-dev mailing list [email protected] List Archive: http://list.sipfoundry.org/archive/sipx-dev Unsubscribe: http://list.sipfoundry.org/mailman/listinfo/sipx-dev sipXecs IP PBX -- http://www.sipfoundry.org/
