[WIP] Implement abstraction for visual selection #269

cherti · 2017-10-09T21:18:39Z

This implements a class abstraction to represent a selection between two positions in the buffer. Most notably this can be used to represent a selection in visual block mode. In addition to the representation, said class allows for manipulation of the represented selection.

Furthermore, a property visual_selection was added to Buffer to allow easy access to a selection made in visual (block) mode and manipulation thereof e.g. via nvim.current.buffer.visual_selection.

justinmk · 2017-10-09T21:38:17Z

This is needed because Range doesn't support partial lines, right?

Instead of Selection, how about Region:

class Selection(object):
    def __init__(self, buffer, startpos, endpos):

then it can take any pair of (line, column) positions, instead of just '< and '>. Probably Range could be deprecated then, too.

We could still have the Buffer.visual_selection() method as sugar:

    @property
    def visual_selection(self):
        """Get the current visual selection"""
        startmark = self.mark('<')
        endmark   = self.mark('>')
        return Region(self, startmark, endmark)

This implements a class abstraction to represent a selection between two positions in the buffer. Most notably this can be used to represent a selection in visual block mode. In addition to the representation, said class allows for manipulation of the represented selection.

cherti · 2017-10-10T12:07:40Z

This is needed because Range doesn't support partial lines, right?

Aye, exactly this.

@justinmk
So simply rename Selection to Region you mean? Would make sense to further abstract from the originally motivating visual selection.

Regarding the deprecation of Range/merge of Range into Selection/Region: Should this be done immediately in this PR or as a follow-up one?

A reason against having those two objects merged would be that Range would implement writing through onto the buffer while not caring about partials and Selection/Region takes care of the partial-lines-implementation, separating those logics. But otherwise, merging both into one object would probably reduce the overall footprint.

Furthermore, in case we go for direct merge, I think I might have found a Bug in Range that I'd like to investigate and possibly fix before we would merge the two classes, therefore I would stall this PR for the moment then to keep Bugfixes separate from Feature implementations in that case if that's ok.

cherti · 2017-10-10T12:25:19Z

Probably Range could be deprecated then, too.

Actually, another reason to not deprecate Range would be that it would make perfect sense for a Range of three lines to replace that by e.g. five lines, the Range in the original buffer would just expand a little.

This however is not well defined for a Selection/Region: inserting more lines than the Region would duplicate some part of the lines that are not part of the selection, that's why I implemented the explicit assert(…) in between because I noticed that behavior in my tests, duplicating certain lines completely.

r = Range[1:3]
r[1:3] = ['foo'] * 10  # this would be fine as it would just insert some lines

s = Selection((1, 5), (3, 8))
s[1:3] = ['bar'] * 10 # this would not make sense or would duplicate parts of the buffer that are not part of the selection

Capturing this in one object would lead to rather nested code and the separation into a range-object adn a region-object that is based on a range-object would be a much cleaner solution and range would be able to keep its larger set of functionality.

Therefore I'd argue merging Range and Region/Selection is not a good idea.
Your take on this, @justinmk ?

justinmk · 2017-10-10T13:33:45Z

"deprecate Range" doesn't need to be discussed right now, it was only a minor future suggestion.

I think I might have found a Bug in Range that I'd like to investigate

Bugfix is welcome.

cherti · 2017-10-10T13:48:12Z

Ok, Code ready for merge now. There are some complaints of flake8 left causing the CI to fail, but they do not affect those parts of the code that this PR touches, so that shouldn't be an obstacle for the merge of this PR.

Re: Bugfix in Range: not a blocker for this PR, so this one can be merged, I'll track down the Bug I suspect to be in Range in the next couple of days and open a separate PR for that if I actually can substantiate my suspicion.

cherti · 2017-10-10T16:00:53Z

@bfredl
You might also want to take a look as we briefly discussed this in IRC.

bfredl · 2017-10-10T16:24:14Z

