Open jeremybobbin opened 1 year ago
Why did you change Filerange sel
to Filerange range
?
It seems less clear now. The Filerange
in those functions wasn't some arbitrary range; it was a range representing the current selections.
PS: 3 newlines snuck in at the end of main.c.
Thanks - I've addressed the newlines.
I had to introduce Selection *sel
in selections_match_next
so that we could run view_selections_flip
on it.
I believe the distinction is useful since a range's beginning must come before its end. With the selection, however, the anchor & cursor can come in any order.
That part seems fine. I'm talking about all of the changes in the commit: "replace instances of Filerange sel with Filerange range"
You're probably right. It may be more confusing than before.
diff --git a/main.c b/main.c
index 858af23..266d70c 100644
--- a/main.c
+++ b/main.c
@@ -1394,25 +1394,37 @@ static const char *selections_match_next(Vis *vis, const char *keys, const Arg *
if (!buf)
return keys;
+ bool flip;
bool match_all = arg->b;
Filerange primary = sel;
+ Selection *new_sel;
for (;;) {
+ flip = view_cursor_get(view) == primary.start;
sel = find_next(txt, sel.end, buf);
if (!text_range_valid(&sel))
break;
- if (selection_new(view, &sel, !match_all) && !match_all)
- goto out;
+ if (new_sel = selection_new(view, &sel, !match_all) {
+ if (flip)
+ view_selections_flip(new_sel);
+ if (!match_all)
+ goto out;
+ }
}
sel = primary;
for (;;) {
+ flip = view_cursor_get(view) == primary.start;
sel = find_prev(txt, sel.start, buf);
if (!text_range_valid(&sel))
break;
- if (selection_new(view, &sel, !match_all) && !match_all)
- break;
+ if (new_sel = selection_new(view, &sel, !match_all) {
+ if (flip)
+ view_selections_flip(new_sel);
+ if (!match_all)
+ break;
+ }
}
out:
I'm considering applying it like this.
It all seems correct to me, but indeed, it might not be helpful. Perhaps ctrl-n should simply change the current selection to be not flipped before doing anything?
There's a coherence issue with selection_new
, in that information is lost when going from a selection to a range.
selection_new
takes a range, and given selection_new
is only called in selections_match_next
, either selection_new
needs a bool flip
argument, or, as you suggested:
Perhaps ctrl-n should simply change the current selection to be not flipped before doing anything?
@jeremybobbin Is this still alive? Could we get rebase here for the current master, please?
Thank you
Fixes: https://github.com/martanne/vis/issues/1022
Unfortunately I had to change
Filerange sel
toFilerange range
inselections_match_next
for clarity. This seemed to be a theme used in a number of other functions.Also, though the behavior in this patch seems more correct, I struggle to imagine a case wherein this fix is useful.