<C-n> flip fix

[jeremybobbin]

Fixes: #1022

Unfortunately I had to change Filerange sel to Filerange range in selections_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.

[rnpnr]

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.

[jeremybobbin]

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.

[rnpnr]

That part seems fine. I'm talking about all of the changes in the commit:
"replace instances of Filerange sel with Filerange range"

[jeremybobbin]

You're probably right. It may be more confusing than before.

[ninewise]

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?

[jeremybobbin]

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?

[mcepl]

@jeremybobbin Is this still alive? Could we get rebase here for the current master, please?

Thank you

[mcepl]

@rnpnr ??? What is going on here?

[rnpnr]

I don't think the issues mentioned above were ever resolved.

[jeremybobbin]

The original issue was resolved.

[mcepl]

@jeremybobbin So, this PR could be closed?