Later on we might want abstractions directly in the API for regions on the character-level, especially with extmarks that care about adjusting character position. At first API functions that gets or sets the text between two mark objects (an abstraction that either is a named mark or concrete position, extmarks PR neovim/neovim#5031 does steps towards this), also nvim_put neovim/neovim#6819 could take a mark object and not just put at cursor position.

cherti · 2017-10-10T19:37:32Z

@bfredl @justinmk
Do we do something about the failing flake8-check in this PR?

bfredl · 2017-10-10T19:50:13Z

Maybe just add it to the ignore list in setup.cfg

cherti · 2017-10-11T11:57:34Z

ok, then I'd say this one is ready to be merged.

bfredl · 2017-10-12T18:16:20Z

neovim/api/buffer.py

+class Region(object):
+    def __init__(self, buffer, startmark, endmark):
+        self._range = buffer.range(startmark[0], endmark[0] + 1)
+        self.slice = slice(startmark[1], endmark[1] + 1)


this is not a slice as startmark[1] and endmark[1] are indicies into different sequences (lines in this case).

hm, this implementation is actually intended for visual block selection. This is indeed not sufficient for normal visual selection, when thinking about it.
How can a simple visual selection and a visual block selection be differentiated? Start- and end-marks are not sufficient for that, obviously, as it starts both of them.

cherti · 2017-10-14T15:43:21Z

Thinking about this, I think I'll introduce a BlockRegion in addition to a normal Region, distinguishing e.g. standard visual selection from visual block selection.
As this would be two types of regions that behave significantly different in what area is affected by them, I think two separate Objects for those would be the most clean approach.

If possible I try to let both inherit from a GeneralRegion object to minimize code duplication, but testing will tell if that is reasonable.

…tion

justinmk · 2017-10-16T20:30:38Z

BlockRegion in addition to a normal Region, distinguishing e.g. standard visual selection from visual block selection.
As this would be two types of regions that behave significantly different in what area is affected by them, I think two separate Objects for those would be the most clean approach.

Let's not go crazy with OOP :) They aren't very different. The interface is the same:

They both return text.
They both are defined by a pair of (row,col) tuples.
A "region type" flag (charwise, linewise, block) can switch the behavior.

cherti · 2017-10-16T21:06:40Z

They both are defined by a pair of (row,col) tuples.

Actually not: standard visual mode has full lines between the first and the last lines and partial lines in the first and last row. That makes handling of such a selection actually quite different for my taste as both differ in a number of nasty details, requiring very different flavors of __setitem__ and __getitem__.

Merging both into one Class and having a switch would be an option, though. I'm not yet sure if that's a nice solution, because __getitem__ and __setitem__ work very differently and also len() can be well defined for a normal selection (as the number of characters) whereas this is very much less obvious for a block-region.

This then make up more or less the entirety of that class (as __init__ is also different (at least how I implement it) for both due to the slightly different requirements outlined above), therefore it might be advisable to have to classes for that to not also make the code harder to read by such an inline-switch-case.

(I dropped the "let's inherit from a general object"-idea, though, as that does indeed make very little sense in this context.)

At least this is my reasoning to this point as far as I have thought the thing out so far. I'm happy to be convinced otherwise, @justinmk :) Wouldn't be the first time that I don't see a more beautiful solution right away and go for a very much inferior one.^^

(and don't mind the failing CI, btw., I just pushed the current state for brief review, I will fix and test ASAP. However, feel free to comment on the current code right now (at least on a conceptual level), if you want, that's why I already pushed even if it's not yet finished).

justinmk · 2017-10-16T21:39:49Z

They both are defined by a pair of (row,col) tuples.
Actually not: standard visual mode has full lines between

It's still a pair of (row,col).

The behavior is different, but the interface is the same. It isn't a different "class", it's just a region.

Merging both into one Class and having a switch would be an option, though. I'm not yet sure if that's a nice solution, because getitem and setitem work very differently and also len() can be well defined for a normal selection (as the number of characters) whereas this is very much less obvious for a block-region.

Could be functions like get_text_linewise() and get_text_charwise(), whatever. The point is users shouldn't need to fumble around with different object types just to work with the same region in different ways.

cherti · 2017-10-16T21:45:45Z

Good point indeed. I'll reconsider the implementation and see how to merge both objects.

cherti · 2017-12-11T23:33:18Z

Based on the suggestions of @justinmk I redesigned the API for the Region object and it is indeed much more general now.

I still like to rebase onto master, but feel free to already review the code and give your opinion and suggestions!

justinmk · 2017-12-13T12:53:47Z

test/test_buffer.py

+    # writing
+    vim.current.buffer[:] = reference_buffer
+    r = Region(vim.current.buffer, 2, 4, partials=[(1, 3), (1, 3), (0, 3)])
+    r[:] = ['bar']*3


I would strongly prefer explicit functions rather than cute list operations. I don't know if it's pythonic. This is an RPC API, not pandas or whatever. Though I also find pandas far too magical.

@bfredl uses python more than I , would like to hear his opinion.

To be honest until extmarks (and api or lua functions to manipulate charwise ranges in an efficient and atomic way) are available this is a pretty weak abstraction, "cute" list operations (no matter what one thinks about them) are more or less what the class offers.

@justinmk
Regarding the list-operatons I tried to keep as close as possible to the Buffer- and Range-classes, that's where the idea for this kind of implementation came from.
Regarding the API: which API is this wrapping? The code footprint would actually much decrease if there is an API I overlooked giving me the current selection that I can use.

@justinmk @bfredl
So we proceed with the list operators like Buffer and Range use it for the Region-abstraction (until there is something better)?

Ok, fair enough

justinmk · 2017-12-13T12:56:54Z

neovim/api/buffer.py

+
+
+class Region(object):
+    def __init__(self, buffer, startrow, endrow, partials=(None, None)):


In the doc should mention that it's 1-indexed.

justinmk · 2023-07-14T20:24:36Z

See neovim/neovim#13896 (comment) and neovim/neovim#21115 (comment) .

The Lua function vim.region() should be used for this. Anything missing from vim.region() should be fixed/enhanced in vim.region() rather than adding client-specific features.

cherti changed the title ~~Implement abstraction for visual selection~~ [WIP] Implement abstraction for visual selection Oct 9, 2017

justinmk mentioned this pull request Oct 9, 2017

Current visual block #268

Closed

cherti added 2 commits October 10, 2017 13:52

Add property 'visual_selection' to Buffer

e73f6f5

cherti force-pushed the visual-selection branch from 5fdc50a to e73f6f5 Compare October 10, 2017 11:52

cherti added 3 commits October 10, 2017 15:36

Clean code according to flake8

27add3f

Rename Selection to Region to be more general

161a3f6

Correct missing period at end of docstring

b179f11

cherti changed the title ~~[WIP] Implement abstraction for visual selection~~ Implement abstraction for visual selection Oct 10, 2017

cherti mentioned this pull request Oct 11, 2017

Ignore D401-type-complaints of flake8 #271

Merged

cherti closed this Oct 11, 2017

cherti reopened this Oct 11, 2017

bfredl reviewed Oct 12, 2017

View reviewed changes

cherti changed the title ~~Implement abstraction for visual selection~~ [WIP] Implement abstraction for visual selection Oct 14, 2017

cherti added 3 commits October 16, 2017 22:07

Rename Region to BlockRegion

a4e67e0

Fix incorrect computation of relative index

86158c1

Implement NormalRegion as a representation of a standard visual selec…

cdf980d

…tion

cherti added 7 commits December 11, 2017 01:04

implement region type

c720a73

add tests for region

9d79313

merge master during dev

6109981

add tests to for __setitem__

3b6f0ea

implement Iterator-API properly

45a4373

fix assemble_line for full-line-regions

968d15f

implement visual selection extraction as buffer-function

2e8c1a8

remove double extraction of marks

06baaa2

justinmk reviewed Dec 13, 2017

View reviewed changes

justinmk mentioned this pull request Mar 13, 2019

TextYankPost: add v:event["inclusive"] neovim/neovim#9717

Merged

justinmk closed this Jul 14, 2023



		class Region(object):
		def __init__(self, buffer, startrow, endrow, partials=(None, None)):

[WIP] Implement abstraction for visual selection #269

[WIP] Implement abstraction for visual selection #269

Uh oh!

Conversation

cherti commented Oct 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmk commented Oct 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cherti commented Oct 10, 2017

Uh oh!

cherti commented Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmk commented Oct 10, 2017

Uh oh!

cherti commented Oct 10, 2017

Uh oh!

cherti commented Oct 10, 2017

Uh oh!

bfredl commented Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cherti commented Oct 10, 2017

Uh oh!

bfredl commented Oct 10, 2017

Uh oh!

cherti commented Oct 11, 2017

Uh oh!

bfredl Oct 12, 2017

Choose a reason for hiding this comment

Uh oh!

cherti Oct 13, 2017

Choose a reason for hiding this comment

Uh oh!

cherti commented Oct 14, 2017

Uh oh!

justinmk commented Oct 16, 2017

Uh oh!

cherti commented Oct 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmk commented Oct 16, 2017

Uh oh!

cherti commented Oct 16, 2017

Uh oh!

cherti commented Dec 11, 2017

Uh oh!

justinmk Dec 13, 2017

Choose a reason for hiding this comment

Uh oh!

bfredl Dec 13, 2017

Choose a reason for hiding this comment

Uh oh!

cherti Dec 15, 2017

Choose a reason for hiding this comment

Uh oh!

justinmk Dec 15, 2017

Choose a reason for hiding this comment

Uh oh!

justinmk Dec 13, 2017

Choose a reason for hiding this comment

Uh oh!

justinmk commented Jul 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

cherti commented Oct 9, 2017 •

edited

Loading

justinmk commented Oct 9, 2017 •

edited

Loading

cherti commented Oct 10, 2017 •

edited

Loading

bfredl commented Oct 10, 2017 •

edited

Loading

cherti commented Oct 16, 2017 •

edited

Loading

justinmk commented Jul 14, 2023 •

edited

Loading