Thanks for the review Krzysztof.
Anybody confirm about first point? If is wrong with no manual XML modification... Second point. Any body can help me? Third point is bad because i like the strokes like the original, I dislike force the style to black and always filled. If is posibol to continue with it i need a better name...you are welcome.
Hi, Jabier.
El lun, 09-09-2013 a las 15:54 +0000, Krzysztof Kosinski escribió:
Review: Disapprove
- This feature is misleadingly named. It will apply the first stroke
shape encountered in the defs. As far as I know, there is no guarantee on the order of defs.
- The line "if (pattern == (const gchar *)"M 0,0 1,0") continue;" is
wrong - you can't test for pointer equality even when one of the strings is constant.
- Confusing control flow around the "shape_applied" variable. It
should default to false and be set to true inside the if.
2013/9/9 Jabiertxo Arraiza Cenoz <jabier.arraiza@...2893...>:
Thanks for the review Krzysztof.
Anybody confirm about first point? If is wrong with no manual XML modification...
It should be called "Last applied", but in this case it should be implemented in a significantly different way. You should store the last shape e.g. in a static variable whenever a shape is applied. Using the first effect in defs is not very useful.
Second point. Any body can help me?
Use strcmp().
Third point is bad because i like the strokes like the original, I dislike force the style to black and always filled. If is posibol to continue with it i need a better name...you are welcome.
Point 3 is not about what the code does, it's about how it is written. You should not set a variable to true, then reset it back to false when you don't find something.
Regards, Krzysztof
Thanks very much Krysztof, i rewrite it soon.
Jabier. El mié, 11-09-2013 a las 17:02 +0200, Krzysztof Kosiński escribió:
2013/9/9 Jabiertxo Arraiza Cenoz <jabier.arraiza@...2893...>:
Thanks for the review Krzysztof.
Anybody confirm about first point? If is wrong with no manual XML modification...
It should be called "Last applied", but in this case it should be implemented in a significantly different way. You should store the last shape e.g. in a static variable whenever a shape is applied. Using the first effect in defs is not very useful.
Second point. Any body can help me?
Use strcmp().
Third point is bad because i like the strokes like the original, I dislike force the style to black and always filled. If is posibol to continue with it i need a better name...you are welcome.
Point 3 is not about what the code does, it's about how it is written. You should not set a variable to true, then reset it back to false when you don't find something.
Regards, Krzysztof
Hi Krzysztof.
I made the changes you point to me. Its, ready now. Much less code! I only have a little problem with the name, i attemp to explain: I could do "Last clipboard applied" -what i choose now- it make a cleaner code. Or use "Last applied", for this, think need apply the ellipse, powerstroke and clipboard one. To do i need to insert the stroke type switch inside a while. But this give a poor benefits and think isnt necesary. Whats your opinion?
Thanks very much.
Jabier. El mié, 11-09-2013 a las 17:02 +0200, Krzysztof Kosiński escribió:
2013/9/9 Jabiertxo Arraiza Cenoz <jabier.arraiza@...2893...>:
Thanks for the review Krzysztof.
Anybody confirm about first point? If is wrong with no manual XML modification...
It should be called "Last applied", but in this case it should be implemented in a significantly different way. You should store the last shape e.g. in a static variable whenever a shape is applied. Using the first effect in defs is not very useful.
Second point. Any body can help me?
Use strcmp().
Third point is bad because i like the strokes like the original, I dislike force the style to black and always filled. If is posibol to continue with it i need a better name...you are welcome.
Point 3 is not about what the code does, it's about how it is written. You should not set a variable to true, then reset it back to false when you don't find something.
Regards, Krzysztof
------------------------------------------------------------------------------ How ServiceNow helps IT people transform IT departments: 1. Consolidate legacy IT systems to a single system of record for IT 2. Standardize and globalize service processes across IT 3. Implement zero-touch automation to replace manual, redundant tasks http://pubads.g.doubleclick.net/gampad/clk?id=51271111&iu=/4140/ostg.clk... _______________________________________________ Inkscape-devel mailing list Inkscape-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/inkscape-devel
Sorry i lost put the branch with the new name. https://code.launchpad.net/~jabiertxof/inkscape/lastClipboardApplied hi.
Jabier. El mié, 11-09-2013 a las 17:02 +0200, Krzysztof Kosiński escribió:
2013/9/9 Jabiertxo Arraiza Cenoz <jabier.arraiza@...2893...>:
Thanks for the review Krzysztof.
Anybody confirm about first point? If is wrong with no manual XML modification...
It should be called "Last applied", but in this case it should be implemented in a significantly different way. You should store the last shape e.g. in a static variable whenever a shape is applied. Using the first effect in defs is not very useful.
Second point. Any body can help me?
Use strcmp().
Third point is bad because i like the strokes like the original, I dislike force the style to black and always filled. If is posibol to continue with it i need a better name...you are welcome.
Point 3 is not about what the code does, it's about how it is written. You should not set a variable to true, then reset it back to false when you don't find something.
Regards, Krzysztof
------------------------------------------------------------------------------ How ServiceNow helps IT people transform IT departments: 1. Consolidate legacy IT systems to a single system of record for IT 2. Standardize and globalize service processes across IT 3. Implement zero-touch automation to replace manual, redundant tasks http://pubads.g.doubleclick.net/gampad/clk?id=51271111&iu=/4140/ostg.clk... _______________________________________________ Inkscape-devel mailing list Inkscape-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/inkscape-devel
participants (2)
-
Jabiertxo Arraiza Cenoz -
Krzysztof Kosiński