[Patch] Fix for LP 402274

[Maximilian Albert-2]

Hi,

attached is a fix for bug 402274. It's trivial and I cannot imagine it
causing any problems but I'd be glad if someone could take a quick
look anyway in case I overlooked something.

Thanks,
Max

[fix_for_bug_402274.patch]

---

diff --git a/src/text-editing.cpp b/src/text-editing.cpp
index 5b9db13..be12b27 100644
--- a/src/text-editing.cpp
+++ b/src/text-editing.cpp
@@ -1547,7 +1547,7 @@ static bool redundant_double_nesting_processor(SPObject **item, SPObject *child,
     if (!objects_have_equal_style(SP_OBJECT_PARENT(*item), child)) return false;
 
     Inkscape::XML::Node *insert_after_repr;
-    if (prepend) insert_after_repr = SP_OBJECT_REPR(SP_OBJECT_PREV(*item));
+    if (prepend && SP_OBJECT_PREV(*item)) insert_after_repr = SP_OBJECT_REPR(SP_OBJECT_PREV(*item));
     else insert_after_repr = SP_OBJECT_REPR(*item);
     while (SP_OBJECT_REPR(child)->childCount()) {
         Inkscape::XML::Node *move_repr = SP_OBJECT_REPR(child)->firstChild();

---

------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Inkscape-devel mailing list
Inkscape-devel@...
https://lists.sourceforge.net/lists/listinfo/inkscape-devel

[Krzysztof Kosiński]

Yes, it was an obvious omission of a NULL check (in a way).
I assume it compiles and fixes the bug, so go ahead and commit.

Regards, Krzysztof

------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Inkscape-devel mailing list
Inkscape-devel@...
https://lists.sourceforge.net/lists/listinfo/inkscape-devel

[Maximilian Albert-2]

2009/10/3 Krzysztof Kosiński <tweenk.pl@...>:
> Yes, it was an obvious omission of a NULL check (in a way).
> I assume it compiles and fixes the bug, so go ahead and commit.

Thanks for taking a look. Regarding committing, I assumed that only
the release wardens were allowed to commit at this stage, so I'll wait
for one of them to do it.

Cheers,
Max

------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Inkscape-devel mailing list
Inkscape-devel@...
https://lists.sourceforge.net/lists/listinfo/inkscape-devel

[Richard Hughes-2]

> attached is a fix for bug 402274. It's trivial and I cannot imagine it
> causing any problems but I'd be glad if someone could take a quick
> look anyway in case I overlooked something.

I'm not sure. I think that patch might re-order text in some cases,
although I haven't tried to come up with an example where it would.
Attached is a version that I think is more correct.

Richard.

------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Inkscape-devel mailing list
Inkscape-devel@...
https://lists.sourceforge.net/lists/listinfo/inkscape-devel

[Joshua A. Andler-2]

Krzysztof,

While I appreciate your willingness to review a patch, you do not have
the authority to commit, nor to approve other people committing so late
in a release cycle. This is purely the responsibility of release
wardens. Also note, a patch in which you said to commit was still
further fixed by Richard who happens to be more knowledgeable in the
area of our text handling. Oh, and I should have mentioned it when it
happened, but, when you recently removed a couple files from SVN, that
should have been handled by requesting myself to do so, not by you just
doing it.

I know that you haven't dealt with the releases before, but we do have
protocols. As you will notice, bulia, Jon Cruz, JazzyNico (translations)
and myself have done the majority of committing over the last couple
months and it is due to the protocols which have been in place for
years.

Cheers,
Josh

On Sat, 2009-10-03 at 13:16 +0200, Krzysztof Kosiński wrote:

> Yes, it was an obvious omission of a NULL check (in a way).
> I assume it compiles and fixes the bug, so go ahead and commit.
>
> Regards, Krzysztof
>
> ------------------------------------------------------------------------------
> Come build with us! The BlackBerry® Developer Conference in SF, CA
> is the only developer event you need to attend this year. Jumpstart your
> developing skills, take BlackBerry mobile applications to market and stay
> ahead of the curve. Join us from November 9-12, 2009. Register now!
> http://p.sf.net/sfu/devconf
> _______________________________________________
> Inkscape-devel mailing list
> Inkscape-devel@...
> https://lists.sourceforge.net/lists/listinfo/inkscape-devel

------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Inkscape-devel mailing list
Inkscape-devel@...
https://lists.sourceforge.net/lists/listinfo/inkscape-devel

[Maximilian Albert-2]

2009/10/3 Richard Hughes <cyreve@...>:
>> attached is a fix for bug 402274. It's trivial and I cannot imagine it
>> causing any problems but I'd be glad if someone could take a quick
>> look anyway in case I overlooked something.
>
> I'm not sure. I think that patch might re-order text in some cases,
> although I haven't tried to come up with an example where it would.
> Attached is a version that I think is more correct.

Yes, I had a hunch something like this might be the case, but I felt I
was too ignorant in this area to come up with a counterexample myself.
Thanks a lot for taking a look and for providing the corrected fix.

Max

------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Inkscape-devel mailing list
Inkscape-devel@...
https://lists.sourceforge.net/lists/listinfo/inkscape-devel

[Joshua A. Andler-2]

On Sat, 2009-10-03 at 12:43 +0100, Richard Hughes wrote:
> > attached is a fix for bug 402274. It's trivial and I cannot imagine it
> > causing any problems but I'd be glad if someone could take a quick
> > look anyway in case I overlooked something.
>
> I'm not sure. I think that patch might re-order text in some cases,
> although I haven't tried to come up with an example where it would.
> Attached is a version that I think is more correct.

Okay, it no longer crashes. However, if you select a portion of the text
in that example file from the report so that it crosses the different
styles and change font size or the font, it seems to strip the style
from the unselected black text. Any chance for a better fix?

Note, the above behavior is not the same if I create a new tspan and try
to reproduce it... so I do feel comfortable committing it, but if
there's hope for an even more improved one I would rather wait on that.

Cheers,
Josh


------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Inkscape-devel mailing list
Inkscape-devel@...
https://lists.sourceforge.net/lists/listinfo/inkscape-devel

[Richard Hughes-2]

> Okay, it no longer crashes. However, if you select a portion of the text
> in that example file from the report so that it crosses the different
> styles and change font size or the font, it seems to strip the style
> from the unselected black text. Any chance for a better fix?

I can't reproduce this. I am doing:

1) Open file
2) Double click the text with the red dashed rectangle around it
3) Select "O: MA"
4) Choose 40 from the size drop-down on the toolbar.

