OpApplyDirectStyling created on text replacement fails in collab mode

[kossebau]

Each time when overwriting existing text with other text also a OpApplyDirectStyling is created with the complete style properties of the first character in the selection, to ensure the current styling is kept (at least that way reasoned in #296, commit e2b7b378f152dc976836537175f18e8678a6e243).

While this works fine in single-editor mode, because the styles are resolved to nothing when executed by odf.TextStyleApplicator in the same style environment, it is a problem in collab mode, because it is handled as a serious op, with the intention to style with all the properties, which only results in unexpected stylings, as we saw today.

No idea yet how to apporach the original problem in another, less side-effecting way.

[peitschie]

which only results in unexpected stylings, as we saw today.

Are you able to provide more detail and some worked examples of what exactly didn't work correctly? There are definitely shortcomings and bugs with the current implementation of OpApplyDirectStyling (and how cursor styling is implemented), but the basic premise I would have thought still largely applies in a collab environment.

Some improvements that could be made regardless

1. OpInsertText should grow at least one, possibly two properties: characterStyleName, and possibly direct character styling as well (e.g., for use with direct cursor styling). This makes it the responsibility of the creating client to set the style to use for character insertion. This allows direct cursor styling (i.e., setting the bold state just before typing a bold character) to be implemented by creating an auto style that is sent just before the insert-op, with the insert op requesting the new autostyle as it's character style.
2. Remove the existing OpApplyDirectStyling entirely. It should be replaced with an OpSetCharacterStyle that behaves almost identical to SetParagraphStyle. I.e., Applying bold to a selection should pre-calculate on the initiating client the number of different auto-styles that are necessary, and send 1 text range per unique auto-style. E.g., if a half italic, have bold sentence is selected and underlined, two OpSetCharacterStyle ops will be created, one setting the style on the italic side, one on the bold side. Effectively, the container calculation logic in TextStyleApplicator is moved to only occur once on the generating client.

The result of these two changes is very deterministic behaviour of the operations (rather than TextStyleapplicator which can change it's behaviour at execution). In addition, the ops become simpler as more of the logic is moved into the controller used to create them.

I can provide PoCs pretty quickly showing these changes... but it'd help to get more detail on the collab issue first in case there is a simpler fix available :)

[kossebau]

Are you able to provide more detail and some worked examples of what exactly didn't work correctly?

Quick example: Collab session of users A & B with welcome.odt. User A selects "about" in "What is so exciting about it?", then presses "o" to replace "about" with "o". Which results in the ops "RemoveText", "InsertText" and "ApplyDirectStyling". User B puts cursor at begin of "What is so exciting about it?", presses Backspace to join this paragraph with the preceding. Results in op "RemoveText". The resulting merged paragraph has the style of the first paragraph, "Text Body". Now the ops of A and B are synched, for converging the two documents of A & B. With the current system this means that the "ApplyDirectStyling", which was based on the state at A at that time (including all properties of paragraph style "Heading 1"), will be applied on the text at B which then has "Text Body" as paragraph style.

[kossebau]

This allows direct cursor styling (i.e., setting the bold state just before typing a bold character) to be implemented by creating an auto style that is sent just before the insert-op, with the insert op requesting the new autostyle as it's character style.

Sadly this is not possible with the current system where automatic text styles are named on creation by the clients as they are needed, so names are not shared across clients (see #181).

[peitschie]

Sadly this is not possible with the current system where automatic text styles are named on creation by the clients as they are needed, so names are not shared across clients (see #181).

That is exactly what I am proposing we change :-). In the existing system, it is already possible to create and send automatic styles to other clients (see the paragraph styling such as alignment and indent).

I believe my approach would fix the issue you've identified with the paragraph merging. In my proposed idea, the situation would work as follows:

1. User A selects "about" and replaces with "o". This generates RemoveText, InsertText, AddStyle(auto char style with relevant properties for the "o" specified), SetCharacterStyle(on the "o")
2. User B merges paragraph into previous one. This generates RemoveText

On convergence, because User A's operations now specify all character-style information, the "o" will be consistently named and styled on all clients.

[vandenoever]

What I fail to see in the given example, "replace 'about' with 'o'" is why a style is applied at all? I'd think that the intent would be for the use to give 'o' the same style as 'about'. By applying InsertText first and then RemoveText, the style would be kept.

[peitschie]

By applying InsertText first and then RemoveText, the style would be kept.

That's the user experience we've had to implement, yes :wink:

The problem is, the step system does not allow text insertion inside the (now) empty span, so in order to achieve this user behaviour, we re-apply the style after text insertion.

[vandenoever]

How can the span be empty?

'about' -> InsertText 'o' -> 'abouto' -> RemoveText 'about' -> 'o'

If the controller creates the operations like that, then at least the issues does not occur in the current UI. I'm asking because OwnCloud would like to update to a newer WebODF. If issue #551 does not occur in practice, they could upgrade.

[peitschie]

Agreed that the described scenario would avoid the issue (at least, for this specific case, haven't considered hard enough whether it has it's own version of this bug).

Unfortunately, that is not what the current code does. The existing code does the remove first, followed by the insert. So, in the existing implementation, the span is undoubtedly empty.

This is only one of the difficulties however, as similar problems can still be created in collab mode no matter how you do the insert/remove pair.

E.g., client A does: 'about' -> InsertText 'o' -> 'abouto'

client B does: select 'about' -> RemoveText

When the insertion eventually arrives at client B, the span was already long destroyed that client A inserted it into.

[peitschie]

I'm asking because OwnCloud would like to update to a newer WebODF. If issue #551 does not occur in practice, they could upgrade.

Which version is OwnCloud currently using? Does their current version have collab mode?

[vandenoever]

They use collaborative editing with webodf 0.4.2 https://github.com/owncloud/documents/blob/master/js/3rdparty/webodf/webodf.js and have some serious issues trying to upgrade: https://github.com/owncloud/documents/pull/419

[peitschie]

Based on the comments on that issue, that is different possibly from this current one. This issue is about a specific edge case when working in collab mode which as far as I can tell has existed as long as we've attempted to support direct formatting in collab.

Are we able to get more specific reproduction cases from them?

[peitschie]

Just for record keeping, some discussion on this has already occurred in https://github.com/kogmbh/WebODF/issues/37#issuecomment-25418471

[VicDeo]

@peitschie exact reproduction steps are listed in https://github.com/owncloud/documents/pull/354#issuecomment-54651167

UPD: I mean the first and the second issues with applying font size and bold/italic/etc, the third issue is surely unrelated to WebODF.

[peitschie]

@peitschie exact reproduction steps are listed in owncloud/documents#354 (comment)

UPD: I mean the first and the second issues with applying font size and bold/italic/etc, the third issue is surely unrelated to WebODF.

Are you able to drop by IRC and discuss those issues sometime? They appear unrelated to the bug described here. The font sizes & bold/italics etc. are all currently working in trunk's split-screen editor demo.

[VicDeo]

@peitschie yes, I've tried to sync from scratch and it does work now. Strange.

[peitschie]

@peitschie yes, I've tried to sync from scratch and it does work now. Strange.

Indeed, that is unexpected. Still... glad to hear it works :smile: