Fiddle with handleCompareWith to make it cleaner

[robertbrignull]

The aim of this PR is to make the code for handleCompareWith cleaner and clearer.

I've been reading the query history commands a lot recently, and handleCompareWith keeps catching my eye. It's unusual because it's the only command we have that actually uses both singleItem and multiSelect, because it wants to know which of the selected items you actually clicked on so it can use that one as the "from" part of the comparison. Unfortunately the method is also rather confusingly written with lots of redundant checks and generally confusing logic.

This PR should theoretically be completely behaviour preserving. I'm not aiming to change any behaviour, but then the this.compareWithItem field in particular is new to me so I might have misunderstood something in its operation.

I haven't split up the commits into separate tiny changes because they're all on top of each other and I wasn't thinking about it as I wrote this. I recommend just reading the final code and seeing what you think of it. I could go back and split it up, but I hope a descriptive list will be just as good. Let me know what you think. The general list of things that are a little wrong in the original code and are changed by this PR are:

• The use of determineSelection is unnecessary.
  • This is a change also made in Remove determineSelection and simplify selection commands for the query history #2356, so see that PR for a full description and justification.
• The code checks that the items are a LocalQueryItem and then a CompletedLocalQueryItem in multiple places.
  • We check singleItem at the start of handleCompareWith, but then we check it again in findOtherQueryToCompare, and we only check multiSelect in findOtherQueryToCompare.
  • The error message is different in each place, and also talks as if it's about a single query but doesn't make it clear which of the selected queries it means.
  • This PR makes so we check both singleItem and multiSelect once at the start of handleCompareWith and provide a consistent error message.
• We use this.compareWithItem without checking that it is one of the selected queries.
  • This maybe theoretically is fine, since the treeView.onDidChangeSelection should have fired and updated this.compareWithItem. But maybe it's possible that didn't fire or they happen in the wrong order.
  • I check that this.compareWithItem is one of the elements from multiSelect.
  • I will write up tomorrow some info on this.compareWithItem because I expect most people won't have encountered it before, so I'll try to summarise how I think it works.
• We do a lot of dubious type casting because we don't utilise the type system to enforce things for us
  • I add isSuccessfulCompletedLocalQueryInfo and use that at the start to safely cast everything to a CompletedLocalQueryInfo. We can then remove a lot of casts and unnecessary error checks.
  • I also use isSuccessfulCompletedLocalQueryInfo in findOtherQueryToCompare when we're searching the list of all history items for possible comparisons.
• There's a big try-catch and we emit telemetry when really they're user errors
  • I move the try-catch to only be around findOtherQueryToCompare and just show the notification without telemetry.

Checklist

• CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
• Issues have been created for any UI or other user-facing changes made by this pull request.
• [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

[charisk]

Thanks for tidying this up, the changes LGTM. I only have some minor suggestions that you can ignore.

I've not tested this locally but it looks like there's some code coverage around it.

[elenatanasoiu]

The previous code had a comment explaining a bit why we're doing this. That meaning is lost a little bit here. Is there something we can do to make it clear again?

🤔 We could wrap this condition in a method like you've done in other places. Something like firstUnselectedItem()?

[robertbrignull]

I've updated the comments to be a bit clearer. Does that make it better?

[elenatanasoiu]

Looks good!