Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Annotations selection range #1139

Open
2 tasks done
NicolaiSoeborg opened this issue Mar 19, 2019 · 10 comments
Open
2 tasks done

Annotations selection range #1139

NicolaiSoeborg opened this issue Mar 19, 2019 · 10 comments

Comments

@NicolaiSoeborg
Copy link

NicolaiSoeborg commented Mar 19, 2019

  • I have searched existing issues and could not find my issue.
  • I have studied the documentation.

A few questions about annotations:

  1. Wouldn't it make most sense if "annotations" in the very first "update"-RPC response was an empty list (instead of a selection of nothing?)

  2. I think the AnnotationRange is wrong when using add_selection_below, e.g.:

Create a file with:

ABCD
EFGH

Now send events: move_right_and_modify_selection, move_right_and_modify_selection, add_selection_below and the annotation range will be:

  • start_line=0, start_col=0, end_line=0, end_col=2
  • start_line=1, start_col=2, end_line=1, end_col=2

But I think that should be (i.e. all of first line + half of second line):

  • start_line=0, start_col=0, end_line=0, end_col=3
  • start_line=1, start_col=0, end_line=1, end_col=1

Relevant RPC output:

> {"method": "client_started", "params": {}}
> {"id": 1, "method": "new_view", "params": {"file_path": "TEST"}}
< {"method":"config_changed","params":{"changes":{"auto_indent":true,"autodetect_whitespace":true,"font_face":"InconsolataGo","font_size":14,"line_ending":"\n","plugin_search_path":[],"save_with_newline":true,"scroll_past_end":false,"surrounding_pairs":[["\"","\""],["'","'"],["{","}"],["[","]"]],"tab_size":4,"translate_tabs_to_spaces":true,"use_tab_stops":true,"word_wrap":false,"wrap_width":0},"view_id":"view-id-1"}}
< {"method":"update","params":{"update":{"annotations":[{"n":1,"payloads":null,"ranges":[[0,0,0,0]],"type":"selection"}],"ops":[{"lines":[{"cursor":[0],"ln":1,"styles":[],"text":"ABCD\n"},{"ln":2,"styles":[],"text":"EFGH\n"},{"ln":3,"styles":[],"text":""}],"n":3,"op":"ins"}],"pristine":true},"view_id":"view-id-1"}}

> {"method": "edit", "params": {"view_id": "view-id-1", "method": "move_right_and_modify_selection", "params": {}}}
< {"method":"update","params":{"update":{"annotations":[{"n":1,"payloads":null,"ranges":[[0,0,0,1]],"type":"selection"}],"ops":[{"lines":[{"cursor":[1],"ln":1,"styles":[0,1,0],"text":"ABCD\n"}],"n":1,"op":"ins"},{"n":1,"op":"skip"},{"ln":2,"n":2,"op":"copy"}],"pristine":true},"view_id":"view-id-1"}}

> {"method": "edit", "params": {"view_id": "view-id-1", "method": "move_right_and_modify_selection", "params": {}}}
< {"method":"update","params":{"update":{"annotations":[{"n":1,"payloads":null,"ranges":[[0,0,0,2]],"type":"selection"}],"ops":[{"lines":[{"cursor":[2],"ln":1,"styles":[0,2,0],"text":"ABCD\n"}],"n":1,"op":"ins"},{"n":1,"op":"skip"},{"ln":2,"n":2,"op":"copy"}],"pristine":true},"view_id":"view-id-1"}}

> {"method": "edit", "params": {"view_id": "view-id-1", "method": "add_selection_below", "params": {}}}
< {"method":"update","params":{"update":{"annotations":[{"n":2,"payloads":null,"ranges":[[0,0,0,2],[1,2,1,2]],"type":"selection"}],"ops":[{"lines":[{"cursor":[2],"ln":1,"styles":[0,2,0],"text":"ABCD\n"},{"cursor":[2],"ln":2,"styles":[],"text":"EFGH\n"}],"n":2,"op":"ins"},{"n":2,"op":"skip"},{"ln":3,"n":1,"op":"copy"}],"pristine":true},"view_id":"view-id-1"}}
@cmyr
Copy link
Member

