RFC: Making OpApplyStyle and OpUpdateParagraphStyle more consistent
Hi, OpApplyStyle and OpUpdateParagraphStyle are currently quite different: a) how they encode the style attributes to change b) OpApplyStyle does not really support unsetting of attributes c) OpApplyStyle and related code only does textstyle properties d) OpApplyStyle does not directly modify existing styles, but create any needed automatic styles to have the given styling e) OpApplyStyle has a too generic name Before I start to write anything for OT of OpApplyStyle things needs to be cleaned up a little, to reduce the complexity due to the different behaviour. For a) OpApplyStyle takes some "info" payload that are actually directly the ODF element names and properties+values that should be set to the direct formatting. The names of the property and attributes use the usual prefix to indicate the namespace. OpUpdateParagraphStyle instead has a "setProperties" payload that carries the properties+values to set, with an own id scheme that first needs to be mapped to the ODF values. Q: anyone would mind to rename that "info" property to "setProperties", for some more entropy in the name? Okay to remove the custom property names and mapping in OpUpdateParagraphStyle and use directly the ODF attribute names as well? In matters of b), OpUpdateParagraphStyle has a payload "removedProperties" which stores the names of all attributes that should be removed. That was done like that instead of listing the attribute with value >null<, >undefined< or "" in the "setProperties" to express that is should be removed, because >null< and >undefined< might not exist in other languages and "" could be an actual valid value. Q: does it make sense to add something similar like "removedProperties" to OpApplyStyle? I guess it does. For c) I wonder what the plans are for paragraph styling. There is no TODO or anything else mentioned. And I am only thinking of the op spec, not of the actual implementation how to execute this op. Q: Should the same op be used for direct paragraph styling? So should there be, like in OpUpdateParagraphStyle, another section "paragraphProperties", next to "textProperties"? The difference mentioned in d) seems wanted, as this op is rather a direct mapping of some UI action, like we have with OpRemoveText or OpSplitParagraph. So no question about that issue. WRT e), I propose the name "OpApplyDirectStyling" Q: Better name? Cheers Friedrich -- Friedrich W. H. Kossebau // KO GmbH http://kogmbh.com/legal/
On 25/07/2013, at 5:38 AM, Friedrich W. H. Kossebau wrote:
Hi,
OpApplyStyle and OpUpdateParagraphStyle are currently quite different: a) how they encode the style attributes to change b) OpApplyStyle does not really support unsetting of attributes c) OpApplyStyle and related code only does textstyle properties d) OpApplyStyle does not directly modify existing styles, but create any needed automatic styles to have the given styling e) OpApplyStyle has a too generic name
Before I start to write anything for OT of OpApplyStyle things needs to be cleaned up a little, to reduce the complexity due to the different behaviour.
For a) OpApplyStyle takes some "info" payload that are actually directly the ODF element names and properties+values that should be set to the direct formatting. The names of the property and attributes use the usual prefix to indicate the namespace. OpUpdateParagraphStyle instead has a "setProperties" payload that carries the properties+values to set, with an own id scheme that first needs to be mapped to the ODF values.
+1 I'm happy for this standardisation
Q: anyone would mind to rename that "info" property to "setProperties", for some more entropy in the name? Okay to remove the custom property names and mapping in OpUpdateParagraphStyle and use directly the ODF attribute names as well?
+1 for this as well. I don't think the custom mapping adds any benefit in reality, as that would put the onus on us to write documentation for this custom mapping. At least ODF styles are (fairly) well documented
In matters of b), OpUpdateParagraphStyle has a payload "removedProperties" which stores the names of all attributes that should be removed. That was done like that instead of listing the attribute with value >null<, >undefined< or "" in the "setProperties" to express that is should be removed, because >null< and >undefined< might not exist in other languages and "" could be an actual valid value.
Q: does it make sense to add something similar like "removedProperties" to OpApplyStyle? I guess it does.
This concept doesn't really make any sense for direct formatting. For example: <p style="font-weight: bold"><span>hi</span> there!</p> I might accidentally think that to make the "hi" text non-bold, I should OpApplyStyle with removedProperties =[font-weight]. This doesn't make any real sense though, as the element I'm formatting (the styling of the span) doesn't have a property to remove. If I remove it off the paragraph element, I'll incorrectly unbold " there!" as well. The end user intent is never to remove formatting (with the exception of a specific OpRemoveDirectFormatting option or similar). Rather, when they say "unbold this", they really mean "make this range font-weight=normal".
For c) I wonder what the plans are for paragraph styling. There is no TODO or anything else mentioned. And I am only thinking of the op spec, not of the actual implementation how to execute this op.
Q: Should the same op be used for direct paragraph styling? So should there be, like in OpUpdateParagraphStyle, another section "paragraphProperties", next to "textProperties"?
I wrote OpApplyStyle to allow the possibility of that taking care of paragraph and text properties if desired. But, I'm not particularly attached to that approach. I like OpApplyStyle's range-based operation. It makes translation from the cursor easier, and the operation is still quite straightforward. But… I am worried about whether inverse operations (i.e., proper undo) can be created for range-based operations .
The difference mentioned in d) seems wanted, as this op is rather a direct mapping of some UI action, like we have with OpRemoveText or OpSplitParagraph. So no question about that issue.
WRT e), I propose the name "OpApplyDirectStyling"
+1 I'm happy for that! Thanks again for your work maintaining the technical integrity of webodf :). The codebase is a pleasure to work in (apart from CMakeFiles.txt that I keep forgetting how to update properly…)
Thanks for the comments, Philip! Everybody else, look at him, he is doing it right :) Am Donnerstag, 25. Juli 2013, 00:38:40 schrieb Philip Peitsch:
On 25/07/2013, at 5:38 AM, Friedrich W. H. Kossebau wrote:
Hi,
OpApplyStyle and OpUpdateParagraphStyle are currently quite different: a) how they encode the style attributes to change b) OpApplyStyle does not really support unsetting of attributes c) OpApplyStyle and related code only does textstyle properties d) OpApplyStyle does not directly modify existing styles, but create any needed automatic styles to have the given styling e) OpApplyStyle has a too generic name
Before I start to write anything for OT of OpApplyStyle things needs to be cleaned up a little, to reduce the complexity due to the different behaviour.
For a) OpApplyStyle takes some "info" payload that are actually directly the ODF element names and properties+values that should be set to the direct formatting. The names of the property and attributes use the usual prefix to indicate the namespace. OpUpdateParagraphStyle instead has a "setProperties" payload that carries the properties+values to set, with an own id scheme that first needs to be mapped to the ODF values.
+1 I'm happy for this standardisation
Part of MR100.
Q: anyone would mind to rename that "info" property to "setProperties", for some more entropy in the name? Okay to remove the custom property names and mapping in OpUpdateParagraphStyle and use directly the ODF attribute names as well?
+1 for this as well. I don't think the custom mapping adds any benefit in reality, as that would put the onus on us to write documentation for this custom mapping. At least ODF styles are (fairly) well documented
MR98.
In matters of b), OpUpdateParagraphStyle has a payload "removedProperties" which stores the names of all attributes that should be removed. That was done like that instead of listing the attribute with value >null<,
undefined< or "" in the "setProperties" to express that is should be removed, because >null< and >undefined< might not exist in other languages and "" could be an actual valid value.
Q: does it make sense to add something similar like "removedProperties" to OpApplyStyle? I guess it does.
This concept doesn't really make any sense for direct formatting.
For example:
<p style="font-weight: bold"><span>hi</span> there!</p>
I might accidentally think that to make the "hi" text non-bold, I should OpApplyStyle with removedProperties =[font-weight]. This doesn't make any real sense though, as the element I'm formatting (the styling of the span) doesn't have a property to remove.
If I remove it off the paragraph element, I'll incorrectly unbold " there!" as well.
The end user intent is never to remove formatting (with the exception of a specific OpRemoveDirectFormatting option or similar). Rather, when they say "unbold this", they really mean "make this range font-weight=normal".
While I agree for what the user expects by his input via the UI, I was more thinking in op spec terms and of people doing some manipulations of the DOM based on algorithms, where they might want to be able to also remove properties, not just add/change.
For c) I wonder what the plans are for paragraph styling. There is no TODO or anything else mentioned. And I am only thinking of the op spec, not of the actual implementation how to execute this op.
Q: Should the same op be used for direct paragraph styling? So should there be, like in OpUpdateParagraphStyle, another section "paragraphProperties", next to "textProperties"?
I wrote OpApplyStyle to allow the possibility of that taking care of paragraph and text properties if desired. But, I'm not particularly attached to that approach.
Okay, so would be possible to have also paragraph properties in the same way like in OpUpgradeParagraphStyle in theory. Just needs someone in practise to write the proper code :)
I like OpApplyStyle's range-based operation. It makes translation from the cursor easier, and the operation is still quite straightforward. But… I am worried about whether inverse operations (i.e., proper undo) can be created for range-based operations .
Agreed. Same fate as OpRemoveText possibly...
The difference mentioned in d) seems wanted, as this op is rather a direct mapping of some UI action, like we have with OpRemoveText or OpSplitParagraph. So no question about that issue.
WRT e), I propose the name "OpApplyDirectStyling"
+1 I'm happy for that!
Part of MR100.
Thanks again for your work maintaining the technical integrity of webodf :). The codebase is a pleasure to work in (apart from CMakeFiles.txt that I keep forgetting how to update properly…)
:) And once more thanks for your fine feature additions! Cheers Friedrich -- Friedrich W. H. Kossebau // KO GmbH http://kogmbh.com/legal/
On 27 July 2013 01:15, Friedrich W. H. Kossebau
In matters of b), OpUpdateParagraphStyle has a payload "removedProperties" which stores the names of all attributes that should be removed. That was done like that instead of listing the attribute with value >null<,
undefined< or "" in the "setProperties" to express that is should be removed, because >null< and >undefined< might not exist in other languages and "" could be an actual valid value.
Q: does it make sense to add something similar like "removedProperties" to OpApplyStyle? I guess it does.
This concept doesn't really make any sense for direct formatting.
For example:
<p style="font-weight: bold"><span>hi</span> there!</p>
I might accidentally think that to make the "hi" text non-bold, I should OpApplyStyle with removedProperties =[font-weight]. This doesn't make any real sense though, as the element I'm formatting (the styling of the span) doesn't have a property to remove.
If I remove it off the paragraph element, I'll incorrectly unbold " there!" as well.
The end user intent is never to remove formatting (with the exception of a specific OpRemoveDirectFormatting option or similar). Rather, when they say "unbold this", they really mean "make this range font-weight=normal".
While I agree for what the user expects by his input via the UI, I was more thinking in op spec terms and of people doing some manipulations of the DOM based on algorithms, where they might want to be able to also remove properties, not just add/change.
I'm meaning the ambiguity of how a text property is removed seems unsolvable. If I give the instruction to have a text property removed from a block of content in a paragraph, does that mean crawl up the parent styles and clone with this setting removed? Or does it mean delete it off the parent styles directly? Apart from the limited remove direct formatting case (which means delete all auto-style references from the selection), I still don't know how you would remove a text-property. As in the above example, how do you figure out which member in the style hierarchy to remove the text-propery off of? It seems like an extremely murky feature, so personally I'd leave it out until someone finds the first concrete usecase to help us understand what the end goal is :-)
For c) I wonder what the plans are for paragraph styling. There is no TODO or anything else mentioned. And I am only thinking of the op spec, not of the actual implementation how to execute this op.
Q: Should the same op be used for direct paragraph styling? So should there be, like in OpUpdateParagraphStyle, another section "paragraphProperties", next to "textProperties"?
I wrote OpApplyStyle to allow the possibility of that taking care of paragraph and text properties if desired. But, I'm not particularly attached to that approach.
Okay, so would be possible to have also paragraph properties in the same way like in OpUpgradeParagraphStyle in theory. Just needs someone in practise to write the proper code :)
Depending on workload, I may have a crack at this at some point soon. Am I correct in assuming this should have the same style-clone logic (i.e., create an auto-style linked to the parent and override)? Alternatively, if someone else wanted to better understand that part of the code, this would be a wonderful introduction task... ^_^
I like OpApplyStyle's range-based operation. It makes translation from the cursor easier, and the operation is still quite straightforward. But… I am worried about whether inverse operations (i.e., proper undo) can be created for range-based operations .
Agreed. Same fate as OpRemoveText possibly...
I have some theories about Operations being able to generate their inverse as they process which might be able to help here. But... nothing proven or even beyond the thought experiment stage. I suspect that is an entirely other email chain we'll start at some point in time. The landing MRs all look good! It even feels like we had a process & discussion around this change! Well done :D -- Philip Peitsch Mob: 0439 810 260
participants (3)
-
Friedrich W. H. Kossebau
-
Philip Peitsch
-
Philip Peitsch