sublimehq / sublime_text

Issue tracker for Sublime Text
https://www.sublimetext.com
803 stars 39 forks source link

Prevent on_modified commands from breaking undo history #5616

Open eugenesvk opened 1 year ago

eugenesvk commented 1 year ago

Problem description

(this is similar to the closed issue https://github.com/sublimehq/sublime_text/issues/1817, though that issue deemed the resulting behavior positive and was closed, while I view it as negative and would prefer a fix; also, the cause is somewhat different) I'm using a PersistentRegexHighlight plugin to highlight so non-standard whitespace chars (e.g. non-break spaces etc.) However, this plugin introduces a serious limitation to the undo history behavior — with it it's only able to undo one character at a time instead of roughly one word at a time, which is rather inconvenient and requires too many Ctrl-Z's.

As far as I understand this is due to the fact that the plugin invokes this command on every change (it's called from this line that defines a on_modified behavior)

Preferred solution

  1. I'd prefer if undo history would be able to somehow ignore all those extra invocations of on_modified commands from this plugin and continue to treat typing a word as typing a word, not as a sequence of separate characters interrupted by some command

Alternatives

I can disable the on_modified functionality, though that would come at a cost of "live" functionality, which I'd very much prefer to retain (it's rather valuable to spot typos etc.) Maybe there is a potential plugin coding change that could fix it, thought the plugin hasn't been updated for years and that's unlikely to happen. But if it's something simple, please advise, maybe I could tweak a local install.

Additional Information

No response

deathaxe commented 1 year ago

I don't see a sane way how ST should handle the undo stack better without breaking something if a plugin synchronously invokes a potential text manipulating command after each key stroke from within an event handler. What should the criteria to group operations be?

IMHO, there are 2 possible ways out:

  1. add some kind of debouncing to PersistentRegexHighlight's event listener so highlighting is updated once typing is finished or paused to not interrupt each key stroke.
  2. avoid invoking TextCommand from within on_modified() in order to just add some Regions (view.add_regions()) for highlighting purpose.
keith-hall commented 1 year ago

I'm using a PersistentRegexHighlight plugin to highlight so non-standard whitespace chars (e.g. non-break spaces etc.)

this specific use case for the plugin really isn't necessary any more since ST4 added the draw_unicode_white_space preference...

eugenesvk commented 1 year ago

I don't see a sane way how ST should handle the undo stack better without breaking something if a plugin synchronously invokes a potential text manipulating command after each key stroke from within an event handler. What should the criteria to group operations be?

But there is no text modification, only some highlighting changes, can ST not see that no text changed?

avoid invoking TextCommand from within on_modified() in order to just add some Regions (view.add_regions()) for highlighting purpose

Oh, so it's possible to just reclassify this TextCommand into some other category so that it's not a "potential text manipulating" command? That sounds doable, though what is the other category? I've briefly checked that API docs and WindowCommand/ApplicationCommand is not it either. Or do you imply that instead of executing a command the plugin would need to "just" execute all the code the command is currently doing itself without invoking the TextCommand?

add some kind of debouncing to PersistentRegexHighlight's event listener so highlighting is updated once typing is finished or paused to not interrupt each key stroke.

Yeah, that would be awesome (also for performance reasons), debouncing is seriously underused in many cases, though no chance for this abandoned addon :( Do you know of any good example of an addon that does that?

eugenesvk commented 1 year ago

this specific use case for the plugin really isn't necessary any more since ST4 added the draw_unicode_white_space preference...

Unfortunately, it's still necessary since the native way <EMSP> <ENSP> <NBSP> is just awful — after all, not all the spaces are typos, some of the no-break space chars and the like are used for legitimate typography. What the addon is doing is just right — visible, but at the same time unobtrusive underlines of various shapes/colors

eugenesvk commented 1 year ago

2. avoid invoking TextCommand from within on_modified() in order to just add some Regions (view.add_regions()) for highlighting purpose

Ok, so I've copied the highlighting logic that was previously in a separate TextCommand to a class that doesn't declare any ST command allegiances ;) and it seems to continue working without breaking the undo history. Thanks for the suggestion!

I still think it'd be conceptually valuable for ST to differentiate between "potential" text edits and actual text edits, but for my specific issue I'd submit a more immediate upstream fix

keith-hall commented 1 year ago

Possible duplicate of https://github.com/sublimehq/sublime_text/issues/2975

deathaxe commented 1 year ago

I still think it'd be conceptually valuable for ST to differentiate between "potential" text edits and actual text edits

A text command is called synchronous from a synchronous event handler, which indicates a text change. I guess the edit token being created for the invokd text command therefore takes a snapshot of the buffer in some intermediate (outdated?) state and therefore records the most recently typed character as if it was added by the text command itelf. ... or something along those lines.

Maybe using on_modified_async would already help here, but I haven't tried something like that.

eugenesvk commented 1 year ago

Maybe using on_modified_async would already help here, but I haven't tried something like that

interesting idea, but unfortunately that didn't help :(