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

[Peter Hosey]

When Sparkle looks for a usable version among the appcast items (see #228485), it sorts the items by date.
This is a problem, demonstrated by Adium 1.4b6.

Our appcast for Adium betas includes both 1.4b7 and 1.3.5; of these, 1.3.5 is actually the newer (more recent) release. Because Sparkle sorts the items by date, it looks at 1.3.5 first; it finds that 1.3.5 will run on the host, so it stops looking for more versions. However, 1.3.5 is a lower version number than 1.4b7, so it tells the user that “1.4b6 is the latest version available!”—which is wrong, since b7 is available.

We're not sure exactly which version of Sparkle we're using (it was a build from source, and the developer who committed it didn't note the version in his commit message). All I know is that CFBundleVersion is 340, for whatever that's worth.

[Peter Hosey (boredzo)]

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.

[Hofman (cmhofman)]

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.

[Hofman (cmhofman)]

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 ;)

[Peter Hosey (boredzo)]

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.

[Hofman (cmhofman)]

> “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.

[Peter Hosey (boredzo)]

> 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.

[Hofman (cmhofman)]

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.

[Peter Hosey (boredzo)]

> 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.)

[Peter Hosey (boredzo)]

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.

[Peter Hosey (boredzo)]

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.

[Andy Matuschak (andymatuschak)]

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.