The result is exactly what I expect: the selected text gets bigger,
nothing else changes.

Richard.


------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Inkscape-devel mailing list
Inkscape-devel@...
https://lists.sourceforge.net/lists/listinfo/inkscape-devel

[Joshua A. Andler-2]

On Sat, 2009-10-03 at 22:13 +0100, Richard Hughes wrote:

> > Okay, it no longer crashes. However, if you select a portion of the text
> > in that example file from the report so that it crosses the different
> > styles and change font size or the font, it seems to strip the style
> > from the unselected black text. Any chance for a better fix?
>
> I can't reproduce this. I am doing:
>
> 1) Open file
> 2) Double click the text with the red dashed rectangle around it
> 3) Select "O: MA"
> 4) Choose 40 from the size drop-down on the toolbar.

Correct, I can't reproduce with that method, however, try this:

1) Open file
2) On the second line of text in the red box, select "RIA · MEL".
3) Change the font size

The black is then stripped from all of the black text on that line (even
the stuff not selected).

I can reproduce it every time here.

Cheers,
Josh


------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Inkscape-devel mailing list
Inkscape-devel@...
https://lists.sourceforge.net/lists/listinfo/inkscape-devel

[Richard Hughes-2]

> 1) Open file
> 2) On the second line of text in the red box, select "RIA · MEL".
> 3) Change the font size

> The black is then stripped from all of the black text on that line (even
> the stuff not selected).

Well, you've certainly opened up a can of worms here. Attached are
three independent patches:

bug_402274_v2.patch is a repost of the same file I attached to my
earlier e-mail, which fixes the crash

bug-402274-part-2.patch fixes the problem you spotted here where
altering the style of one bit of text would cause something else to
change as well

bug-402274-part-3.patch is another problem I spotted when testing
part 2 where some simplifications of the XML tree were missed

The first patch definitely needs to go in to this release. The second
part is certainly desirable, however I wonder whether it crosses the
threshold of seriousness for this late stage of the release, given
that the bug must have existed for ages and alters some code which is
used in just about every part of text styling (it did seem obviously
wrong to me, however). The third part is actually more important than
it sounds, because without it repeatedly re-styling overlapping bits
of text will create an ever-deepening XML tree until the limit of each
character being in its own tspan is reached.

