Re: JavaHL bindings for the conflict resolution callback API

On Tue, 10 Jul 2007, Mark Phippard wrote:

> On 7/9/07, Daniel Rall <dlr@collab.net> wrote:
> >On Mon, 09 Jul 2007, Mark Phippard wrote:
> >
> >> On 7/9/07, Daniel Rall <dlr@collab.net> wrote:
> >...
> >> >> On 6/28/07, C. Michael Pilato <cmpilato@collab.net> wrote:
> >> >>
> >> >> >> I like the concept, but in addition to putting a "path" into the
> >> >> >> svn_client_conflict_description_t, I'd recommend putting an
> >> >"adm_access"
> >> >> >> baton. Otherwise, won't callback implementers will be lacking a
> >vital
> >> >> >tool
> >> >> >> for working copy manipulation?
> >...
> >> >While implementing conflict resoultion callback bindings for JavaHL, I
> >> >ran into a bit of a snafu with regard to this key data structure:
> >> >JavaHL does not wrap libsvn_wc; it stops at libsvn_client.
> >...
> >> >Am I going to need to wrap parts of libsvn_wc for JavaHL to make
> >> >proper use of the conflict resolution callback? If so, which bits are
> >> >important?
> >>
> >> I do not understand why you would need to. Can't you just "swallow"
> >> that parm in the C++ code and not even expose it to Java?
> >
> >Sure -- this is the approach I took last Friday.
> >
> >> It seems like the Java code just needs to be able to set a return
> >> value if it wants the caller to do something. Otherwise it is up to
> >> the Java code to take the other parms it will get and do something.
> >
> >For some reason I was under the impression that when the callback
> >returned a result like svn_wc_conflict_result_resolved (that's
> >"resolveD", as in "I already did it"), that it was up to the callback
> >itself to perform the resolution. However, after examining the code
> >in libsvn_wc/merge.c:svn_wc__merge_internal(), this does not seem to
> >be the case. This is Good -- looks like the WC merge handling paired
> >with the new svn_wc_conflict_result_t code should be sufficient for
> >JavaHL's use cases.
>
> Dan,
>
> Trying to write an implementation of this callback in Subclipse

Cool! You should hold off another day or two, though. The binding
isn't complete yet. :-) (This was mentioned in the commit log
message.)

> and having a few problems:
>
> 1) The callback method throws SubversionException. This has to be
> changed as it is not possible for the implementation of the callback
> to construct an object of that type. Also, since this is a callback
> who is responsible for catching this exception? Shouldn't the return
> value just indicate if there was an error?

SubversionException is a public class with a protected constructor.
You can create an instance of it via sub-classing. You may be able to
do this in an ultra light-weight fashion using an anonymous sub-class.

The exception is currently caught by ConflictResolutionCallback.cpp's
resolve() instance method:

  jint jresult = env->CallIntMethod(m_conflictResolver, mid, jdesc);
  if (JNIUtil::isJavaExceptionThrown())
    // ### FIXME: If an exception is thrown by our conflict resolver,
    // ### convert it into an svn_error_t * and return it.
    return SVN_NO_ERROR;

(Note that this from my WC, and differs a tad from what's in the
repository this mornin'.)

The Java ConflictResolutionCallback.resolve() method returns the
result of the resolution attempt. Changing it to return some type of
error code or Exception object would make its API a bit more
complicated, since the result would have to be returned via an
intermediate object, or instance state (ick!).

> 2) What is with all of the usage of Object? Are those just place
> holders for now and you are going to replace them with new classes you
> need to create? As a user of the API I have no idea what I am
> receiving in those parameters -- which seem like important parms, and
> also I do not know what to return from the callback.

Yup, those are just placeholders for the unimplemented portions of the
API. I've got a patch in-progress to plug those holes.

While working on this patch, I have run into one discrepancy with
existing JavaHL patterns: I need to convert from a Java pseudo-"enum"
(a series of integer values defined in public static final fields)
into the enum values from svn_wc_conflict_result_t.

To date, JavaHL has typically used its EnumMapper class to convert
from Subversion enum values into Java "enum" values, but doesn't seem
to ever need to go the other way around. There's only one place where
use of the EnumMapper pattern has been broken, in the Java API's
DiffSummary.DiffKind inner class (new in 1.5, implemented as a
type-safe enum directly coupled to the enum values from the
svn_client_diff_summarize_kind_t enum).

Any recommendations on how to best fit the enum conversion process
into the existing patterns? Personally, I don't see a huge need for
the pattern used by EnumMapper to decouple the "enum" values used in
Java from the real enum values declared by the Subversion API. The
pattern is used pervasively throughout JavaHL, but has never been used
to do this type of "reverse" conversion.