Merge lp://qastaging/~inkscape.dev/inkscape/bspline+spirolive into lp://qastaging/~inkscape.dev/inkscape/trunk

The edited svg should be saved using the inkscape compact options like the previous revision.

Actually I think this has already been merged, just a bit of fluff in the diff there.

Im sorry Martin, dont undertand well. the last revision diff includes all po files i start translating. It make a big diff. Finaly i decide focus in code and take the translation work after. And reload in branch the original po folder.
Focusing, im not sure about what i need to do with "inkscape compact options" or it is removed by "a bit of fluff".

Hi, and thanks for the review.

Look below. The only difference between your branch and trunk is one svg file. This svg file has been broken by opening it without the inkscape compact options and then saving it. Not a problem. But it suggests quite strongly that your code has already been merged.

This branch has _not_ been merged yet - there is no option to create bspline curves in trunk (pen, pencil tool), or to see spiro-curves live when drawing paths with the pen or pencil tool?

@Martin, scroll down to the end of the page, there's message telling this:
«The diff has been truncated for viewing. »
or download the diff and check yourself:

Diffstat:
 share/icons/icons.svg |26094 ++++++++++++++++++++++++++++-----
 share/icons/symbolic_icons.svg |23875 +++++++++++++++++++++++++-----
 share/icons/tango_icons.svg | 97
 src/draw-context.cpp | 35
 src/draw-context.h | 2
 src/libavoid/makefile | 17
 src/live_effects/CMakeLists.txt | 2
 src/live_effects/Makefile_insert | 2
 src/live_effects/effect-enum.h | 3
 src/live_effects/effect.cpp | 11
 src/live_effects/lpe-bspline.cpp | 647
 src/live_effects/lpe-bspline.h | 59
 src/pen-context.cpp | 1066 +
 src/pen-context.h | 5
 src/pencil-context.cpp | 40
 src/ui/tool/curve-drag-point.cpp | 20
 src/ui/tool/multi-path-manipulator.cpp | 13
 src/ui/tool/multi-path-manipulator.h | 2
 src/ui/tool/node.cpp | 179
 src/ui/tool/node.h | 9
 src/ui/tool/path-manipulator.cpp | 118
 src/ui/tool/path-manipulator.h | 17
 src/widgets/icon.cpp | 1
 src/widgets/mappings.xml | 1
 src/widgets/pencil-toolbar.cpp | 9
 src/widgets/toolbox.cpp | 2
 26 files changed, 44789 insertions(+), 7537 deletions(-)

@jabertox - trying to compile your branch 12091 fails for me (on OS X) with this error:

  CXX live_effects/lpe-bspline.o
In file included from ../../src/live_effects/lpe-bspline.cpp:20:
../../src/ui/tool/node-tool.h:78: error: ‘Inkscape::Selection’ has not been declared
make[3]: *** [live_effects/lpe-bspline.o] Error 1
make[2]: *** [all] Error 2
make[1]: *** [all-recursive] Error 1
make: *** [all] Error 2

> @jabertox - <…>

@Jabiertxo - sorry for mistyping your name!

Beat me to it ~suv (as usual though, you were much more thorough than I would have been)...

This definitely needs code review as well as UX review...

Krzysztof had mentioned a desire to implement B-Splines a while back but had some reservations about how to handle certain things. If I recall correctly he brought up issues such as what the controls for them look like, as well as how to handle connecting to adjacent segments of a path which aren't B-splines (straight segments, etc). Essentially, it wouldn't be an all or nothing approach, but B-Spline segments which could mix in with lines and regular curves on a path.

No problem ~suv!

El jue, 19-09-2013 a las 23:36 +0000, ~suv escribió:
> > @jabertox - <…>
>
> @Jabiertxo - sorry for mistyping your name!

Thanks ~suv, soon is fixed

El jue, 19-09-2013 a las 23:33 +0000, ~suv escribió:
> @jabertox - trying to compile your branch 12091 fails for me (on OS X) with this error:
>
> CXX live_effects/lpe-bspline.o
> In file included from ../../src/live_effects/lpe-bspline.cpp:20:
> ../../src/ui/tool/node-tool.h:78: error: ‘Inkscape::Selection’ has not been declared
> make[3]: *** [live_effects/lpe-bspline.o] Error 1
> make[2]: *** [all] Error 2
> make[1]: *** [all-recursive] Error 1
> make: *** [all] Error 2
>
>