cmyr commented Mar 23, 2019

This does look suspicious. cc @scholtzan, thoughts?

@scholtzan
Copy link
Member

scholtzan commented Mar 23, 2019

One thing to note here is that carets are also represented as annotations. So if we'd send an empty list of annotations in the first update then we would lose the caret which starts out to be at the beginning of the document (position 0,0,0,0).

The add_selection_below/add_selection_above (ctrlshift/) seems to imitate the same behaviour as in Sublime. Which seems to be adding a new caret to the previous/next line that is in the same "column" as the original caret. The behaviour you describe sounds like doing shift/. I guess the naming is a bit confusing here since add_selection_below basically means adding the next line to the selection and not expanding the selection to the next line.

@NicolaiSoeborg
Copy link
Author

Okay, so I confused add_selection_below with move_down_and_modify_selection.

Would you like me to make a PR adding:

  • add_caret_above => add_selection_above
  • add_caret_below => add_selection_below
    and "deprecating" add_selection_above/below (in the documentation) ?

And followup question: Would it make sense to differentiate between carets and selections? I think it would be more logical with an annotation with type caret

Also, should new frontends ignore the "cursor" property of lines? And only put caret at annotation with start_line = end_line and start_col = end_col ?

@scholtzan
Copy link
Member

One thing to keep in mind is that every change in the frontend protocol will break existing clients. Maybe @cmyr has some thoughts about renaming the methods?

@Peltoche
Copy link
Contributor

And followup question: Would it make sense to differentiate between carets and selections? I think it would be more logical with an annotation with type caret

Hi everyone.
A big +1 for this proposition! IMHO the selection zone and the carets are different things and we can see it.

This following gif is taken from the vim-multi-cursor readme and we can see different colors for the caret and the selection. If the two data are mixed into a single annotation, this kind of thing would became really difficult.

An example

@cmyr
Copy link
Member

cmyr commented Mar 26, 2019

I'm not totally convinced. The abstraction of treating the caret as a length-zero selection is useful in other ways, and separating carets and selections increases overhead in other areas.

The caret is always going to require special care when drawing. The case above is no different; when drawing the text, you will have to check if the line contains a caret and then invert the text color if necessary.

So basically: I think this is a question of differences in display. A 'caret' is just one end of a selection region. A selection region can be empty. 🤷‍♂️

@nangtrongvuon
Copy link
Member

I'm also with @cmyr. I think a simple API change to accommodate just the adding carets case would suffice.

@NicolaiSoeborg
Copy link
Author

When I started developing a tiny frontend for Xi, I did not find it very logical that a caret = selection + which end of selection and that a selection might have length 0.

I would find it much more logical if annotations contains anything that isn't text (e.g. selections, find, cursors, etc)

I'm currently using the following code to "extract" carets and test if a "real" selection has been made:

class View:
   # [...]

    @property
    def carets(self):
        cursors = []
        for line in self.lines:
            if line['valid'] and "cursor" in line:
                for x in line["cursor"]:
                    cursors.append((line["ln"], x))
        return cursors

    @property
    def has_selection(self):
        for annotation in self.annotations:
            if annotation["type"] == "selection":
                for r in annotation["ranges"]:
                    if r[0] != r[2] or r[1] != r[3]:
                        return True
        return False

If we made a distinction between carets and selections, then the above logic would be much more simple, but if this is at the expense of Xi performance / codebase, then clearly that's a bad choice.

(This topic clearly have the potential for bikeshedding, so if anybody with a lot of insight into the Xi code can say "probably not gonna happen", then I think the solution for now must be to update the frontend doc a bit)

@Peltoche
Copy link
Contributor

So basically: I think this is a question of differences in display. A 'caret' is just one end of a selection region.

This could work indeed but in this case the frontends needs to know at with end the cursor is and I don't think this information is available today.

@cmyr
Copy link
Member

cmyr commented Mar 27, 2019

historically we've bundled selections up in the styles system, and we provided information about carets alongside that as a convenience.

With annotations, we have a concept of a 'payload'; it may be the case that we want to include caret-related stuff in the payload of selection annotations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants