Review Request 110928: Always save the position of a shape
Sebastian Sauer
mail at dipe.org
Mon Jun 24 12:22:28 BST 2013
> On June 10, 2013, 5:52 p.m., C. Boemann wrote:
> > I still don't see the relation to the bug nor do I understand why we need to save 0,0
>
> Inge Wallin wrote:
> Ah, now I understand why everybody focusses on the bug. I was unclear in my description. "This bug" in the description is about the bug I described, not the bug in bugzilla. I was just mentioning the bug in bugzilla because it provided me with a file where I noticed the problem first time.
>
> Regarding why we need to save 0,0... Can you point me to the place in the spec where it says that 0,0 is the default value? If not, there's your answer.
>
> Elvis Stansvik wrote:
> In 19.573.5 the spec refers to 5.2.1 of the SVG spec which says 0,0 is the default [1]. Not 100% sure this is what we're talking about here, so someone please fill me in if I'm wrong :)
>
> [1] http://www.w3.org/TR/2003/REC-SVG11-20030114/struct.html#SVGElement
>
> Inge Wallin wrote:
> No, this is the svg element. We are talking about svg:x and svg:y attributes. But there is probably a relation.
>
> Elvis Stansvik wrote:
> Right, but that's the section of the SVG spec the ODF spec references in 19.573.5 [1], and it includes the definition of the x and y attributes (see under "Attribute definitions"). So the way I read it is that x/y should be treated the same as for the svg element in the SVG spec. But I may be wrong.
>
> [1] http://docs.oasis-open.org/office/v1.2/os/OpenDocument-v1.2-os-part1.html#__RefHeading__1418216_253892949
>
> Sebastian Sauer wrote:
> Yes, that's correct. Grep for "svg-compatible" in http://docs.oasis-open.org/office/v1.2/OpenDocument-v1.2-part1.html which gives a nice table explaining that ODF uses the svg prefix for "Elements and attributes that are compatible to elements or attributes defined in [SVG]." including link to the SVG specs.
>
> For a longer explanation of the background see http://www.w3.org/Graphics/SVG/WG/wiki/Proposals/SVG_in_ODF
>
> re optional see also http://docs.oasis-open.org/office/v1.2/os/OpenDocument-v1.2-os-schema.rng
>
> <define name="common-draw-position-attlist">
> <optional>
> <attribute name="svg:x">
> <ref name="coordinate"/>
> </attribute>
> </optional>
> <optional>
> <attribute name="svg:y">
> <ref name="coordinate"/>
> </attribute>
> </optional>
> </define>
>
>
> Inge Wallin wrote:
> Ok, that does it. I'm convinced. :)
> Still, I think that it would be nice to always save x and y so that also non-well behaved applications could read the files. Opinions? If there are only negative reactions to this I will retract the RR.
>
> C. Boemann wrote:
> I don't have negative reaction to your suggestion to always save. I mean normally we don't save that many shapes at 0,0 anyway so it's unlikely to be a big change in practice.
>
> Sebastian Sauer wrote:
> hmmm, hmmm... I see multiple problems with that.
>
> 1. I think there is a good reasons that we NOT save default values (not talking here about scg:x/y only but in general default values). That is that we *greatly* (and I mean by factors) reduce the ODF produced what makes it factors faster to investigate, manipulate (hand-edit to check for behavior), save those XML files.
>
> 2. Afaik neither Apache OpenOffice nor LibreOffice nor Microsoft Office do that or?
>
> 3. This would then apply to all default values, right? If yes, 1) applies (lots of noise) plus I fear about the consequences of 2).
>
> 4. This:
>
> > that also non-well behaved applications could read the files
>
> What "non-well behaved applications" we are talking about?
>
> In an ideal world we would never work around broken apps. Unfortunately that's unrealistic and so we do lots of workarounds during loading ODF. But we do so by clearly documenting that (KoOdfWorkaround). Now if we try to do the same for saving it becomes imho an even bigger problems cause:
>
> a) We cannot really target the "consumer" since there is no "if consumer_is_aoo {} else if consumer_is_mso {}" logic. We could probably use app=specific namespaces for that. But that may break on newer versions of the specific app and app-namespaces not contain version-informations.
>
> b) What about conflicts? Like a workaround fixes broken app x but breaks broken app y?
>
> 5. When we do this we need to cross-check at least AAO, LO and MSO that they still proper load our ODF's.
>
> 6. And the most important question (for me) is: What is that what we try to fix here?
>
>
> Inge Wallin wrote:
> > 1. I think there is a good reasons that we NOT save default values (not talking here about scg:x/y only
> > but in general default values). That is that we *greatly* (and I mean by factors) reduce the ODF
> > produced what makes it factors faster to investigate, manipulate (hand-edit to check for behavior),
> > save those XML files.
>
> This is totally new to me. Before you showed me the svg defaults (0, 0) for svg:x and svg:y, I didn't know about *any* default values in ODF. Of course all applications have defaults but I'm talking about the spec here. Also, since you make such a right-out bold statement, I'd like to know what these defaults are and why (as far as I know) not a single one of them is mentioned in the source code of Calligra. The "lacking" svg:x and svg:y didn't look as if it was deliberate but I could be wrong there. A comment in the code would have gone a long way here.
>
> > 2. Afaik neither Apache OpenOffice nor LibreOffice nor Microsoft Office do that or?
>
> The reason I started on this at all was because LibreOffice didn't want to load a file created by Calligra.
>
> > 3. This would then apply to all default values, right? If yes, 1) applies (lots of noise) plus I fear
> > about the consequences of 2).
>
> Of course. But where are those values? I'd love a list of them.
>
> > 4. This:
> >
> >> that also non-well behaved applications could read the files
> >
> > What "non-well behaved applications" we are talking about?
>
> I don't have any specific application in mind. It was an application of the general principle "be generous in what you accept and precise in what you produce".
>
> > In an ideal world we would never work around broken apps. Unfortunately that's unrealistic and so we
> > do lots of workarounds during loading ODF. But we do so by clearly documenting that (KoOdfWorkaround).
> > Now if we try to do the same for saving it becomes imho an even bigger problems cause:
> >
> > a) We cannot really target the "consumer" since there is no "if consumer_is_aoo {} else if
> > consumer_is_mso {}" logic. We could probably use app=specific namespaces for that. But that
> > may break on newer versions of the specific app and app-namespaces not contain version-informations.
>
> Again, the reason all of this started was because LO didn't want to read a file created by Calligra.
>
> > b) What about conflicts? Like a workaround fixes broken app x but breaks broken app y?
>
> I doubt very much that an application that knows that the default for (x, y) is (0, 0) would crash or break if the (0, 0) is found in the output. Could you think of a case where that assumption would make sense?
>
> > 5. When we do this we need to cross-check at least AAO, LO and MSO that they still proper load our ODF's.
>
> See answer to 2. and 4b)
>
> > 6. And the most important question (for me) is: What is that what we try to fix here?
>
> See answer to 2. and 4.
>
> I didn't know about *any* default values in ODF.
Yes, that's a huge problem (still). More so in MSOOXML but also in ODF :/ At least in ODF it was/is improving. In MSOOXML its still try and error reverse-engineering to figure out. That's actually problem number 1 we had with Smartart but well, its ISO :/
> Of course all applications have defaults but I'm talking about the spec here.
See specs as what they are: a help to implement. They are not the ultimate ratio, not 100% perfect and in some cases even just plain wrong. The ultimate ratio are existing documents and producing/consuming applications. The real world out there. That's why things like KoOdfWorkaround exist in the first place. And I think we did a great job there (especially with KoOdfWorkaround) to document that.
Sure in an ideal world everything would be proper documented and would follow documentation to the point but we are on planet earth with human beings. best we can do (and do) is to be compatible to real world, point out problems in the specs (or the real world), document them, talk about them, try to fix/workaround them, etc. pp.
In the case of svg:x and svg:y I not see any such problem here. They are very *well* documented as pointed out before. All major ODF applications - that is AOO, LO and MSO - to proper handle that case.
> Also, since you make such a right-out bold statement, I'd like to know what these defaults are and why (as far as I know) not a single one of them is mentioned in the source code of Calligra.
hmmmm... if its a general rant about missing/incomplete documentation vs default values then +1
> The "lacking" svg:x and svg:y didn't look as if it was deliberate but I could be wrong there. A comment in the code would have gone a long way here.
+2
I would love to see more inline documentation/comments that explains such cases. I absolutely agree that Calligra could really need more inline comments that explain the "why?". More so for cases that are deep hidden in the specs, or more worse, that are result of try+error, based on findings in the AOO code (yes, that also exist) or made out of drunken evenings.
I think it would lower the entry-barrier and time wasted significant if we would/could improve there.
>> 2. Afaik neither Apache OpenOffice nor LibreOffice nor Microsoft Office do that or?
> The reason I started on this at all was because LibreOffice didn't want to load a file created by Calligra.
Yes. I think we have to remember that usually code is read/touched way more then written. So, it would make more then sense to share knowledge (more so if otherwise hard to find) direct in the code so devs have a possibility to discover and understand before getting into conversations with others.
>> 3. This would then apply to all default values, right? If yes, 1) applies (lots of noise) plus I fear about the consequences of 2).
> Of course. But where are those values? I'd love a list of them.
Agreed. I fear such a list does not exist :/ All we have are
a) the specs which are incomplete and probably always will be
b) other existing applications which may have bugs
c) our code which may has bugs
In some cases those points are in competition to each other (eg KoOdfWorkaround). In some cases things change later on (like with OpenFormula or where a workaround becomes obsolete or where a followup-spec fixes a wrongly documented case in a later revison or ...) and so on and on.
Yes, it would make lots of sense to improve the specs and our implementation.
>>> that also non-well behaved applications could read the files
>> What "non-well behaved applications" we are talking about?
> I don't have any specific application in mind.
Okeli, then its not driven by a case (we eg need to workaround), not by the specs (which may wrong), etc.
Then there is the opposite case: If we save default values then we do different then AOO/LO/MSO. Why should we? What problems does that introduce? And is it worth it if there is no reason that we actually have to?
> It was an application of the general principle "be generous in what you accept and precise in what you produce".
Yes, its a good principle and it would solve all the default-value problem if all producers just always save all values. It would probably also make reading/interpreting/saving easier.
I think the reason this was not done is that it produces much bigger XML documents and those need to be saved/loaded/parsed and that takes disk-space, memory, time. A classic trade-off.
Personally I think it makes sense to not save all default values. It makes things not only faster on save/load but it also makes it way easier to debug ODF documents. I seems at least LO, AOO and MSO agree since they do the same.
- Sebastian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/110928/#review34071
-----------------------------------------------------------
On June 10, 2013, 7:18 p.m., Inge Wallin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110928/
> -----------------------------------------------------------
>
> (Updated June 10, 2013, 7:18 p.m.)
>
>
> Review request for Calligra.
>
>
> Description
> -------
>
> When a shape is in the position 0, 0 and there are no other transformations, the position will not be saved back. In other words, there will be an svg:height and svg:width but no svg:x or svg:y. I noticed this while trying to fix https://bugs.kde.org/show_bug.cgi?id=184727.
>
> This patch fixes that the code doesn't save the position. I suggest that we apply it to 2.7 as well and maybe also to 2.6.
>
>
> Diffs
> -----
>
> libs/flake/KoShape.cpp 6d14a8c
>
> Diff: http://git.reviewboard.kde.org/r/110928/diff/
>
>
> Testing
> -------
>
> Tested with the file mentioned above.
>
>
> Thanks,
>
> Inge Wallin
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20130624/95789ebd/attachment.htm>
More information about the calligra-devel
mailing list