Source date handler improvement

[worksofliam]

Changes

• Source date changes are now calculated by diff. This improves change accuracy a whole bunch, and has better support for undo-redo.
• Ability to colorise lines based on search filter
• Hover messages on source date gutter

Closes #819 and #279.

To test

• Enable source dates
• Enable source date gutter
• Apply search date filter

Checklist

• have tested my change
• updated relevant documentation
• Remove any/all console.logs I added
• eslint is not complaining
• have added myself to the contributors' list in CONTRIBUTING.md
• for feature PRs: PR only includes one feature enhancement.

[jkyeung]

Well, I did some rudimentary testing. What little I tried did seem better than the previous scheme, but there is still a very simple way in which it behaves weirdly:

If I insert a line somewhere in the middle of the member, the data (SRCDTA) gets shifted down as one would expect, but the dates (SRCDAT) stay where they are, and a zero gets put at the very end. See below. (Please excuse the use of a slightly proportional font in these screenshots.)

Before edit:

After adding 1 line, things get wonky:

Then somehow after adding another line, things are the way they should be:

Obviously, this is something that would need to be resolved before using VS Code in earnest in a source-date-sensitive shop. I have to stress that I've done very little testing. This is just something that popped up right away. Further testing could well uncover other issues.

[worksofliam]

@jkyeung A new build is available which should solve this issue.

Check it out and let me know what you find.

[jkyeung]

Well, the good news is that I haven't been able reproduce the problem using the latest build (which is the first one with 1.5.10 listed as the version).

The bad news is I haven't been able to reproduce the problem using the previous build either. Why this is bad is that I therefore cannot really establish that the new build fixed anything. And not being able to reproduce a behavior... certainly doesn't inspire confidence.

So we're back to where we were yesterday, which is: I doubt this code is bug-free, but I also doubt that I can reliably find any more buggy behavior in a short time, so probably the best thing to do is just release it, and if there are any remaining bugs, at least there will be a lot more testers.

[worksofliam]

@jkyeung I actually found the issue again and think I solved it this time. Try this last build and if you're still happy, then we will release.

[jkyeung]

Yeah, I stumbled on a way to reproduce the thing I just said I couldn't reproduce, and you probably stumbled on it too.

Just tried the latest build, and this time I can confirm: Couldn't reproduce the buggy behavior in the latest build, but reinstalling the previous build, I could reproduce it.

So, this does make me feel better, and I'm excited for its release!

[jkyeung]

This is truly just a comment, and not a bug report.

It's interesting. I've been using the latest and greatest build, and I observed more weirdness when cutting a bunch of lines and pasting them a few lines down from where they had been. Here's before the cut:

And here's after the paste:

What I expected was that the 5 lines I pasted would get today's date, and everything else would stay the same. From my perspective, I moved the 5 lines marked by the blue box and didn't touch the 3 lines marked by the red box.

I was dismayed at first, but then it hit me: As far as the diff algorithm is concerned, the net result of moving the blue box lines down is indistinguishable from moving the red box lines up! In this case, it chose to update the lines producing the smaller diff.

In this light, I went back and looked at a previous "bug" report and saw the same thing: From my perspective, I moved line A below lines B and C; but I could just as well have moved lines B and C above line A.

So, ultimately the exhibited behavior may be "correct" in some sense, but it feels kind of spooky and out of my control. And this precisely illustrates my initial doubts about a diff-based approach, rather than tracking actual user actions.

When I asked "What will be diffed against what?" I guess what I was after (now that I can see it in action) is: Will you be able to treat a cut followed by a paste as two separate actions? Because it seems like the general diff approach could still produce intuitive behavior if only we could force a snapshot to be taken after a cut, and have the result of the paste diffed against that.

[jkyeung]

OK, I've been trying to just go about my business and adjust my expectations regarding "equivalent edits". But in a particularly intricate sequence of individual user actions (delete a line, delete another nearby line, copy a different line and paste it a few lines down, copy another line and paste it on the line after that), the date behavior was just getting weirder and weirder until I couldn't reason about it anymore, even armed with the insight I expressed in my previous comment.

I can produce some screenshots if you really need them, but I am getting the distinct impression that what's happening is the differ is comparing the current buffer contents against the previous saved buffer contents. So some old dates from deleted lines can be resurrected if you happen to create a new line with the same content in the right place. From a user's perspective, it is utterly bizarre.

You did say, from the very early stages of tossing around the idea of using diffs, that you could compare the current buffer to the last saved buffer, and I kind of challenged you on that. I'm not sure everything I said at that time really sunk in for you, perhaps because there were multiple conversations going on at the same time. Besides completely unrelated stuff, even the conversation about source dates essentially had two concurrent threads going (one focused on the change-event scheme, the other on the diff scheme).

Here is an excerpt from Ryver, starting on "Tue Aug 16 at 2:20pm" (I don't know if everyone sees their own localized time; that's just what was on my screen):

---

John Y. Tue Aug 16 at 2:20pm
Hmm... when a change event comes, is there a way to know anything about the state of the document before the change? For example, can you tell whether ENTER was pressed while the cursor was in the middle of a line?

Liam Barry
I don't believe so.

John Y.
Hmm... not knowing the pre-change state could make it fundamentally impossible to ensure sensible date behavior.
People who, for whatever reason, need the dates to be accurate would need to really contort their editing style, to the point where I'm not sure it would be an advantage over just using SEU, frankly.

Liam Barry
I can confirm, the document in the event is the document post edit
The other way we could do it is to do a pure diff of when the file is opened/last saved to the current dirty one. But I am worried about performance if the file is huge.

John Y.
I agree, a whole-buffer diff sounds unreasonably expensive. And you need the buffer immediately before the change, which could be very different from the buffer at the last clean point. [emphasis added here]

Liam Barry
I will think about it over some beers tonight. That usually helps. Source dates is an incredibly tricky one.. it has been through many iterations before what it is now

John Y.
Conceptually, I think you would need the change event object to not just include the changed range coordinates, but the changed range contents.
I don't know how much you can monkey-patch the change object.

Liam Barry
You cannot, really

