-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Disallow undo on "clear authorship colors" #3801
Conversation
Updating
@muxator wanna give your opinion on this? I'd like to merge it. |
I agree removing support for undoing As usual, there is an underlying bug laying somewhere that needs to be tackled (I suspect the usual joys of dynamic languages & lack of runtime validation). Testing it now. |
b2914ee
to
41a2b52
Compare
Edit: message removed, non relevant content. |
Clearing the authorship colors of a document with at least two authors, and then undoing that action caused a disconnect from the pad. This change disallows undoing clearing authorship colors in order to prevent the problem from affecting users, and adds the relative test coverage. This is a change of behaviour, and is documented in the changelog. Fixes ether#2802 (sidestepping it).
41a2b52
to
301e65b
Compare
Since this is a change of behaviour, I added a note in the changelog. Waiting for final comments at #2802, or merging if nothings surfaces. |
NOTE: You will need to merge this into develop then give TW / @Nikerabbit 's teams some time to do translations before releasing..
@Nikerabbit - is there a way we can set a translation requirement as a "priority" so it gets dealt with within X duration? Obv we will compensate for time if required. Or is it possible we can get the strings processed on TW then merge them in? Like if we manually add a en.json change to you guys? |
Due to time constraints here I will get test coverage done within an hour. |
Tests in. |
One thing worth noting is that the change in behavior (lack of undo event in the stack) might confuse some people. For example;
I could write a "blank event" to the undo stack OR we just leave it as it currently stands in this PR. For me I'm inclined to leave it as it is because it's bad practice to pollute the undo stack with "null" events and also we do say "it can't be undone in the dialog (note that this is not present on clearing just a selection of text". I'm no UX expert and I'm mindful we need to get this in quickly. Another option could be to allow undo events of clear authorship to be written to the stack but on execution of the undo event to pop up a gritter msg saying "can't undo clear authorship events".. Either way, we need translations stat :) |
Not nice, but acceptable for me. The other options seem even more confusing. Waiting for translations. |
Don't wait for translations, see above. Afaik TW can't do work until it's in develop! :D |
ba71f9f
to
b56de2d
Compare
Merged, thanks. |
Don't allow user to undo clear authorship colors.
This fixes #2802 in a not so graceful way.
I also think it will fix a lot of broken Etherpad related issues, however, it doesn't stop the potential pad breakage issue. That needs to be fixed somehow, for example, if someone did manage to get the server in a bad state this is only remedied here but not fixed.
The best thing to remedy this would be a plugin (such as ep_author_hover) which shows the author of a span on hover.