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

Undo manager should wrap reinserted text #509

Closed
quangbuule opened this issue Oct 27, 2015 · 5 comments · Fixed by #3823
Closed

Undo manager should wrap reinserted text #509

quangbuule opened this issue Oct 27, 2015 · 5 comments · Fixed by #3823
Labels

Comments

@quangbuule
Copy link

Selection UX is really bad. For example when we select text to delete, then we undo after deleting, the selection should wrap that text (also be forwards or backwards if needed). I think we should store selection to stacks also.

@jhchen
Copy link
Member

jhchen commented Oct 27, 2015

I think an acceptable heuristic is just wrap reinserted text that was deleted? Also what do you mean by forwards or backwards?

@jhchen jhchen changed the title Undo manager is not working right Undo manager should wrap reinserted text Oct 27, 2015
@quangbuule
Copy link
Author

Backwards mean the selection start from the right, and end to the left (by Shift + Left). The difference compare to forwards is after we do a backwards selection, we press Shift + Left again then the selection should expand to the left. Currently quill.setSelection cannot do that, I have to write quill.prototype.setBackwardSelection for my case.

@quangbuule
Copy link
Author

Wrap reinserted text is ok, but the undo manager is not doing right in other case also so I think the implementation of _getLastChangeIndex is not correct in UX perspective. For example, we deleted this text "Lorem ipsum" then undo, the selection move to right before the "ipsum" while we expected it should be right after ipsum.

Can you explain the implementation of _getLastChangeIndex? I was confused when I read it.

@jhchen jhchen added this to the Quill 1.0 milestone Dec 15, 2015
@jhchen jhchen removed this from the Quill 1.0 milestone Aug 29, 2016
@thomsbg
Copy link
Contributor

thomsbg commented Mar 15, 2018

I just noticed this behavior too.

I think an acceptable heuristic is just wrap reinserted text that was deleted? Also what do you mean by forwards or backwards?

I think I'd amend this to make it more precise by saying: if the last op of the original delta was a highlight-and-delete, then the restored characters should be selected when that delta is un-done.

The problem is, it's impossible to know whether a { delete: 1 } op came from a highlight-and-delete operation, or from a simple backspace. Perhaps this means that the clipboard module needs to keep track of selection history?

@thomsbg
Copy link
Contributor

thomsbg commented Apr 11, 2018

@jhchen I encountered another reason for the clipboard module to track the selection history. I have a key binding set up to turn 1/2 into ½ once the spacebar is pressed, resulting in ½ (including the inserted space).

I have more similar keybindings to accomplish the same for -- => (emdash), " => (curly quotes), etc.

My key bindings insert the space first, before calling history.cutoff() and doing the automatic transformation. I would like the editing UX to be such when you press undo, the automatic transformation is undone, keeping the inserted space, and keeping the selection unchanged (after the inserted space) to be able to continue typing seamlessly.

Right now, the act of undoing the change from 1/2 into ½ moves the cursor before the inserted space, because of the getLastChangeIndex logic in the clipboard module. Ideally, the history module would capture the selection when it calls record(), and then would restore that range after applying the undo/redo delta. That way, the selection can be restored accurately to what it was at the time the original change was made (i.e. after the space in this case).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants