post-rename detect screen doesn't allow review

[metajack]

Here: https://critic.hoppipolla.co.uk/369c5b83?review=468

Hit 'm' and then select 'Any' and 'Any' and hit ok. Now you can't actually perform the review and hitting spacebar scrolls to the bottom them pops you back to the top. It's all very weird and I have no idea what I'm supposed to do here.

cc @jgraham

[jensl]

The reason you can't mark changes as reviewed via the "move detection" view is essentially that it's not (necessarily) a complete view of the changes; if a file contains both regular modifications and moved code, the "move detection" view only shows the latter. Critic's UI will as a rule only let you mark changes as reviewed from a view that accurately displays them. Having said that, there are obviously cases where the "move detection" view really does display all the changes in a meaningful way; no effort has been spent detecting such cases and enabling reviewing then.

The spacebar behavior is arguably a bit weird on that page. I'm not sure what's going on; it's certainly not exactly intentional.

[metajack]

It's not clear what's going on on that page to me at all. Why am I on a different page when I just asked for it to detect renames? For example, GitHub shows me renames without needing to take me to some other page. I found it very disorienting.

I'd prefer it if it detected non-edited renames automatically and not clog up the review screen. Then I wouldn't have reached for this strange function in the first place :)

[jensl]

Yes, the whole feature is rather under-documented. :-) (A news items was posted in Opera's internal system when it was originally added, explaining it to users there, but this of course doesn't help users in other systems.)

So, what it does is detect moved code using somewhat fuzzy matching, and then displays a side-by-side diff of the source of the move (i.e. a block of code that was removed from somewhere) and the target, to allow you to review changes made to the code while moving it. If it was a straight move, the diff will be "empty."

Much improvement is possible. The implementation is for instance somewhat stupid and gets confused when a changeset renames several files that share common elements (for instance some kind of license/copyright header.) If file A is renamed A^ and file B is renamed B^, it will often decide that the header in both A^ and B^ was copied from A (or B).

It would of course also be good to detect renamed files (at least) automatically, and include that information directly in the basic diff display. The "move detection" view is really not intended for that case, nor is it particularly good at it. It's purpose is to make it possible to review non-straight moves.