Sparkle assumes that greater versions are always newer, which is wrong

[sparkle-project]

Sparkle assumes that greater versions are always newer, which is wrong

[andymatuschak]

**Peter Hosey (boredzo)* wrote on 2009-06-24:*

The fix is for Sparkle to sort items by version, not date. I've created
a patch that does this by making SUStandardVersionComparator a subclass
of NSSortDescriptor.

[andymatuschak]

**Hofman (cmhofman)* wrote on 2009-06-25:*

That's why there is a delegate method
-bestValidUpdateInAppcast:forUpdater:. But I actually also think sorting
by date is rather arbitrary, and sorting by version makes more sense.

[andymatuschak]

**Hofman (cmhofman)* wrote on 2009-06-25:*

As for the proposed patch, it should not be an option for two reasons.
First, SUVersionComparision is not a subclass of NSSortDescriptor, so it
can't be just made that because of backward compatibility. Moreover, the
patch does not take into account the possible ovverride of
SUStandardVersionComparison. Last but not least, it certainly should not
be added before 1.5 (whch is supposed to be imminent for as long as I
can remember ;)

[andymatuschak]

**Peter Hosey (boredzo)* wrote on 2009-06-25:*

Hofman:

“First, SUVersionComparision is not a subclass of NSSortDescriptor, so
it can't be just made that because of backward compatibility.”

What backward compatibility restriction is that? As long as the app
developer recompiles with the new framework, it should work just fine.

The patch does not change any existing functionality in
SUStandardVersionComparator; it only adds the NSSortDescriptor nature.

“Moreover, the patch does not take into account the possible ovverride
of SUStandardVersionComparison.”

I disagree. A subclass that overrides compareVersion:toVersion: should
continue to work, and should inherit the same dual usefulness as an
NSSortDescriptor.

[andymatuschak]

**Hofman (cmhofman)* wrote on 2009-06-26:*

“First, SUVersionComparision is not a subclass of NSSortDescriptor, so
it can't be just made that because of backward compatibility.”

What backward compatibility restriction is that? As long as the app
developer recompiles with the new framework, it should work just fine.

The patch does not change any existing functionality in
SUStandardVersionComparator; it only adds the NSSortDescriptor nature.

The developer also has to change the code. My point is that your
proposal REALLY changes the Sparkle API, and this should not be done at
this point in the development cycle.

“Moreover, the patch does not take into account the possible ovverride
of SUStandardVersionComparison.”

I disagree. A subclass that overrides compareVersion:toVersion: should
continue to work, and should inherit the same dual usefulness as an
NSSortDescriptor.

But it's not used, you're using SUStandardVersionComparison, not any
subclass. So I disagree with your disagreement.

[andymatuschak]

**Peter Hosey (boredzo)* wrote on 2009-06-26:*

The developer also has to change the code. My point is that your
proposal REALLY changes the Sparkle API, and this should not be done at
this point in the development cycle.

What application code would this change? I used it in Adium by just
dropping the new framework in; I didn't have to change a single line of
Adium code.

Even if you subclass SUStandardVersionComparator, I don't see what
application code the application developer would have to change. The
#import statement remains the same, and the subclass only declares its
immediate parent class, which remains SUStandardVersionComparator.

It occurs to me that -init was missing. This was a bug, an omission on
my part, not an intentional part of the design. I have now fixed it in
revision 352 on my branch.

I don't see what part of this would require an application developer to
add, change, or delete a single line of code.

“Moreover, the patch does not take into account the possible ovverride of SUStandardVersionComparison.”

I disagree. A subclass that overrides compareVersion:toVersion: should continue to work, and should inherit the same dual usefulness as an NSSortDescriptor.

But it's not used, you're using SUStandardVersionComparison, not any subclass. So I disagree with your disagreement.

I'm using SUStandardVersionComparator directly in SUAppcast because
that's all I need. I don't need to subclass it. I don't see what that
has to do with the possibility of subclassing it; as I wrote previously,
any subclass of SUStandardVersionComparator should continue to work.

I don't see what part of this would require an application developer to
change or delete their subclass.

[andymatuschak]

**Hofman (cmhofman)* wrote on 2009-06-26:*

SUVersionComparision is a protocol, not a class. That basically
invalidates all your comments.

And I don't think it makes sense to compare version numbers
inconsistently. If a developer needs a different way to compare version
number than this says something about the format of the version numbers
he uses, and that is relevant for this situation just as well. It
doesn't matter what YOU need, it matters what others can expect.

[andymatuschak]

**Peter Hosey (boredzo)* wrote on 2009-06-26:*

SUVersionComparision is a protocol, not a class. That basically
invalidates all your comments.

No, it doesn't. I've only ever been talking about the class,
SUStandardVersionComparator. I thought you were talking about it because
you keep talking about subclasses; protocols don't have subclasses
because they aren't classes.

I never touched the SUVersionComparison protocol. It's still there,
unchanged.

And I don't think it makes sense to compare version numbers
inconsistently. If a developer needs a different way to compare version
number than this says something about the format of the version numbers
he uses, and that is relevant for this situation just as well.

That's why SUStandardVersionComparator is only the standard version
comparator. That hasn't changed: it was before, and still is. The only
difference is that now you can use it anywhere you can use a sort
descriptor.

I think I've finally guessed your complaint: You don't like that
SUAppcast sorts using the standard version comparator, and doesn't look
for an application-defined version comparator. Since application-defined
version comparators will conform to the SUVersionComparison protocol but
probably not be subclasses of the SUStandardVersionComparator class, you
see this as a breaking API change.

It's not. It's an omission, and it's fixable.

That does lead to a question, though: How should SUAppcast obtain the
application's version comparator? It doesn't have a reference to its
SUUpdater; should I add one? (sharedUpdater is plainly wrong, since the
appcast may be for a different updater.)

[andymatuschak]

**Peter Hosey (boredzo)* wrote on 2009-06-26:*

One possible way: appcast.delegate.updater. I see two problems with
this:

1. SUUpdateDriver doesn't include a getter for obtaining its updater (fixable).
2. This requires assuming that the appcast's delegate is non-nil and is an update driver.

The more I look at the code and think about this, the more I think that
SUAppcast is not the proper place to perform this sort. I should do it
in SUBasicUpdateDriver instead, so that I can sort by the custom
comparator if there is one.

[andymatuschak]

**Peter Hosey (boredzo)* wrote on 2009-06-26:*

It's becoming clear now that SUStandardVersionComparator-as-
NSSortDescriptor is not a correct solution. It's on the right track, but
it is not correct.

I now specifically and explicitly ask that Sparkle maintainers not
merge in my current branch. I'll create a new branch for the correct
fix.

[andymatuschak]

**Andy Matuschak (andymatuschak)* wrote on 2009-07-06:*

This bug is unfortunate.

Peter, you're absolutely right that Sparkle's present behavior is
stupid. I think you're right that the sort should take place in the
update driver; this even makes sense when thinking about the separation
of responsibility. The appcast provides all the available updates, and
the client (the update driver) chooses which one(s) it cares about.

It'd be great if we could get this fixed in a totally non-breaking way
for 1.5, but I'm concerned that this would break poorly written but
presently working feeds which have a most recent update which is not the
highest in version number.

[kornelski]

Fixed