My guess is that bits of code in the second and third parts used to
work because they mirrored an ordering bug either in the style parsing
or XML attribute parsing code. Does anybody remember fixing such a bug
in either of those areas, and if so, when?

Looks like it's time for the release wardens to make some hard
decisions. If you need any more information on the possible impact of
each patch, please ask.

Richard.



------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Inkscape-devel mailing list
Inkscape-devel@...
https://lists.sourceforge.net/lists/listinfo/inkscape-devel

[bulia byak]

The first patch is obvious, I tested and committed it. Now looking at
the others...

BTW: I think we should do a pre4 with all these late fixes coming in?

--
bulia byak
Inkscape. Draw Freely.
http://www.inkscape.org

------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Inkscape-devel mailing list
Inkscape-devel@...
https://lists.sourceforge.net/lists/listinfo/inkscape-devel

[Joshua A. Andler-2]

On Sun, 2009-10-04 at 15:58 +0100, Richard Hughes wrote:
> > 1) Open file
> > 2) On the second line of text in the red box, select "RIA · MEL".
> > 3) Change the font size
>
> > The black is then stripped from all of the black text on that line (even
> > the stuff not selected).
>
> Well, you've certainly opened up a can of worms here. Attached are
> three independent patches:

That's what I do. ;)

> Looks like it's time for the release wardens to make some hard
> decisions. If you need any more information on the possible impact of
> each patch, please ask.

I think that the patches read safe, but obviously the can I opened up
was on more of an edge case (at least that's how it appears). I'm not
sure that it's worth potentially breaking more so close to a release,
then again, what do you think Richard? What is the possible impact of
the 2nd and 3rd patches?

I will definitely commit the first patch for the release (one less
crash, yay!). If you don't think it's worth chancing so close to
release, the other 2 can just go in after trunk opens back up (obviously
safest option). However, if you feel that it really fixes a deeper issue
that we just weren't aware of and the risks aren't terribly high, I will
defer to you in this area. As mentioned before, I'm willing to do a .1
for 0.47 (just would prefer not to).

Cheers,
Josh


------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Inkscape-devel mailing list
Inkscape-devel@...
https://lists.sourceforge.net/lists/listinfo/inkscape-devel

[bulia byak]

On Sun, Oct 4, 2009 at 11:58 AM, Richard Hughes <cyreve@...> wrote:
> My guess is that bits of code in the second and third parts used to
> work because they mirrored an ordering bug either in the style parsing
> or XML attribute parsing code. Does anybody remember fixing such a bug
> in either of those areas, and if so, when?

I don't remember any changes in style reading order, at least not
recent. Looks like your patches 2 and 3 only change the order style is
read and written, right? So the worst that can happen is that some bit
of text will get a wrong style? If so I think it's safer to commit
them post-0.47.

--
bulia byak
Inkscape. Draw Freely.
http://www.inkscape.org

------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Inkscape-devel mailing list
Inkscape-devel@...
https://lists.sourceforge.net/lists/listinfo/inkscape-devel

[Richard Hughes-2]

> On Sun, Oct 4, 2009 at 11:58 AM, Richard Hughes <cyreve@...> wrote:
>> My guess is that bits of code in the second and third parts used to
>> work because they mirrored an ordering bug either in the style parsing
>> or XML attribute parsing code. Does anybody remember fixing such a bug
>> in either of those areas, and if so, when?

> I don't remember any changes in style reading order, at least not
> recent. Looks like your patches 2 and 3 only change the order style is
> read and written, right? So the worst that can happen is that some bit
> of text will get a wrong style?

Correct.

> If so I think it's safer to commit them post-0.47.

I agree. I think they go in to the bucket of 'annoying, but not worth
causing a delay in the release schedule for'. They might be annoying
enough for a .1 release.

R.


------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Inkscape-devel mailing list
Inkscape-devel@...
https://lists.sourceforge.net/lists/listinfo/inkscape-devel

[bulia byak]

On Mon, Oct 5, 2009 at 2:33 PM, Richard Hughes <cyreve@...> >>
If so I think it's safer to commit them post-0.47.
>
> I agree. I think they go in to the bucket of 'annoying, but not worth
> causing a delay in the release schedule for'. They might be annoying
> enough for a .1 release.

Great, just don't forget to commit them when we reopen :)

--
bulia byak
Inkscape. Draw Freely.
http://www.inkscape.org

------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Inkscape-devel mailing list
Inkscape-devel@...
https://lists.sourceforge.net/lists/listinfo/inkscape-devel