Fixed

El vie, 20-09-2013 a las 00:06 +0000, Jabiertxo Arraiza Cenoz escribió:
> Thanks ~suv, soon is fixed
>
> El jue, 19-09-2013 a las 23:33 +0000, ~suv escribió:
> > @jabertox - trying to compile your branch 12091 fails for me (on OS X) with this error:
> >
> > CXX live_effects/lpe-bspline.o
> > In file included from ../../src/live_effects/lpe-bspline.cpp:20:
> > ../../src/ui/tool/node-tool.h:78: error: ‘Inkscape::Selection’ has not been declared
> > make[3]: *** [live_effects/lpe-bspline.o] Error 1
> > make[2]: *** [all] Error 2
> > make[1]: *** [all-recursive] Error 1
> > make: *** [all] Error 2
> >
> >
>
>
>

El jue, 19-09-2013 a las 23:39 +0000, ScislaC escribió:
> Beat me to it ~suv (as usual though, you were much more thorough than I would have been)...
>
> This definitely needs code review as well as UX review...
>
> Krzysztof had mentioned a desire to implement B-Splines a while back but had some reservations about how to

> handle certain things. If I recall correctly he brought up issues such as what the controls for them look like,

> as well as how to handle connecting to adjacent segments of a path which aren't B-splines (straight segments, etc).

> Essentially, it wouldn't be an all or nothing approach, but B-Spline segments which could mix in with lines and regular curves on a path.

Hello and yes ScislaC, it need a full review, i never touch C++ before.
I think is a problem have a path with mixed types, you always can make
straight lines pressing the shift key, but if is necesary two kinds of
curves in the same "line", maybe the live effect way isnt a solution and
we want to wait to SVG adopt BSplines. For me this isn`t a problem, is
like spiro ones, coulden`t be mixed with regular curves.

Another cuestion, whats UX review?

Hi and thanks for contribute.

UX is User Experience design... and it may not be applicable in this case given the way you went about implementing it. I guess I should have realized that this was handled via a live effect (hence the spiro related changes too). Spiro seems usable enough for people, so given that this is a similar effect it's seemingly a non-issue.

It's definitely less of an "incomplete" implementation than I was assuming.

Many apologies everyone.

Half my time is spent interpreting Launchpad and the other half committing to getting it wrong ;-)

Carry on everyone! ScislaC, let me know if you change your mind about the UX review.

El vie, 20-09-2013 a las 02:19 +0000, ScislaC escribió:
> UX is User Experience design... and it may not be applicable in this
> case given the way you went about implementing it. I guess I should
> have realized that this was handled via a live effect (hence the spiro
> related changes too). Spiro seems usable enough for people, so given
> that this is a similar effect it's seemingly a non-issue.
>
> It's definitely less of an "incomplete" implementation than I was
> assuming.

Ok, thanks for the new review.

El jue, 19-09-2013 a las 13:37 +0000, Martin Owens escribió:
> Review: Needs Fixing code review
>
> The edited svg should be saved using the inkscape compact options like the previous revision.

Hi, icons.svg saved whith trunk instead 0.48.3. Martin, think it fix.

On 2013-09-20 12:37 +0200, Jabiertxo Arraiza Cenoz wrote:
> El jue, 19-09-2013 a las 13:37 +0000, Martin Owens escribió:
>> Review: Needs Fixing code review
>>
>> The edited svg should be saved using the inkscape compact options like the previous revision.
>
> Hi, icons.svg saved whith trunk instead 0.48.3. Martin, think it fix.

@Jabiertxo - 'compact' refers to the Inkscape settings used to save the
icons.svg file (to keep the changes between revisions minimal, and the
size of the icons.svg file overall small):

<http://wiki.inkscape.org/wiki/index.php/HowToEditIcons>

El vie, 20-09-2013 a las 11:03 +0000, ~suv escribió:
> On 2013-09-20 12:37 +0200, Jabiertxo Arraiza Cenoz wrote:
> > El jue, 19-09-2013 a las 13:37 +0000, Martin Owens escribió:
> >> Review: Needs Fixing code review
> >>
> >> The edited svg should be saved using the inkscape compact options like the previous revision.
> >
> > Hi, icons.svg saved whith trunk instead 0.48.3. Martin, think it fix.
>
> @Jabiertxo - 'compact' refers to the Inkscape settings used to save the
> icons.svg file (to keep the changes between revisions minimal, and the
> size of the icons.svg file overall small):
>
> <http://wiki.inkscape.org/wiki/index.php/HowToEditIcons>
>
>
@~suv and @doctormo thanks and done!

On 2013-09-20 13:48 +0200, Jabiertxo Arraiza Cenoz wrote:
> El vie, 20-09-2013 a las 11:03 +0000, ~suv escribió:
>> On 2013-09-20 12:37 +0200, Jabiertxo Arraiza Cenoz wrote:
>>> El jue, 19-09-2013 a las 13:37 +0000, Martin Owens escribió:
>>>> Review: Needs Fixing code review
>>>>
>>>> The edited svg should be saved using the inkscape compact options like the previous revision.
>>>
>>> Hi, icons.svg saved whith trunk instead 0.48.3. Martin, think it fix.
>>
>> @Jabiertxo - 'compact' refers to the Inkscape settings used to save the
>> icons.svg file (to keep the changes between revisions minimal, and the
>> size of the icons.svg file overall small):
>>
>> <http://wiki.inkscape.org/wiki/index.php/HowToEditIcons>
>>
>>
> @~suv and @doctormo thanks and done!

hmm - looks like the last commit to 'share/icons/icons.svg' in trunk already broke with that rule and committed a version saved with different preferences:
<http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/revision/12533>

(@Jabiertxo - that's not your fault, this need to be fixed in trunk first AFAIU)

> hmm - looks like the last commit to 'share/icons/icons.svg' in trunk already broke with that rule and committed a version saved with different preferences:
<http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/revision/12533>

> (@Jabiertxo - that's not your fault, this need to be fixed in trunk first AFAIU)

Right, this is my fault.

Fixed in rev 12553.

r12098 fails to build:

  CXX ui/tool/path-manipulator.o
../../src/ui/tool/path-manipulator.cpp: In constructor ‘Inkscape::UI::PathManipulator::PathManipulator(Inkscape::UI::MultiPathManipulator&, SPPath*, const Geom::Affine&, guint32, Glib::ustring)’:
../../src/ui/tool/path-manipulator.cpp:155: error: ‘sp_lpe_item_has_path_effect’ was not declared in this scope
../../src/ui/tool/path-manipulator.cpp:156: error: ‘sp_lpe_item_has_path_effect_of_type’ was not declared in this scope
make[3]: *** [ui/tool/path-manipulator.o] Error 1
make[2]: *** [all] Error 2
make[1]: *** [all-recursive] Error 1
make: *** [all] Error 2

El jue, 26-09-2013 a las 10:30 +0000, ~suv escribió:
> r12098 fails to build:
>
> CXX ui/tool/path-manipulator.o
> ../../src/ui/tool/path-manipulator.cpp: In constructor ‘Inkscape::UI::PathManipulator::PathManipulator(Inkscape::UI::MultiPathManipulator&, SPPath*, const Geom::Affine&, guint32, Glib::ustring)’:
> ../../src/ui/tool/path-manipulator.cpp:155: error: ‘sp_lpe_item_has_path_effect’ was not declared in this scope
> ../../src/ui/tool/path-manipulator.cpp:156: error: ‘sp_lpe_item_has_path_effect_of_type’ was not declared in this scope
> make[3]: *** [ui/tool/path-manipulator.o] Error 1
> make[2]: *** [all] Error 2
> make[1]: *** [all-recursive] Error 1
> make: *** [all] Error 2
>
Thanks i know it im searching solutions, i think both functions
disappear from code.
Regards, Jabier.

On 2013-09-26 12:48 +0200, Jabiertxo Arraiza Cenoz wrote:
> El jue, 26-09-2013 a las 10:30 +0000, ~suv escribió:
>> r12098 fails to build:
>>
>> CXX ui/tool/path-manipulator.o
>> ../../src/ui/tool/path-manipulator.cpp: In constructor ‘Inkscape::UI::PathManipulator::PathManipulator(Inkscape::UI::MultiPathManipulator&, SPPath*, const Geom::Affine&, guint32, Glib::ustring)’:
>> ../../src/ui/tool/path-manipulator.cpp:155: error: ‘sp_lpe_item_has_path_effect’ was not declared in this scope
>> ../../src/ui/tool/path-manipulator.cpp:156: error: ‘sp_lpe_item_has_path_effect_of_type’ was not declared in this scope
>> make[3]: *** [ui/tool/path-manipulator.o] Error 1
>> make[2]: *** [all] Error 2
>> make[1]: *** [all-recursive] Error 1
>> make: *** [all] Error 2
>>
> Thanks i know it im searching solutions, i think both functions
> disappear from code.

r12098 compiled for me successfully with attached diff, and in a quick
test bspline-node-editing seems to work ok, as with earlier builds.

Please review (I don't speak C++ ;-) ).

Thanks ~suv for the help! Think today fix with your work.
Regards, Jabier.

El jue, 26-09-2013 a las 12:09 +0000, ~suv escribió:
> On 2013-09-26 12:48 +0200, Jabiertxo Arraiza Cenoz wrote:
> > El jue, 26-09-2013 a las 10:30 +0000, ~suv escribió:
> >> r12098 fails to build:
> >>
> >> CXX ui/tool/path-manipulator.o
> >> ../../src/ui/tool/path-manipulator.cpp: In constructor ‘Inkscape::UI::PathManipulator::PathManipulator(Inkscape::UI::MultiPathManipulator&, SPPath*, const Geom::Affine&, guint32, Glib::ustring)’:
> >> ../../src/ui/tool/path-manipulator.cpp:155: error: ‘sp_lpe_item_has_path_effect’ was not declared in this scope
> >> ../../src/ui/tool/path-manipulator.cpp:156: error: ‘sp_lpe_item_has_path_effect_of_type’ was not declared in this scope
> >> make[3]: *** [ui/tool/path-manipulator.o] Error 1
> >> make[2]: *** [all] Error 2
> >> make[1]: *** [all-recursive] Error 1
> >> make: *** [all] Error 2
> >>
> > Thanks i know it im searching solutions, i think both functions
> > disappear from code.
>
> r12098 compiled for me successfully with attached diff, and in a quick
> test bspline-node-editing seems to work ok, as with earlier builds.
>
> Please review (I don't speak C++ ;-) ).
>
>
>

Martin i request a change status of "SVG icons" for fix. I think its fixed.

Hi ~suv, I add to your code "if (SP_IS_LPE_ITEM(_path) && _path->hasPathEffect()){" Because a path editing crash in bend lpe.
Thanks again for your work.

El vie, 27-09-2013 a las 12:06 +0000, Martin Owens escribió:
> Review: Approve
>
>
Thanks.

Hey Jabiertxof,

How are you doing with this merge? Is it read or complete?

El jue, 03-10-2013 a las 20:22 +0000, Martin Owens escribió:
> Hey Jabiertxof,
>
> How are you doing with this merge? Is it read or complete?

Hi Martin!

Its working but its pending for a full review -first code in c++-, im
updating but only to follow the trunk.

Regards, Jabier.

There are a few things that could be improved, like the use of enum to represent widget mode (instead of plain and magical int numbers) and the use of new and delete (that we still need to keep, but hopefully will go away when we migrate to C++11), but none of this has anything to do with Jabier's changes. I think the branch can be merged already.

I discussed changes I thought worth through private emails with Jabier and they were already done.

Possibly some devs would like to see the use of SPCurve migrated to Geom::PathVector, but (1) I don't think it's important here and (2) looks like Geom::PathVector will receive breaking changes from KK soon.

Possibly other devs would like to see some asserts after some unchecked dynamic_cast, but I don't think this is important.

Any bugs related to this branch or has it become stable already?

Anyway, I'll move to another task within the Inkscape project.

It was a lot of code to review and took me some time.
=)

> It was a lot of code to review and took me some time.
> =)

Thanks very much for the review!

> It was a lot of code to review and took me some time.
> =)

Thanks very much for the review!