Allow toggling patch checkbox by clicking on lines

[simonwiles]

This PR changes the line-select UI in patch mode to allow toggling lines by clicking anywhere on the line. It doesn't address all the desired improvements described in #1421 (and so won't close it), but I've been using this modification for a while and it does make things somewhat easier.

If it passes an initial review I can update the CHANGELOG.md and bump the version number.

[wmertens]

Would it be possible as a quick win to add a button for toggling all? Just an idea, not worth blocking this over.

[wmertens]

Knockout novice but LGTM

[simonwiles]

Would it be possible as a quick win to add a button for toggling all? Just an idea, not worth blocking this over.

Yeah, I like the idea of an "invert selection" button, or something like that. I'm not going to be able to do it right now, but I'll make a note. I might look at addressing actual drag selecting if I can find time too, but I'd rather see this closed off first, because with the best will in the world I don't know if/when I'll have time!

Knockout novice but LGTM

Likewise, but tbh this is more about getting the regex find/replace right :)

Is the advice at https://github.com/FredrikNoren/ungit/blob/master/CONTRIBUTING.md#pull-requests still valid? I notice the release process seems to have changed a bit since this was written. I've updated the changelog under "unreleased" and not touched the version number in package.json -- hope that's okay.

[Hirse]

Since the checkbox is inside the row, this click event on the checkbox is not really needed, right?

[simonwiles]

Well, it's needed in the sense that it's invoked here:

ungit/components/textdiff/textdiff.js

Lines 228 to 231 in 406a4b0

	toggleCheckboxFromRowClick(c, e) {
	if (this.editState() === 'patched')
	e.currentTarget.querySelector('input[type=checkbox]').click();
	}

and simply removing it means the functionality no longer works :)

But yes, I agree it feels like it should be redundant (I originally made these changes as a quick-and-dirty hack for my own use some months ago, but the conversation at #1421 prompted my to PR them).

The alternative, it seems to me, is to bind the "checked" state of the checkbox to the index in TextDiffViewModel.patchLineList and bind togglePatchLine(index) to the click event on the <td/> -- so I've added a commit that does that.

There are two downsides to this: 1) is that, as the comment at

ungit/components/textdiff/textdiff.js

Lines 168 to 170 in 406a4b0

	// ko's binding resolution is not recursive, which means below ko.bind refresh method doesn't work for
	// data bind at getPatchCheckBox that is rendered with "html" binding.
	// which is reason why manually updating the html content and refreshing kobinding to have it render...

mentions, it is necessary to force a re-render -- this makes the behaviour a little awkward in the edge case where the user tries to select text on a patchable line when patch-mode is enabled; and 2) is that the data-bind="checked: ... binding is two-way, so the normal behaviour of the checkbox has to be inhibited otherwise a) the checkbox is changed and the immediately re-changed, and b) togglePatchLine(index) is only called once, and the interface ends up in an inconsistent state. I've addressed that by simply applying point-events: none to the checkbox itself (making the checked binding effectively one-way), but there may be a better way?

All in all, I'm not 100% convinced this is better than before -- what do you think?

[simonwiles]

The numberOfSelectedPatchLines variable was only used to reset the edit state when it evaluated to zero, so I dropped it and replace the check with a test against this.patchLineList().filter(Boolean).length (see 825e120).

[simonwiles]

Important:

When investigating this, I noticed a bug in the way that ungit operates in the present release version (1.5.11). See below for a quick demonstration:

The reason this occurs is that after all the checkboxes are cleared, the edit state is set to none, and they are hidden; when the patch button is clicked again, the array returned by patchLineList() is reinitialized to all true, but the UI is not re-rendered, so the boxes all appear empty. Clicking one inverts its state, and so what gets committed as the patch is the inverse of the UI state. Note too that not moving the mouse for 3 seconds and then moving it again causes the UI to re-render (because of

ungit/public/source/main.js

Lines 115 to 125 in 9374f6c

	(function detectReActivity() {
	var lastMoved = Date.now();
	document.addEventListener('mousemove', function () {
	// If the user didn't move for 3 sec and then moved again, it's likely it's a tab-back
	if (Date.now() - lastMoved > 3000) {
	console.log('Fire change event due to re-activity');
	programEvents.dispatch({ event: 'working-tree-changed' });
	}
	lastMoved = Date.now();
	});
	})();

):

This became much more obvious with the earlier commits in this PR, because the diff (and checkboxes) are re-rendered on click:

My choice to address this was to make the diff re-render on each render (i.e. not checking for a difference in the html string, as this check is inadequate to determine if the state of the checkboxes has changed anyway). This doesn't generate more re-renders or cause any thrashing, and fixes the issue.

[campersau]

Suggested change

	.d2h-code-linenumber {
	.d2h-code-linenumber, .d2h-code-side-linenumber {

[campersau]

Unfortunatley this makes selecting lines a bit hard. Because the selection gets lost after the detectReActivity mouse move delay...

Maybe we will need the checked attribute on the checkbox again to force the HTML to be different in your described scenario?

[simonwiles]

Unfortunatley this makes selecting lines a bit hard. Because the selection gets lost after the detectReActivity mouse move delay...

Yeah, that's an additional problem I hadn't noticed, thanks (sigh).

Maybe we will need the checked attribute on the checkbox again to force the HTML to be different in your described scenario?

This won't work, as demonstrated by the fact that the bug I described in my last post is present in the v1.5.11 release where this attribute is still written. The html == this.htmlSrc check compares the html the last time the render function ran against the newly generated html -- in the scenario above, the editState is set to 'none' when all the boxes are unchecked, and the patchLineList array is emptied (not set to all false, for example), and so the TextDiffViewModel.render method recreates it will all values set to true. The result is the html string contains all checked attributes on all checkboxes, and is identical to the last time the component rendered.

I'll give it some more thought; the whole way the patchLineList is connected with the editState and instantiated in textdiff.js and invalidated in staging.js could stand a rethink, and I'm starting to get far more familiar with the architecture here and also knockout.js than I ever intended.

Fwiw none of these problems, except the bug already in production, exist with my original PR as submitted, and I'll be continuing to use it that way locally, as I have been for months now.

[simonwiles]

Okay, this last commit addresses the bug in production, I think (although it's really a workaround rather than a proper fix).

[wmertens]

I wish that you could click and drag (or tap + hold and drag) to select/unselect lines, and that there was a toggle select button like the toggle files button, but this PR is already a great improvement and I'd rather not wait for those improvements.

@campersau I tried it out, it merged cleanly and LGTM, would you consider merging?

[simonwiles]

I'm just getting around to rebasing all my local changes on top of the most recent release, and I realized this PR is still open. Might there be any interest in revisiting this?