Copy-markers clipboard mechanism refactor #16005
Conversation
Mati365
changed the title
change:data when copy with markers
Mati365
force-pushed
the
ck/15943
branch
2 times, most recently
from
c6efece to
803f623
Compare
| range | ||
| } ) ) | ||
| .filter( marker => this._canPerformMarkerClipboardAction( marker.name, action ) ); | ||
| return entries.flatMap( ( [ markerName, range ] ): Array<CopyableMarker> => { |
There was a problem hiding this comment.
I can see that you're using flatMap, presumably to eliminate empty values. However, using flatMap solely for this purpose might be somewhat convoluted. Instead, opting for a for-loop approach can offer simplicity and clarity in code comprehension. By directly iterating over the data and modifying the result array within the loop, we avoid the overhead of creating additional result arrays, which occurs when using flatMap. This direct approach can lead to an easier understanding of the code.
There was a problem hiding this comment.
@DawidKossowski It's a discussion between functional (immutable) and imperative (mutable) approach. From my point of the view, the first one is much easier to debug because it disallows programmer to modify previously added items and allows to easily chain new transformers on already mapped array. In this particular case, performance is not even an issue because it'll work exactly the same for arrays with even ~5000 items.
There was a problem hiding this comment.
I have nothing against functional programming, but we simply don't use such practices in our project.
@niegowski WDYT?
Edit: I've also checked the performance, and it appears that flatMap is slower by ~80% compared to the for-loop for these operations. However, as Mateusz mentioned, given the scale of the data we're working with, this difference is marginal (maximum a few milliseconds).
…tend configuration
There was a problem hiding this comment.
LGTM. There is one open discussion, but I don't think it is a blocker, and we can go with the current approach as well. Anyway, waiting for @niegowski final check.
Suggested merge commit message (convention)
Fix (clipboard): By default, do not copy markers if they are partially selected.
Tests (clipboard): Add missing tests for pasting duplicated markers in table plugin.
Internal (clipboard): Add
duplicateOnPasteto marker clipboard configuration. Closes #15966. Related to #15968.Fix (engine): Do not trigger
change:dataon copy with markers. Closes #15943.Fix (clipboard): Importing from Word crashes when the document contains suggestions.
Fixes https://github.com/cksource/ckeditor5-commercial/issues/6053.
Closes https://github.com/cksource/ckeditor5-commercial/issues/6056.