Cleanup: Simplify open document - #1773
Conversation
This reverts commit 3a5e5fb.
|
@jeremypw can you resolve conflicts please |
# Conflicts: # src/MainWindow.vala
|
Converting back to draft for testing after tricky merge |
| // to confine Document creation to this class. | ||
| // Documents are treated the same regarding the focus, cursor_position and range parameters | ||
| // whether already open or not. | ||
| // Cursor position may have any value > -2 to set cursor. Lower values are ignored (default) |
There was a problem hiding this comment.
Because -1 and 0 are valid values, sending the cursor to the start or end of the document. So I chose -2 or anything more negative to mean leave the cursor where it is.
There was a problem hiding this comment.
Ah that's interesting. Can you document that? I didn't find that choice immediately obvious
There was a problem hiding this comment.
I learnt this from https://valadoc.org/gtk+-3.0/Gtk.TextBuffer.get_iter_at_offset.html which is what the sourceview uses to place the cursor. The interpretation of a position <= -2 is mine as it could be useful to not specify a cursor position. I'll make the comment clearer.
There was a problem hiding this comment.
Looking at it afresh it looks like I set the cursor position twice. I'll fix that.
There was a problem hiding this comment.
Actually the "-2" value of the parameter is only used internally when a range override present.
|
Probably to much to review - I'll try and split up. |
@jeremypw I don't think that is case. I was about to review it, but if you really want to break it up go ahead. |
|
@zeebok Sorry, I have reset it as "ready". Thanks for having a look at this. |
zeebok
left a comment
zeebok
left a comment
There was a problem hiding this comment.
Just a couple thoughts but it seems to work well with no found regressions!
| for (int n = 0; n < docs.length (); n++) { | ||
| doc = docs.nth_data (n); | ||
| //TODO Is this check necessary? | ||
| if (doc == null || doc.file == null) { |
There was a problem hiding this comment.
I feel like if we are checking any doc of being null before adding it to the list of docs, or the view, we probably don't need the check
There was a problem hiding this comment.
You are probably right about doc; not so sure about doc.file - there is nothing to stop this being set to null by an external call as it is public. This will be the subject of future cleanup PRs. We should not need to keep a separate list of docs - we can already iterate over the tabpage children if needed. We should make doc.file private and stop it becoming null then the check can be removed. I'll leave this for now as outside scope.
Thanks for the review!
| // Values of -1 and greater than number of characters | ||
| // in the buffer place the cursor at the end of the buffer. | ||
| // Ignore lower values | ||
| if (value <= -2) { |
There was a problem hiding this comment.
Do we want to ignore this, or clamp it? What happens if the cursor position is larger than the document supports? We may want to just mimic that.
There was a problem hiding this comment.
I am relying on https://valadoc.org/gtk+-3.0/Gtk.TextBuffer.get_iter_at_offset.html which says that TextBuffer.get_iter_at_offset (out iter, int char_offset) already handles any offset from -1 upwards (even if beyond the end of the buffer). The value of -2 is our own special case for "leave the cursor alone" used for a range override.
3 participants
Fixes #1703
The foldermanagerview member of window is temporarily made public but after #1778 this will not be necessary as that function will be moved to GitManager