lazygit icon indicating copy to clipboard operation
lazygit copied to clipboard

Add support for setting the similarity threshold for detecting renames (closes #2904)

Open isti115 opened this issue 2 years ago • 11 comments

  • PR Description Implement the feature as discussed in #2904.

  • Please check if the PR fulfills these requirements

  • [x] Cheatsheets are up-to-date (run go run scripts/cheatsheet/main.go generate)
  • [x] Code has been formatted (see here)
  • [x] Tests have been added/updated (see here for the integration test guide)
  • [x] Text is internationalised (see here)
  • [x] Docs (specifically docs/Config.md) have been updated if necessary
  • [x] You've read through your own file changes for silly mistakes etc

Edit: Previously I have left the "Text is internationalised" checkbox empty, as I thought that it would also have to include translations, but as I understand currently, that would be "localization" instead, so I ticked it.

isti115 avatar Sep 25 '23 18:09 isti115

@Isti115 I haven't heard back from you after giving feedback, and it's been quite a while. Do you intend to keep working on this?

stefanhaller avatar Jan 21 '24 09:01 stefanhaller

@stefanhaller Yes, I still have it on my (ever growing) todo list. I really hope to find time to fix this up soon, but life has been busy lately. Still, each week I hope that I might just randomly find an hour of free time to work on this. 🙃

isti115 avatar Jan 21 '24 10:01 isti115

@stefanhaller I have finally managed to sit down and hopefully get this on track. 🙂 Can you please take a look at the current version? As far as I can tell the only concern I didn't address is the step size of the threshold adjustment. In my opinion adjusting bigger ranges with one percent granularity is not really an issue, since the key can be held down to repeat the action until the desired value is reached. Of course if you insist on changing it to steps of 5%, I'll implement it that way.

isti115 avatar Feb 23 '24 14:02 isti115

In my opinion adjusting bigger ranges with one percent granularity is not really an issue, since the key can be held down to repeat the action until the desired value is reached.

This solves it for people who use a very fast key repeat rate (I'm one of those), but for people with a slower repeat rate it's still cumbersome. Also, my main concern was about setting it back to 50% when you're done, which is cumbersome with a 1% granularity.

That said, while I do feel that using a 5% granularity could be an improvement for both, I'm not sure it's the best solution. Maybe there's a completely different option that we are both missing; would be interested in @jesseduffield's take on this. So no, I definitely don't insist.

Meanwhile, there are still unit tests failing. You can use go test ./... -short (or make unit-test) to run them locally, or use VS Code's test browser (that's what I usually do).

stefanhaller avatar Feb 24 '24 09:02 stefanhaller

@stefanhaller Sorry for needing so much babysitting with this PR, this is at least partly due to the fact that I was trying to get this done quickly while having very little previous Go experience. 🙃

I really hope that I won't be taking away much of your attention / time now! (The remaining failing checks [missing label and project token] seem to be ones that are not under my control.)

I'd also be interested in reading the opinion / ideas of others regarding the step size discussion!

isti115 avatar Feb 24 '24 19:02 isti115

I wonder how many people would actually use 93% instead of 90% or 95%.

I've never used it before so I'm curious to hear how precise does it have to be for actual users.

Also just a nitpick (that I don't expect to be addressed) - holding down the ( or ) doesn't display the increase/decrease message in real time :(

(it makes sense, I just thought it would go brrrrrrrr)

mark2185 avatar Feb 27 '24 08:02 mark2185

@mark2185 Hmm, interestingly enough it does "go brrr" for me. 😀 demo

isti115 avatar Feb 27 '24 13:02 isti115

Huh, that's odd, it's laggy on my side, tried two different terminal emulators.

In any case, if it works for you, consider the matter solved, then!

mark2185 avatar Feb 27 '24 13:02 mark2185

Sorry to bump, but in order to prevent the PR from going stale I'd like to ask, @jesseduffield, if you have any further input based on the discussion above? I wonder whether the question of step size is significant enough to be blocking the merge.

isti115 avatar Mar 19 '24 19:03 isti115

Steps of 5% sounds good to me

jesseduffield avatar Mar 19 '24 20:03 jesseduffield

image Sorry for disappearing again, I've adjusted the step size to 5%.

isti115 avatar Jun 13 '24 09:06 isti115

Sorry for the bump again, @jesseduffield and @stefanhaller, by no means do I feel entitled to quick responses after my several month long hiatuses, but just in case you didn't get a notification of my previous post I thought I'd also tag you to prevent this PR from getting lost indefinitely. 🙂

isti115 avatar Jul 13 '24 03:07 isti115

@isti115 I've just had a look through the code: This is great work, I'm happy to merge it. You just need to fix that lint issue (looks like a code formatting issue) and rebase onto master. There's a conflict in the test_list.go file but that'll be easy to resolve: just accept both changes and then re-run go generate ./...

jesseduffield avatar Jul 13 '24 03:07 jesseduffield

I've taken the liberty of making those changes myself as I'm itching to get this release out today!

jesseduffield avatar Jul 13 '24 04:07 jesseduffield

Wow, it's really weird how my latest commit had those bunch of misalignments, since I had automatic formatting enabled during development. I guess that they must've been introduced during my the latest rebase, maybe I had --ignore-whitespace turned on. Thanks for fixing things up, I'm looking forward to being able to easily access this feature anywhere, not only on my own builds! :slightly_smiling_face:

isti115 avatar Jul 15 '24 18:07 isti115