Move bookmarks on changes to line numbers #147

Androbin · 2018-02-02T15:41:15Z

dschuyler

Thanks!

I'd like to look this over more and give better input. Here's a few comments to get started.

dschuyler · 2018-02-02T18:14:53Z

app/actions.py

+      (boolean) Whether any bookmarks were removed.
+    """
+    if upperRow > lowerRow:
+      return


Even though None will equate to False, let's throw a False on this return.

dschuyler · 2018-02-02T18:16:10Z

app/actions.py

+
+    Args:
+      upperRow: The upper line limit.
+      lowerRow: The lower line limit.


Thanks for writing doc strings. I think it may help someone in the future to know if (for each of) these, whether they are inclusive or exclusive of the row(s). (e.g. just putting ", inclusive" or ", exclusive" at the end of each line is enough).

Good point. I have to admit that I played with both options.

dschuyler · 2018-02-02T18:19:59Z

app/actions.py

+    Returns:
+      None.
+    """
+    for index in range(len(self.bookmarks)):


If we think there may be a lot of bookmarks then a bisect may make sense here (if we think there will be a handful of bookmarks at most then it likely doesn't matter much). What do you think about trying a bisect to find the bookmarks affected.

I gave that some thought as well and just need a moment to figure out how to put things together.

aaxu · 2018-02-02T20:27:08Z

Note that I am working on refactoring on Bookmarks so that it will end up as a separate class. However, thus far, we have been using a tuple. In python, when comparing tuples, I think it compares the first element and if those are equal, it compares the second element, and so on. Using this information, you can use bisect_left/bisect_right to get to where you want.

Are you essentially trying to get the list of bookmarks on the screen? If so, you can take a look at getVisibleBookmarks in window.py to see how it works.

Androbin · 2018-02-02T20:37:07Z

My idea was to mock the __get__ methods etc. and running bisect_left on that.
This functions needs to shift all bookmarks below the lines that changed.
Maybe we can somehow defer some of that using getVisibleBookmarks but could be fragile.

aaxu · 2018-02-02T20:41:16Z

In order to do that, could you use bisect to find the first bookmark you need to change, (should be the first one after the cursor) and iterate the bookmark list starting from there?

Androbin · 2018-02-02T20:43:58Z

Yeah, I think that should be doable.
And when bookmarks have their own class, we can perform only the changes currently visible on the screen and store a list of the pending moves.

aaxu · 2018-02-02T20:45:49Z

Ohh nice idea!

Androbin · 2018-02-02T21:00:09Z

Think so, too. If everything goes well, we can even upgrade to a r-tree 😆 which we can use to efficiently keep track of which line intervals are affected by which changes.

aaxu · 2018-02-02T21:15:07Z

If you know the lower limit, wouldn't you be able to use bisect as well and find the last bookmark closest to that limit?

Androbin · 2018-02-02T21:42:43Z

Okay, I admit it's a bit hacky, but it works just fine.
It's using a anonymous object providing:

__getitem__ = lambda x, index: self.bookmarks[index][0][1]
__len__ = lambda x: len(self.bookmarks)

Thus, finding the first bookmark whose lower limit is >= upper

Androbin · 2018-02-03T17:40:46Z

Performance should be fine, deferring using a r-tree or similar should wait on the refactoring by @aaxu.
The behavior is as expected, but there are some edge cases where we might wanna change it.
Any further thoughts @dschuyler?

aaxu · 2018-02-05T17:10:08Z

Maybe this isn't so much of a bug as a design decision, but if you are inside a bookmark and you create new lines, those new lines will also be part of the bookmark. This can lead to a very long bookmark, and may cause the user to have to delete/recreate it again.

Androbin · 2018-02-05T17:12:15Z

Yeah, I thought this was the point of multi-line bookmarks.
So, what is the desired behavior? Splitting the bookmark into two?

aaxu · 2018-02-05T17:23:08Z

I think this makes sense if we are modifying a 2+line bookmark, but I thought it would be better in the case of having a single line bookmark to maybe keep it as a single bookmark (maybe keep it it at the beginning in the case where the line breaks). I don't use bookmarks, so I can't really say but I personally don't like a bookmark indicator of large size, though I will defer the decision to @dschuyler since he may have a stronger opinion.

aaxu · 2018-02-05T17:25:21Z

Also, upon some more testing, I found that if you create a bookmark and then 'move' a bookmark by deleting a line before it, jumping to next bookmark (F2) will go to the old position of the bookmark, and jumping to previous bookmark (SHIFT_F2) will not work anymore.

Androbin · 2018-02-05T17:28:26Z

Hmm, you have a point regarding single line bookmarks.
Maybe we could differentiate between "newline" at the beginning of the first line, at the end of the last line (could be same as first line) and somewhere in between?
But I am curious, what would you do as a user if you wanted to extend it?

Androbin · 2018-02-05T17:29:25Z

jumping to next bookmark (F2) will go to the old position of the bookmark

Really? Are these values held elsewhere?

aaxu · 2018-02-05T17:33:21Z

Yes I was thinking of that as well. Something along the following lines for single-line bookmarks:

If newline at beginning of bookmark, shift entire bookmark down
if newline at end of a bookmark, do nothing

Also, I'm not sure what I would do as a user if I wanted to extend it. Extending it honestly doesn't have any functionality other than a visual marker, since jumping to a bookmark will place the cursor at the beginning of the bookmark. I think it may have something to do with where the bookmark would be placed as well on the screen once we jump to it.

If we are extending multiline bookmarks, we will also need to handle how the selection mode saved also changes (maybe just change it to selectionNone for simplicity?)

aaxu · 2018-02-05T17:39:52Z

I think the bug is caused by bookmarkData not being changed. Even though you are changing the bookmark position, the bookmarkData still contains old positions. These positions are used to move the cursor back to its original position (from when the bookmark was created), so they should be changed too.

Androbin · 2018-02-05T17:48:40Z

Also, I'm not sure what I would do as a user if I wanted to extend it.

How about ctrl+e -> bm++ or something?

Extending it honestly doesn't have any functionality other than a visual marker

Extending single-line to multi-line or having multi-line at all?
I would certainly miss multi-line bookmark markers.

Androbin · 2018-02-05T19:18:23Z

Just realized: don't we need to update these values on changes to individual lines as well?

aaxu · 2018-02-05T19:26:35Z

Extending bookmarks in general seems to only serve as a visual indicator. I don't see it as having any other functionality that's different from if we didn't extend the bookmark. For example, if you had two bookmarks of different length, jumping to either of them will position them such that the beginning of the bookmark is near the top of the screen. The only difference is if a bookmark is longer than the height of the screen, the beginning of the bookmark will then be situated at the top of the screen when we jump to it.

aaxu · 2018-02-05T19:28:40Z

And yes, we would need to update the data of the bookmarks if we 'move' them regardless of whether they are single-line or multilined.

Androbin · 2018-02-05T19:36:04Z

My last comment was meant differently.
I meant changes in the content of a line:
"dolor # sit amet" -> "lorem #ipsum dolor sit amet"
"dolor # sit amet" -> "lorem ipsum dolor # sit amet"
# stands for either of the positional markers.

aaxu · 2018-02-05T19:49:04Z

Oh yes, you're right. We would need to discuss how to handle these cases. I did consider using positional markers before, but then we'd have to adjust bookmarks for every insertion/deletion. That could end up giving very high overhead, so it may be better to take a more simplistic approach.. This question also extends further to what we should do if a bookmark corresponds to a block of selected text and how we would adjust those values as well.

I would like @dschuyler 's opinion on this since I have no experience using bookmarks so I don't completely understand whether marking the general position is more important, or keeping selection/position of the cursor to where it was before. I would imagine, however, that keeping track of a selection would be useful as a reminder if you want to revisit a bookmark, but easily forget why you bookmarked that spot.

Androbin · 2018-02-05T21:09:05Z

It should help making these values relative the the bookmark position, if possible.

dschuyler · 2018-02-07T18:58:54Z

Thanks for working through this.

Is it time to step back and consider what we want from bookmarks?

I imagine that users may have different needs for bookmarks

to mark a spot to come back to
- might just be one point (not a range)
- might just be one of them
- not much need to store the one bookmark
- can be updated (when editing), but probably unnecessary
to jump between two (or three) locations temporarily
- this is what I use them for personally
- not much need to store them
- akin to putting one's fingers in different sections of a book to flip back and forth
- can be updated (when editing), but probably unnecessary
to make a note for (personal) future reference (like a future editing session)
- these would need to be stored
- akin to bookmarks in a web browser (not URLs per se, but bookmarks)
- can be updated (when editing) and maybe should be
to share (with another person) a reference to a location
- these could be stored (but not critical)
- akin to URLs
- may help to be ascii text (rather than binary data)
- impractical to update
to link locations between documents
- akin to a web page link
- the storage would be in the linking documents
- impractical to update

I think I initially implemented # 2 (per my needs at the time). It sounds like we're heading to # 3 (which I'm okay with).

I'm flexible on whether a bookmark should select a range.

Maybe bookmarks should refer to a 'thing' (which I can see working with # 4 or # 5), like a

word
line
point
class
function
paragraph
comment
relative range
regex
file

i.e. if the bookmark is something like a URL, then part of the encoding could be the type of thing referred to. So if the bookmark was for the class FooBar in the file my_stuff.py, then maybe we could open file my_stuff.py and located the class definition for FooBar and go there (regardless of which line it is on).

Androbin · 2018-02-07T21:04:54Z

Making bookmarks refer to "things" would not allow them to be domain agnostic.
Obviously, this can lead to issues if the file content gets modified with another editor.
But for the time our editor is used for editing, we can have bookmarks "sticking" to text.

Androbin · 2018-02-07T21:04:57Z

Maybe we should consider differentiating between "point" bookmarks (caret positions) and "range" bookmarks (line intervals).
This should be known on creation and would sort out edge cases where we would otherwise not know how things should behave.
They should mostly correspond to cases 1, 2, 5 and 3, 4 respectively.

aaxu · 2018-02-08T00:01:54Z

I was still under the impression that bookmarks was for #2 as well. It's just that sometimes I tend to jump between multiple sections and it would be something like a 'temporary note' (not saved for future sessions).

I would maybe use only single point bookmarks rather than a highlighted range as a bookmark. I'm not sure on how often used this range bookmark would be used, but I would fine to discard it altogether and stick with single line bookmarks to reduce the complexity of this feature.

For example, instead of how we bookmark multiple lines if multiple lines are selected, we would bookmark only the first selected line, and perhaps save the cursor position on that line?

dschuyler · 2018-02-13T03:19:18Z

app/actions.py

@@ -258,6 +277,38 @@ def bookmarkRemove(self):
    self.bookmarks = rangeList[:begin] + rangeList[index:]
    return True

+  def bookmarkMove(self, upper, delta):


How about changing upper to something like startRow.

Androbin · 2018-09-13T22:13:45Z

Have we yet figured out how to treat the edge cases?

Androbin mentioned this pull request Feb 2, 2018

Bookmark stays in the same line numbers #137

Open

dschuyler reviewed Feb 2, 2018

View reviewed changes

Androbin force-pushed the bookmark branch 3 times, most recently from c9d538a to c1231fd Compare February 3, 2018 00:24

Androbin force-pushed the bookmark branch from 1987c81 to c1231fd Compare February 5, 2018 17:32

Androbin force-pushed the bookmark branch from 717ba35 to f2fb2e6 Compare February 5, 2018 19:15

Androbin force-pushed the bookmark branch from f2fb2e6 to 486c52a Compare February 5, 2018 19:46

dschuyler reviewed Feb 13, 2018

View reviewed changes

Androbin force-pushed the bookmark branch 3 times, most recently from 750cfcb to b7da6d1 Compare March 17, 2018 13:37

Androbin force-pushed the bookmark branch from b7da6d1 to 298eeab Compare July 3, 2018 23:30

Androbin force-pushed the bookmark branch from 298eeab to 35b276a Compare July 11, 2018 18:49

Androbin force-pushed the bookmark branch from 35b276a to 26dd8c2 Compare August 19, 2018 20:31

Androbin force-pushed the bookmark branch from 26dd8c2 to 0872b47 Compare September 13, 2018 22:13

Androbin force-pushed the bookmark branch from 0872b47 to 7f1efa0 Compare April 11, 2019 21:50

Move bookmarks on changes to line numbers

ac6ac88

Androbin force-pushed the bookmark branch from 7f1efa0 to ac6ac88 Compare May 21, 2019 17:29

Move bookmarks on changes to line numbers #147

Are you sure you want to change the base?

Move bookmarks on changes to line numbers #147

Conversation

Androbin commented Feb 2, 2018

dschuyler left a comment

Choose a reason for hiding this comment

dschuyler Feb 2, 2018

Choose a reason for hiding this comment

dschuyler Feb 2, 2018

Choose a reason for hiding this comment

Androbin Feb 2, 2018

Choose a reason for hiding this comment

dschuyler Feb 2, 2018

Choose a reason for hiding this comment

Androbin Feb 2, 2018

Choose a reason for hiding this comment

aaxu commented Feb 2, 2018 • edited Loading

Androbin commented Feb 2, 2018

aaxu commented Feb 2, 2018

Androbin commented Feb 2, 2018 • edited Loading

aaxu commented Feb 2, 2018

Androbin commented Feb 2, 2018 • edited Loading

aaxu commented Feb 2, 2018

Androbin commented Feb 2, 2018

Androbin commented Feb 3, 2018

aaxu commented Feb 5, 2018

Androbin commented Feb 5, 2018

aaxu commented Feb 5, 2018

aaxu commented Feb 5, 2018

Androbin commented Feb 5, 2018

Androbin commented Feb 5, 2018

aaxu commented Feb 5, 2018

aaxu commented Feb 5, 2018

Androbin commented Feb 5, 2018 • edited Loading

Androbin commented Feb 5, 2018

aaxu commented Feb 5, 2018

aaxu commented Feb 5, 2018

Androbin commented Feb 5, 2018

aaxu commented Feb 5, 2018 • edited Loading

Androbin commented Feb 5, 2018

dschuyler commented Feb 7, 2018

Androbin commented Feb 7, 2018

Androbin commented Feb 7, 2018 • edited Loading

aaxu commented Feb 8, 2018 • edited Loading

dschuyler Feb 13, 2018

Choose a reason for hiding this comment

Androbin commented Sep 13, 2018

aaxu commented Feb 2, 2018 •

edited

Loading

Androbin commented Feb 2, 2018 •

edited

Loading

Androbin commented Feb 2, 2018 •

edited

Loading

Androbin commented Feb 5, 2018 •

edited

Loading

aaxu commented Feb 5, 2018 •

edited

Loading

Androbin commented Feb 7, 2018 •

edited

Loading

aaxu commented Feb 8, 2018 •

edited

Loading