-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Unified Paste Handling #3258
Comments
About UI - I believe our users are not interested in the source of the paste content. When I'm copying RTF from Gdocs, Word or any other source, I just want to preserve formatting during paste. Additional buttons for each paste type just introduces complexity for something dumb simple (I mean copy/paste use case, not internal mechanism). I would rather simplify naming by renaming Paste from World into Paste Rich Text should be smart enough to choose correct handler for me - I believe that was the big point of the whole refactoring. Simplifying paste mechanism for end users. About the rest -
Please, let's not rush with the whole our code base anywhere where Overall, seems just the same as our discussion from #2472 (comment) so 👍 |
In general - big 👍 from me. In details, just a few comments.
Split button sounds and looks good here, apart for one thing - If we don't need to
I agree on passing an event. Paste handlers aren't listeners, so I wouldn't pass whole
Not so long ago we reworked autolink to use autocomplete, so it works with typing. Do we need to refactor it again? |
Even with fully independent handlers, |
Generally, that's pretty good idea, especially if it can speed up the development process of other paste handlers (like e.g. Libre Office, Pages, Word 365 - the online one) and make testing easier, so 👍 from me. That being said, from my perspective the best moment for this refactor would be introducing support for pasting from another app (not sure when we will do this), because refactoring just for the sake of refactoring doesn't make sense if we are not going to use this mechanism soon. I have added this issue to a Backlog. When it comes to the UI, theoretically we can change it without refactoring the whole paste handlers, however it will be much more cumbersome. And here I agree with @jacekbogdanski, that we should have at most two paste buttons (
Priority could be useful, it introduces some complexity but I'm afraid we won't avoid it in a long run, so we should think ahead about implementing it and as @Comandeer stated in #3258 (comment) it gives additional flexibility.
☝️ Autolink needs to support typing too as @engineering-this mentioned so paste handler cannot be used to fully support it. Still good that you mentioned what interesting possibilities we can gain with improved paste handling. |
@Comandeer as the architectural changes proposed in this PR will be introduce in #3257, I suppose this issue could be closed together with the #3257 PR? As for the UI part mentioned here, it is a separate topic so I'm for extracting it to a separate issue, WDYT? |
Extracted UI issue to #3276. |
There are still many things worth refactoring, e.g. whole mechanism for pasting images. I'd rather treat this issue as umbrella one for all refactoring tasks. |
I have some thoughts about the current solution. It is not consistent with our event system.
Current approach mixes responsibilities a lot:
I feel like here we should mix our event methods or extend it a little bit if it's necessary. And do not invent a separate algorithm for a task which can be nicely supported with heavily tests functions which are the core of entire editor. |
I've proposed the current architecture mainly to omit the whole event mechanism, as I find it quite cumbersome. Middleware-based architectures are commonly used in JS world, especially in backend side of it (Express.js, Koa) - I transplanted it into the frontend. However I had to make an important change – introduce
They aren't aware of other handlers. function listener( evt ) {
evt.cancel();
}
// vs
function middleware( next ) {
next();
} The difference is that
I don't like that idea. Yes, we use events for everything – but that's exactly the problem here. At the moment |
For me, the default behaviour is to run every handler which returns function middleware( next ) {
next();
} In this case it's 2 arguments method ;) function middleware( evt, next ) {
// ...
next();
} As you noticed it doesn't have to be an event on
But for me, it is a good thing. Why would you like to limit users? There always will be some way "to hack" the flow. I feel like making it harder, more limit ourselves than someone "who inject listener into process of handling content". The general issue for me is that having different flows, architectures, for different parts of the editor significantly increase the complexity of the editor. It creates that thought: "why here is used such an approach, not the same as everywhere else". If we have one pattern used commonly in the entire editor we should use it if we can. Any exception from that should be precisely documented. I see potential future benefits from that approach, where we spend much less time on debugging and figuring out what happens as there everything everywhere works very similar. This feature looks like can easily utilize our event system, however, it has developed different flow. I suspect that maintaining it might cost us more in the future than using events and this is what I would like to avoid. |
Type of report
Feature request
Provide description of the new feature
#3257 introduces initial version of
pastetools
andpastefromword
&pastefromgdocs
based on it. However introducing such model for pasting can be further enhanced to form a unified paste handling based on middleware architecture.Possibilities
Such middleware architecture allows to have multiple handlers for one
paste
event, which opens many nice possibilities:pastetext
plugin can just utilize past handler with low priority to convert pasted data into plain text,pastefromword
andpastetext
in form of private variables) can be unified intopaste
event parameter, which will be handled in another paste handler with low priority,autolink
plugin a paste handler,All mentioned changes can be also seen as simplifying our current paste plugins and making them more modular and reusable.
Architecture
Every plugin can register its own paste handler:
For every
paste
event invoked,pastetools
performs following steps:PasteHandler
s whichcanHandle
returnedtrue
for givenevt
and save it inhandlers
collection.handlers
bypriority
.handlers
intocurrentHandler
.currentHandler
requires external filters, load them.handle
method ofcurrentHandler
.handle
callednext
callback and there are stillPasteHandler
s inhandlers
, return to step 3.UI
Unified paste handling can also benefit from some unified UI. I'm mostly thinking of utilizing
splitbutton
(#1766) for it. Instead of many paste buttons, we can have in fact two: normal paste button and special paste button (or even further reduce it to one paste button, just like Word does). Special paste button will be a split button with several options:I realize that it would require resuming work on
splitbutton
plugin, but TBH I think it's worth it. OTOH middleware architecture can be introduced without introducing also unified UI.WDYT?
The text was updated successfully, but these errors were encountered: