Unify trim/delete/insert events as 'move' events. #1948
Conversation
This solves GitHub issue xtermjs#1946.
| if (this.selectionStart) { | ||
| this.selectionStart[1] -= amount; | ||
| if (this.selectionStart && this.selectionStart[1] >= event.start | ||
| && (! event.end || this.selectionStart[1] < event.end)) { |
There was a problem hiding this comment.
Personally I think the current way is much more readable and explicit in explaining what is actually happening, the need to check for end? and handle it is one of the main reasons I think this.
Another good example:
// Where is it moving?
model.onMove({start: 0, amount: 1});A negative amount is also a little confusing unless you go and read the definition of IMoveEvent.
There was a problem hiding this comment.
Gonna answer below to stay in the thread flow...
|
I see the main reason for unifying them is that I believe anywhere you'd need a handler for one would need a handler for all three. Having to write 3 handlers is both more verbose and more error-prone (likely to forget one or make a mistake in one). |
|
Well I dont quite the point of this idea, tbh. Is this just for API unification? Imho this is more a matter of taste (code style) than it shows real benefits. It also has drawbacks. A "one event fits all purposes" paradigm kinda defeats language features like strong typing (well for TS at least during compile time), you will have to read the docs to grasp the correct meaning of an event with parameters xy. With separate events you still need to know what a particular event means, but TS can assist you through several levels of invocations. Thats all not possible with a single event type, where you instead have to implement the meaning orthogonal to the type system. Much like redux's actions with TS, they kinda feel silly in TS as they cannot not leverage the type system well. |
|
I claim there is no functional difference between a 'trim' and a 'delete' event, and there is no place in the code (or possible addons) where you would want to handle one and not the other, or handle them differently: 'trim' is just a special case of 'delete'. When it comes to 'insert' vs 'delete', the only difference is one of sign, and I also believe there is no place where you would want to handle one and not the other. This to me argues strongly for unifying these events. |
This is a good point and I could see us merging these, but it would only result in more GC. Right now I find things fairly readable and merging these events would mean a new object would need to be created for every trim as the trim event just passes in the amount with the event right now. Since thousands+ of these can fire a second that's a lot of We'll just have to be diligent in making sure we listen to everything 🙂 |
This solves GitHub issue #1946.