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

Marking selection to restore #2706

Closed
Reinmar opened this issue Oct 28, 2016 · 8 comments
Closed

Marking selection to restore #2706

Reinmar opened this issue Oct 28, 2016 · 8 comments
Labels
package:undo resolution:expired This issue was closed due to lack of feedback. status:discussion type:feature This issue reports a feature request (an idea for a new functionality or a missing option). type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@Reinmar
Copy link
Member

Reinmar commented Oct 28, 2016

In general, the algorithm which chooses the selection to restore works now really well. I don't remember issues in normal use cases yet.

Unfortunately, in the autoformatting feature the thing is more complex. After typing the "*" character closing the:

<p>xxx **bold me**^</p>

fragment we want to move selection to:

<p>xxx [bold me]</p>

bold this text, move the selection after that fragment and remove the bold attribute from the selection (it will be enabled automatically but we want to quit bold formatting at this point).

The code looks like this:

https://github.com/ckeditor/ckeditor5-autoformat/blob/t/3-selection-issue/src/inlineautoformatengine.js#L146-L155

Now, after autoformatting bolded the word, you may want to undo the default action. What you would like to get after pressing ctrl+z is again <p>xxx **bold me**^</p>, but what you get now is: <p>xxx **[bold me]**</p>.

So what we miss here is the ability to tell the engine/undo feature to use a selection from a particular moment in the process. It's not any more the selection from before the last operation, but more older one.

Do we have any ideas how this could be achieved? I believe that more control over the selection will be needed any way. We have full control over the content cause its changes are grouped in batches, but the selection is independent of all that. It will cause a problems, I'm afraid.

@scofalik
Copy link
Contributor

I don't like how selection and undo work together right now so I am open to discuss this. Unfortunately, this will probably need a bigger refactoring.

@Reinmar
Copy link
Member Author

Reinmar commented Nov 29, 2016

I marked this as a 1.0.0 candidate. It doesn't make sense to deep dive into this now, but it should be considered at some point.

@scofalik
Copy link
Contributor

scofalik commented Nov 30, 2016

There was an idea to hook callbacks somehow to undo steps. The idea is nice but it would be against the other idea that undo integration should be "behind the scenes". When writing a feature you should never thing about undo - that would be perfect.

Since undo operates on "batches" (batches are stores and then reversed) maybe we could somehow add such a callback to the batch? It would be then fired during undoing that batch. Maybe this has some sense.

@Reinmar
Copy link
Member Author

Reinmar commented Nov 30, 2016

It's possible that it can stay "behind the scenes" in the majority of cases. If we'd just enable custom matching selection to batch (or how are they related), then features which need this would be satisfied. In all other cases you wouldn't need using this method and would base on the current default behaviour.

@scofalik
Copy link
Contributor

I am already thinking of other issues, like restoring markers.

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-undo Oct 9, 2019
@mlewand mlewand added this to the unknown milestone Oct 9, 2019
@mlewand mlewand added status:discussion type:improvement This issue reports a possible enhancement of an existing feature. type:feature This issue reports a feature request (an idea for a new functionality or a missing option). package:undo labels Oct 9, 2019
@pomek pomek removed this from the unknown milestone Feb 21, 2022
@CKEditorBot
Copy link
Collaborator

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

@CKEditorBot
Copy link
Collaborator

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may still be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

@CKEditorBot
Copy link
Collaborator

We've closed your issue due to inactivity over the last year. We understand that the issue may still be relevant. If so, feel free to open a new one (and link this issue to it).

@CKEditorBot CKEditorBot added resolution:expired This issue was closed due to lack of feedback. and removed status:stale labels Jan 15, 2024
@CKEditorBot CKEditorBot closed this as not planned Won't fix, can't repro, duplicate, stale Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:undo resolution:expired This issue was closed due to lack of feedback. status:discussion type:feature This issue reports a feature request (an idea for a new functionality or a missing option). type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

No branches or pull requests

5 participants