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
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
2009/10/3 Krzysztof Kosiński <tweenk.pl@...400...>:
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
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.
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@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/inkscape-devel
2009/10/3 Richard Hughes <cyreve@...400...>:
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
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
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.
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:
- Open file
- Double click the text with the red dashed rectangle around it
- Select "O: MA"
- 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
- Open file
- On the second line of text in the red box, select "RIA · MEL".
- 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.
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?
On Sun, 2009-10-04 at 15:58 +0100, Richard Hughes wrote:
- Open file
- On the second line of text in the red box, select "RIA · MEL".
- 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
On Sun, Oct 4, 2009 at 11:58 AM, Richard Hughes <cyreve@...400...> 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.
On Sun, Oct 4, 2009 at 11:58 AM, Richard Hughes <cyreve@...400...> 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.
On Mon, Oct 5, 2009 at 2:33 PM, Richard Hughes <cyreve@...400...> >> 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 :)
participants (5)
-
bulia byak -
Joshua A. Andler -
Krzysztof Kosiński -
Maximilian Albert -
Richard Hughes