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

Restore selectionsMarkerLayer of different editor #16564

Merged
merged 3 commits into from Mar 21, 2018

Conversation

Projects
None yet
7 participants
@t9md
Contributor

t9md commented Jan 15, 2018

Description of the Change

Aiming to fix: #16176
Depends atom/text-buffer#287

  • Register this.selectionsMarkerLayerId to buffer on instantiation
  • Pass selectionsMarkerLayerId on transact/undo/redo/createCheckpoint/groupChangesSinceCheckpoint

Scenario this PR intends

  • When same buffer is opened in two editor(editor1 and editor2).
  • And make multiple changes sometimes on editor1 and sometimes on editor2.
  • Then undo/redo.

Before This PR

Undo/redo restore selection(= cursor) snapshotted on that editor.

before2

After this PR

Undo/redo restore selection(= cursor) snapshotted on change initiated editor.

after2

Alternate Designs

I don't know

Why Should This Be In Core?

Since we cannot fix this on user side(init script/packages).

Benefits

User can see textual change when undo/redo where multiple editors which shares same buffer instance were opened.

Possible Drawbacks

Add some complexity, adding text-editor specific code onto text-buffer's core.

Verification Process

Scenario Which I want to fix(or improve)

Open multiple text-editors which shares same buffer

Condition
  • Open editor1, make change-1
  • Open editor2 by copying editor1, make change-2
  • Open editor3 by copying editor2, make change-3
  • Now editor1, editor2, editor3 shares same buffer instance and have distinct selectionsMarkerLayer which is used to keep distinct cursors
Current behavior
  • Undo on editor1, editor1's cursor is not restored to point-of-before-change-3.
  • Undo on editor2, editor2's cursor is restored to point-of-before-change-2, since editor2 is same editor where change-2 was made.
  • Undo on editor3, editor3's cursor is not restored to point-of-before-change-1.
  • Open editor4 by copying editor3 and then destroy editor1, editor2, editor3
  • Undoing/Redoing on editor4 only undo/redo textual changes, but cursor is not restored, and not scrolled to textual changes, because selectionsMarkerLayer is not shared across editors.
New behavior

NOTE: Require changes' in atom's text-editor to use new machanism

  • Undo on editor1, editor1's cursor is restored to point-of-before-change-3 by restoring marker snapshot taken on editor3's selectionsMarkerLayer to editor1.
  • Undo on editor2, editor2's cursor is restored to point-of-before-change-2.
  • Undo on editor3, editor3's cursor is restored to point-of-before-change-1.
  • Open editor4 by copying editor3 and then destroy editor1, editor2, editor3
  • Undoing/Redoing on editor4 does undo/redo textual changes, and cursor is restored to each snapshot-ed point when change was made, because when undo/redo selectionsMarkerLayer is restored from different selectionsMarkerLayer which shares same buffer.

Applicable Issues

#16176

@iolsen

This comment has been minimized.

Contributor

iolsen commented Jan 23, 2018

@nathansobo @maxbrunsfeld any early feedback on this for @t9md?

@nathansobo

This comment has been minimized.

Contributor

nathansobo commented Feb 22, 2018

This is a pretty daunting change, and I'm not super sold that it's worth the complexity and risk. How real of a problem does this end up being in practice? At the very least, getting a green build would make me more excited to review it.

@t9md

This comment has been minimized.

Contributor

t9md commented Feb 22, 2018

This is a pretty daunting change, and I'm not super sold that it's worth the complexity and risk. How real of a problem does this end up being in practice? At the very least, getting a green build would make me more excited to review it.

I'm a bit confused by this comment, since I thought you understand background and said "make sense" in atom/text-buffer#287 (comment)
Why I stopped to proceed this PR is I'm waiting comment for this and updated atom/text-buffer#287.

And I need this feature in practice.
While I use atom, I frequently casually split-pane to open same file side-by-side.
After spliting by split-right-and-copy-active-pane and make some change without taking much care for which editor I made change.
But currently undo/redo cursor restoration is happen in change-initiated editor only, I want make atom to restore cursor regardless of editor as long as underlying buffers are same.

@smashwilson

This comment has been minimized.

Member

smashwilson commented Mar 19, 2018

@t9md I chatted about this offline a bit with @nathansobo and @maxbrunsfeld and we're all 👍 .

I've merged atom/text-buffer#287, released it in text-buffer@13.12.1, and added it to atom in cdab85d, so if you merge master you should be able to use marker roles. Go ahead and ping me when you've got it in a good state and I'll help you get it merged 😄

t9md added some commits Jan 15, 2018

Pass selectionsMarkerLayer on transact, undo and redo
To restore selections of change initiated editor on undo/redo.
@t9md

This comment has been minimized.

Contributor

t9md commented Mar 21, 2018

@smashwilson Thanks for moving forward this PR!
Now ready to merge. Feel free to ask/change request for any concerns.

@smashwilson

This comment has been minimized.

Member

smashwilson commented Mar 21, 2018

👍 LGTM

@smashwilson smashwilson merged commit fd2d7ba into atom:master Mar 21, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@t9md

This comment has been minimized.

Contributor

t9md commented Mar 21, 2018

WOW thanks!

@maxbrunsfeld

This comment has been minimized.

Contributor

maxbrunsfeld commented Mar 21, 2018

Great job coming up with this solution @t9md.

@Ben3eeE

This comment has been minimized.

Member

Ben3eeE commented Mar 21, 2018

Thank you @t9md 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment