Hi Peter, Thanks for your feedback, I’ve just committed some of these changes. I’ve also renamed the get**Url methods on CMISBrowserBaseService to follow Apple’s naming recommendations [1], hope this is OK with you.
I will try and complete the HttpUploadRequest change today as well. Regards, Gavin [1] https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/NamingMethods.html On 5 May 2014, at 07:55, Sutter, Peter <[email protected]> wrote: > Hi Gavin, > > yes agree with your second proposal (supply start/end data with flag for > HttpUploadRequest). > > I moved the media type constants there because I needed > kCMISMediaTypeOctetStream also for browser binding. And OpenCMIS library > has those constants also in the > org.apache.chemistry.opencmis.commons.impl.Constants class. So at least > the constant kCMISMediaTypeOctetStream should be there. Don’t mind if you > want to move the others back. > > We have a similar method in our coding which is on a NSString category but > it’s also fine for me if we change it back to a private method on the > CMISURLUtil class. > > > Best regards, > Peter > > On 5/2/14, 5:33 PM, "Gavin Cornwell" <[email protected]> wrote: > >> Hi, >> >> I’ve completed the agreed refactoring on the browser binding branch and >> also fixed the underlying issues causing the last remaining tests to >> fail. With the fixes in place all tests are now passing against both >> bindings. I’ve therefore changed the test config file to run the tests >> against both bindings by default. >> >> To fix the JSON parsing issue I’ve added a hack that should be by-passed >> if Apple ever fix the bug. We now remove the minValue and maxValue >> properties from the JSON if a parsing error occurs and re-try and parse >> operation. >> >> I took a look at CMISHttpUploadRequest, I agree we need to move the >> prepareXMLWithCMISProperties method out, we could introduce an elaborate >> mechanism where custom HttpUpload/DownloadRequest objects could be >> plugged into the network provider, but I think the easiest/most sensible >> thing to do is to just expose the underlying base64Encoding flag as a >> parameter and use the combinedStream stream feature you’ve added. Both >> bindings can then supply the start/end data and set the flag >> appropriately, what do you think? >> >> A couple of other questions following the changes provided in CMIS-794... >> >> Was there a particular reason why the atom pub specific media type >> constants (kCMISMediaTypeService etc.) got moved into the common >> CMISConstants file? I personally think these should be moved back to >> CMISAtomPubConstants as they’re only used by that binding, do you agree? >> >> Finally, was there a particular reason to make the replacePathWithPath >> method in CMISURLUtil a category? It’s only used in this class so just >> wondering why it’s not a simple private method? >> >> Have a great weekend. >> >> Regards, >> >> Gavin >> >> >> >> On 1 May 2014, at 16:35, Gavin Cornwell <[email protected]> >> wrote: >> >>> Hi Peter, >>> >>> Excellent. >>> >>> I have assigned CMIS-794 to myself, I’ll commit this tomorrow as well. >>> >>> I will also take a look at the CMISHttpUploadRequest issue you mention. >>> >>> Regards, >>> >>> Gavin >>> >>> >>> >>> On 1 May 2014, at 15:53, Sutter, Peter <[email protected]> wrote: >>> >>>> Hi Gavin, >>>> >>>> I just uploaded the current state of my implementation: >>>> https://issues.apache.org/jira/browse/CMIS-794 >>>> >>>> Now all methods of the interfaces should be implemented. I have also >>>> tested the browser binding implementation in our app and it works very >>>> nicely as far as I can see. >>>> >>>> There is maybe one area where I¹m not really sure because the >>>> CMISHttpUploadRequest class contains atom pub specific coding >>>> (prepareXMLWithCMISProperties) which should probably be moved down to >>>> the >>>> respective atom class. >>>> And still there is the JSONSerializer bug with very large/small double >>>> values where we need to decide how we deal with it. >>>> >>>> You can start with your part 2 refactoring, no problem. Will only do >>>> minor >>>> changes in the next working days, if any. >>>> >>>> Best regards, >>>> Peter >>>> >>>> >>>> On 5/1/14, 11:03 AM, "Gavin Cornwell" <[email protected]> >>>> wrotsue: >>>> >>>>> Hi Peter, >>>>> >>>>> Great work on the browser binding so far, it¹s really coming together. >>>>> >>>>> As you¹ve probably seen by now, I did part 1 of the re-factoring we >>>>> agreed upon previously (updating property names). >>>>> >>>>> Part 2 is going to be a bit more disruptive as it¹s going to involve >>>>> renaming and moving quite a few classes (ensure all Atom classes are >>>>> named as such). So I thought it would be best to co-ordinate this with >>>>> you to avoid a merge headache for you and I. >>>>> >>>>> I was planning on doing this tomorrow, do you think you¹ll have your >>>>> latest set of changes committed by then? >>>>> >>>>> As a side note, I looked into the UriBuilder classes last night and >>>>> found, as you¹d hinted at, they are pretty specific to the AtomPub >>>>> binding so I¹m going to rename them as such and move the constants >>>>> from >>>>> the generic CMISBindingSession class to the CMISAtomPubConstants >>>>> class. >>>>> >>>>> Following this, I¹ll be available to help out with the actual >>>>> implementation, if there are any areas you¹d like me to look at (that >>>>> are >>>>> not on your critical path) please let me know. >>>>> >>>>> Regards, >>>>> >>>>> Gavin >>>> >>> >> >