John Y.
So, I think the best you can do with the existing scheme is to be more eager about changing dates (for example, pressing Enter on pre-change line N would cause post-change N and N+1 to have today's date, even if you pressed Enter at the end of the original line N, so intuitively speaking, it should keep its old date).

John Y.
Hmm... well, without monkey-patching, I think you might be able to always keep your own copy of the pre-change buffer (by saving the post-change state after each change, assuming you have some kind of static storage, to be used as the pre-change state for the next change). Then you could use that to get the content of the pre-change range. I think saving the whole buffer to memory might be quicker than a proper diff, but still could be slow for big documents and potentially susceptible to actions in VS Code that change the buffer without firing a change event.

Liam Barry
I am going to give this package a whirl to track changes. It won't be until tomorrow but it looks promising. https://github.com/micnil/vscode-diff

John Y.
If it looks promising to you, then I'm eager to find out what you manage to do with it. I am personally a little leery of relying on any diff algorithm, no matter how fast or how elaborate, because I haven't seen one which gets things 100% correct, 100% of the time. Classic example: you've inserted an if..endif block, but the differ thinks you inserted the immediately preceding endif but not the one you actually inserted.
I think a differ might be good for ignoring times you've inserted a character and then immediately deleted it, so there's no net change, but SEU would change the date anyway because it's been touched.

Liam Barry
I think the diff is likely the safest way to get the changes in a document. The way we have now works, but it's prone to becoming out of sync with changes.

---

So, as it stands, this PR is significantly better than what it replaces. And for relatively simple maintenance (which is typical for RPG codebases at most places, I'll bet), it works well. But for intensive editing... I don't know. My comment about contorting one's editing style had been about the change event, but it also applies to the current diff scheme, since neither captures enough information about the state of the buffer immediately before any given change. For example, what I've been doing while testing, and what I'll probably have to continue to do to be confident I'm not making the dates wonky, is to make a copy of the member I really want to work on, work on the copy in VS Code, save it, and then replicate the changes in SEU on the real member, until both versions match (aside from the dates). Perhaps if I can get into the habit of saving after every change in VS Code, then I can skip the SEU step. Or maybe use VS Code's autosave?

[worksofliam]

@jkyeung Thanks for the feedback here. It goes a long way. I am starting to get more worried about merging this PR in, and instead should consider keeping the existing method but just adding minor improvements to that instead. At least with the existing method, it's much easier to track changes (since we track every change and don't use a diff). I will have to look into if I can just make the original stuff better.

[worksofliam]

I have decided to re-open this as I am more confident after finding a new way to test that the new diff based method is ok to you.

When you have a member open with dates, open the command palette with F1 and search 'compare active file with saved'. This will show you a diff, but will allow you to see that the lines that have changed have the correct dates based on the diff.

[worksofliam]

@jkyeung Take a look at my above comment and give that a go for me. Let me know what you think.

[github-actions]

👋 A new build is available for this PR based on 4581933.

• Download here.
• Read more about how to test

[Wright4i]

@worksofliam in reference to your comment

Decided to test this out. It's improved but there's still some weirdness.

Simple Test:

• Write a new if block (if sStatus <> '')
• Move an old if block below new if block (if $cust = *blanks)

Current v1.5.13:

Highlighting is good. Date codes are just wrong. Oddly enough if I exit this compare, delete my code, and add it again the dates appear.

PR 824:

Dates are better. Highlighting is fine, it looks like it is a little confused on an existing endif; but it's not wrong there was an endif; added. The only weirdness I see is the fact the highlighted dates don't match up with the highlighting from the diff.

Ideally I'd expect to see something like this where all the date and diff highlighting align.

Photoshopped:

[worksofliam]

@Wright4i Good find and great review. I will have to see about finding that issue later this week. This source date branch is tricky to get right, but when it's right it will be great.

[jkyeung]

So, this seems understandably stalled, but it would be great if there were some way to opt in to this source date handling, even though it's experimental. Installing from VSIX isn't hard, but the last one was for 1.5.10, and as of this writing, the released version is 1.5.23.

Obviously the most user-friendly way would be some kind of setting/configuration in the released extension that enables this. I don't know how difficult it would be to have two selectable source date handlers in the same extension.

Some other alternatives that I can think of, with varying levels of annoyingness for either the extension maintainer or users or both:

• Provide a downloadable VSIX with the alternate source date handling for each new release.
• Post detailed instructions that users can follow to build these for themselves.
• Publish an alternative extension with a slightly different name, that perhaps would not get updates as frequently as the main extension, but that users could leave installed with autoupdate on and still not lose the alternative source date handling when a new version comes along.

What might be the least annoying of all, for both maintainer and users, is to just go ahead and merge this. Right now, as I see it, the released source date handling is not producing complaintsbug reports from users because it's so unusable that no one has it turned on. Merging this PR (and announcing the change "loudly" enough, to the right channels) might entice more people to try it. And as mentioned above, for small edits, or for people who save frequently, it may well work reliably enough that people don't see the weirdness too often. And those who do see it might provide feedback to help refine it.

Now, I should say that it's possible that my editing style and way of using VS Code is incredibly different than most other people, and perhaps there are plenty of Code for i users that have source dates turned on and are reasonably happy with it. If that's the case, then I guess merging this may be risky. But in its current state, I cannot really use the release version of Code for i except as an occasional read-only source browser.

[worksofliam]

@jkyeung Sorry that it has stalled.

I will do two things:

1. Update this branch to the latest version and let a new VSIX build. That will show here automatically when I am done.
2. Actually allow you to switch to this new version in the release version. That will be good for testing.

Liam

[github-actions]

👋 A new build is available for this PR based on 7bda55c.

• Download here.
• Read more about how to test

[jkyeung]

If I had known it was not a big deal to have both source date schemes in production, I would have suggested it way earlier! 😀:tada: