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

Vertical Cursor Guide #383

Open
wants to merge 32 commits into
base: master
Choose a base branch
from

Conversation

ahelsley
Copy link

Requesting a pull to gnachman:master from ahelsley:vertical-cursor-guide

Changes:

51d1fed (Andrew Helsley, 7 minutes ago)
Fill out cursor column tracking API.

ed11a8e (Andrew Helsley, 8 minutes ago)
Render a vertical cursor guide in the software rendered view (known issue:
only renders on the 3 lines centered on the cursor).

6827cb3 (Andrew Helsley, 13 minutes ago)
Separate control over the vertical cursor guide from the horizontal
guide.

68e3d2e (Andrew Helsley, 28 minutes ago)
Render a vertical guide over the column the cursor is in (Metal View
implementation).

@gnachman
Copy link
Owner

This is a big project! Thanks for taking it on. You've got a really good start here. Sorry it took me a while to look at it; I'll give it a review now.

Interfaces/MainMenu.xib Show resolved Hide resolved
iTerm2XCTests/VT100GridTest.m Outdated Show resolved Hide resolved
sources/Metal/iTermMetalDriver.m Outdated Show resolved Hide resolved
sources/Metal/Renderers/iTermCursorGuideRenderer.m Outdated Show resolved Hide resolved
sources/Metal/iTermMetalDriver.m Outdated Show resolved Hide resolved
sources/PTYSession.m Show resolved Hide resolved
sources/PTYTextView.m Outdated Show resolved Hide resolved
sources/PTYTextView.m Outdated Show resolved Hide resolved
sources/iTermTextDrawingHelper.m Outdated Show resolved Hide resolved
@ahelsley
Copy link
Author

ahelsley commented Apr 2, 2019

Thanks for the review! A quick procedural question: do you mind if I put together all of the adjustments you need as additional commits on this branch, or should I work to integrate your changes into the commits so they form a most pristine sequence of changes?

@gnachman
Copy link
Owner

gnachman commented Apr 2, 2019

Doesn't matter, I'll squash everything before merging. Makes it easier to write the changelog for releases when commits are chronological.

Andrew Helsley added 3 commits April 2, 2019 00:57
…namve vvertexBuffer into verticalGuideVertexBuffer. Finish separating out control over the horizontal cursor guide from the vertical cursor guide.
@ahelsley
Copy link
Author

ahelsley commented Apr 2, 2019

I did a first pass cleanup of the easy stuff. I will get to the more involved cleanups tomorrow or the day after.

@ahelsley
Copy link
Author

ahelsley commented Apr 2, 2019

Oops, I forgot to mention/ask: I've left the completed work with comments indicating what has been done. Would you like me to click "Resolved", or is that something you like to do in order to double-check?

sources/Metal/Infrastructure/iTermMetalRenderer.h Outdated Show resolved Hide resolved
sources/Metal/Renderers/iTermCursorGuideRenderer.h Outdated Show resolved Hide resolved
sources/Metal/Renderers/iTermCursorGuideRenderer.m Outdated Show resolved Hide resolved
sources/PTYSession.m Show resolved Hide resolved
sources/PTYSession.m Show resolved Hide resolved
@gnachman
Copy link
Owner

gnachman commented Apr 3, 2019

Oops, I forgot to mention/ask: I've left the completed work with comments indicating what has been done. Would you like me to click "Resolved", or is that something you like to do in order to double-check?

Feel free to resolve comments, I'll reopen them if there are any problems.

Andrew Helsley added 6 commits April 7, 2019 00:29
…e cursor guide renderer. Create additional texture for vertical cursor guide. Create additional vertex buffers for vertical and split-vertical (used for vertical when both vertical and horizontal guides are enabled) drawing of the vertical cursor guide.
…ation instead of requiring them to be passed into the method.
… complete redraw), do not cause the redraw unless there is good reason to do so (ie the vertical cursor guide is enabled).
@ahelsley
Copy link
Author

@gnachman : I've not finished a rebase before (I'm a bit of a git n00b -- mostly add/commit/pull/push workflow for me so far).

I checked this out as and started work as follows:

hub clone https://github.com/gnachman/iTerm2.git
cd iTerm2
git checkout -b vertical-cursor-guide
... changes to source code etc...
git add ...
git commit ...
git push origin vertical-cursor-guide
... repeated add/commit/push to resolve the requested edits above.

I've tried this to accomplish the rebase in the branch I used to construct this pull request:

git pull --rebase=interactive https://github.com/gnachman/iTerm2.git
... resolve some conflicts ...
git rebase --continue
... repeat the above as necessary...
git push origin vertical-cursor-guide

Can you give me any pointers on what I'm missing?

Andrew Helsley added 7 commits April 12, 2019 18:54
…namve vvertexBuffer into verticalGuideVertexBuffer. Finish separating out control over the horizontal cursor guide from the vertical cursor guide.
…e cursor guide renderer. Create additional texture for vertical cursor guide. Create additional vertex buffers for vertical and split-vertical (used for vertical when both vertical and horizontal guides are enabled) drawing of the vertical cursor guide.
@gnachman
Copy link
Owner

Can you give me any pointers on what I'm missing?

This stack of commits is getting pretty big, which makes rebasing an absolute nightmare. I would squash them. Do this:

  1. Find the first commit that's older than any of yours. Run git log and scroll down til you see it. Make a note of the commit number. Let's call it X.
  2. Do git rebase -i X^
  3. Your text editor will be opened with a list of commits, like:

pick 872e3d6 Your most recent commit
pick b985fa5 Your second-most recent commit
... more of your commits
pick a2ea0af Your oldest commit
pick a2ea0af The first commit before any of yours (commit X)

Change the word pick to "squash" beginning with the second line, and up to the one with your oldest commit (but not commit X). Save the file, and git will combine all your commits into one.

Now you can do git rebase origin/master and you'll only need to resolve conflicts a single time.

@ahelsley
Copy link
Author

I'm not sure the squash made things any prettier. Also, the rebase seems to have pulled in some code that breaks my build:

Classes > Core > PYTSession.m: - (void)handleKeyPressInCopyMode:(NSEvent *)event

... by referring to a variable "_copyModeState". Was this introduced in one of the commits to master?

@gnachman
Copy link
Owner

I'm not sure the squash made things any prettier. Also, the rebase seems to have pulled in some code that breaks my build:

Classes > Core > PYTSession.m: - (void)handleKeyPressInCopyMode:(NSEvent *)event

... by referring to a variable "_copyModeState". Was this introduced in one of the commits to master?

Oh dear, the rebase does not seem to have gone well at all.

What commit was the last good state before the rebase of doom? You should reset to that commit, squash, and then rebase again. If it's giving you grief let me know the commit number and I'll give it a try as well.

@gnachman
Copy link
Owner

Based on what I see here I think the last commit of yours was 64bf5c4. I've squashed and rebased—the only conflict was the copyMode thing you mentioned. Not sure why there was a conflict but that function is gone now. Get my squashed-rebased-vertical-cursor-guide branch. It should get you back to where you need to be.

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

Successfully merging this pull request may close these issues.

2 participants