patches for the bughunt waiting for some testers
very trivial patches to test: https://bugs.launchpad.net/inkscape/+bug/1171109 comment #19 and https://bugs.launchpad.net/inkscape/+bug/1251951 comment #9
please test and provide feedback so I can check in the changes for the first one, and figure out what to do with the second one. Thanks
-- View this message in context: http://inkscape.13.x6.nabble.com/patches-for-the-bughunt-waiting-for-some-te... Sent from the Inkscape - Dev mailing list archive at Nabble.com.
On 14-1-2014 21:39, insaner wrote:
very trivial patches to test: https://bugs.launchpad.net/inkscape/+bug/1171109 comment #19
Thanks for your work. Could you fix these things first? - don't use tabs *ever* in our source code - there is no reason to use "gboolean" unless a function specifically wants it, and even then I would not to use it (in case of pointer or reference to a gboolean, then of course you _should_ use gboolean), so please use "bool" (perhaps rename "fix_ids" to "fix_clashing_ids") - no need to do "if (elem", as elem is guaranteed to be non-null there (see first line of function). It is confusing to repeat the check.
thanks, Johan
awesome, thanks for the quick feedback. I have made all the changes you suggested and posted the patch there. Out of curiosity, why no tabs?
also, can you take a look at the iconv problem? I really don't know about build tools enough to come up with a solution I can be confident in.
-- View this message in context: http://inkscape.13.x6.nabble.com/patches-for-the-bughunt-waiting-for-some-te... Sent from the Inkscape - Dev mailing list archive at Nabble.com.
On Tue, Jan 14, 2014 at 02:21:03PM -0800, insaner wrote:
awesome, thanks for the quick feedback. I have made all the changes you suggested and posted the patch there. Out of curiosity, why no tabs?
Per project coding style policy:
http://www.inkscape.org/en/develop/coding-style/
Bryce
also, can you take a look at the iconv problem? I really don't know about build tools enough to come up with a solution I can be confident in.
-- View this message in context: http://inkscape.13.x6.nabble.com/patches-for-the-bughunt-waiting-for-some-te... Sent from the Inkscape - Dev mailing list archive at Nabble.com.
CenturyLink Cloud: The Leader in Enterprise Cloud Services. Learn Why More Businesses Are Choosing CenturyLink Cloud For Critical Workloads, Development Environments & Everything In Between. Get a Quote or Start a Free Trial Today. http://pubads.g.doubleclick.net/gampad/clk?id=119420431&iu=/4140/ostg.cl... _______________________________________________ Inkscape-devel mailing list Inkscape-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/inkscape-devel
On 14-1-2014 23:21, insaner wrote:
awesome, thanks for the quick feedback. I have made all the changes you suggested and posted the patch there. Out of curiosity, why no tabs?
Tabs mess up the indentation, I think mostly because people use different tab sizes. (it's hard to use them properly so that they do not mess up) So, to solve that once and for all, we decided a long time ago to not to use any tabs.
Actually, there is some non-conforming indentation in your patch. Please indent with 4 spaces (not 8), and indent the body of an if-statement: if (a) { hoi; } It's nit-picking, and does not solve the bug. But it makes reading the same code in half a year from now much easier.
Something we should try to remember and fix some day: the new_id generating/finding code should be split-out into a new function; it's useful to have. Can you add "/// \todo" (or something doxygen eats) when you commit? Thanks!
also, can you take a look at the iconv problem? I really don't know about build tools enough to come up with a solution I can be confident in.
Hmm, not now, and tomorrow I have forgotten about it ;-)
cheers, Johan
ok, I changed them from 8 to 4 spaces, but I don't know exactly what you want me to add as a todo marker, I have no experience with doxygen. Let me know and I will check in today.
-- View this message in context: http://inkscape.13.x6.nabble.com/patches-for-the-bughunt-waiting-for-some-te... Sent from the Inkscape - Dev mailing list archive at Nabble.com.
On 17-1-2014 21:05, insaner wrote:
ok, I changed them from 8 to 4 spaces, but I don't know exactly what you want me to add as a todo marker, I have no experience with doxygen. Let me know and I will check in today.
I'm sorry, I forgot about this. Did you already commit it? The todo marker would be "\\ \todo bla bla", I think. Anything with "todo" in it will do really. ;-)
cheers, Johan
hi, could you take a look at the patch and test to make sure it's ok? I really want to commit and finally close this thing. Also, I think that better than adding a "todo" in the code, it might be more effective to file a bug and include a link to the comment where the patch was attached, that way it would be tracked right away, and the code would be immediately available; two birds with one stone for whoever would want to take that task on.
thanks again
-- View this message in context: http://inkscape.13.x6.nabble.com/patches-for-the-bughunt-waiting-for-some-te... Sent from the Inkscape - Dev mailing list archive at Nabble.com.
participants (3)
-
Bryce Harrington -
insaner -
Johan Engelen