Hi! I would like to introduce myself to the list and provide a patch for review. I'm novalis_dt on Sourceforge. I know Bryce from when he worked on the Worldforge project. I have recently started using Inkscape -- I'm not much of an artist, but I redid the front page of my website with Inkscape. I've also done a couple of diagrams for work (I do licensing for the Free Software Foundation). I've submitted a couple of other minor patches, and now have CVS commit access.
This patch provides a new command: "Vacuum unused defs." SVG files can accumulate cruft -- gradients, markers, etc, which aren't in use. I've chosen to make this command manual, because sometimes, you'll want to keep this cruft for later usage. But if you want smaller SVG files, for web usage (or secrecy reasons), you might try this feature.
When I wrote the first version of this patch, I didn't understand the SPObject tree. I think I do now. I'm somewhat baffled that there's no sp_object_get_child_by_tagName function already -- am I just missing it? If not, it probably belongs in sp-object.cpp, but I didn't want to put it there prematurely.
If I hear no objections, I'll commit it in a few days.
David Turner wrote:
When I wrote the first version of this patch, I didn't understand the SPObject tree. I think I do now. I'm somewhat baffled that there's no sp_object_get_child_by_tagName function already -- am I just missing it? If not, it probably belongs in sp-object.cpp, but I didn't want to put it there prematurely.
Just IMHO, but I would think that maybe both functions would belong in sp-object.* ? Since sp-objects are the internal nodes representing the graphical side of an SVG tree , while file.cpp is for a physical file on the disk.
Bob
But it is an excellent idea, though! This would be a good way for someone to clean up their file, just prior to publishing.
Ok, I have been playing too long , now... back to work.
Bob Jamison wrote:
David Turner wrote:
When I wrote the first version of this patch, I didn't understand the SPObject tree. I think I do now. I'm somewhat baffled that there's no sp_object_get_child_by_tagName function already -- am I just missing it? If not, it probably belongs in sp-object.cpp, but I didn't want to put it there prematurely.
Just IMHO, but I would think that maybe both functions would belong in sp-object.* ? Since sp-objects are the internal nodes representing the graphical side of an SVG tree , while file.cpp is for a physical file on the disk.
On Tue, 2004-08-03 at 13:32, Bob Jamison wrote:
David Turner wrote:
When I wrote the first version of this patch, I didn't understand the SPObject tree. I think I do now. I'm somewhat baffled that there's no sp_object_get_child_by_tagName function already -- am I just missing it? If not, it probably belongs in sp-object.cpp, but I didn't want to put it there prematurely.
Just IMHO, but I would think that maybe both functions would belong in sp-object.* ? Since sp-objects are the internal nodes representing the graphical side of an SVG tree , while file.cpp is for a physical file on the disk.
I thought file.cpp was for things that appear on the File menu. For instance, sp_export_png_file has nothing to do with the physical svg file.
On Tue, 2004-08-03 at 13:24, David Turner wrote:
This patch provides a new command: "Vacuum unused defs." SVG files can accumulate cruft -- gradients, markers, etc, which aren't in use. I've chosen to make this command manual, because sometimes, you'll want to keep this cruft for later usage. But if you want smaller SVG files, for web usage (or secrecy reasons), you might try this feature.
When I wrote the first version of this patch, I didn't understand the SPObject tree. I think I do now. I'm somewhat baffled that there's no sp_object_get_child_by_tagName function already -- am I just missing it?
No, it's never been needed. I don't actually think it helps in this case either; defs can appear anywhere in the document, not just as children of the root element (plus we may also want to cover certain non-def elements as well).
I think the best thing to do for a first implementation is just to use SP_ROOT(SP_DOCUMENT_ROOT(doc))->defs.
In general, tagName_get isn't necessary either, as you can already do sp_repr_name(SP_OBJECT_REPR(obj)).
In this specific case, I think checking the name is also asking the wrong question ... what you really want to ask is "should this object be collected if it has no SVG references?", not "does this XML name of the element this object is attached to start with 'inkscape:'?".
[ for example, some future inkscape: elements might be collectable, and most of the current ones actually start with sodipodi: anyway ]
I think what I want to do in future is add another value to the orphan collection policy enum, which can indicate this. But for right now I don't think you need to worry about checking it; we don't appear to put any "special" elements in <defs> currently.
If not, it probably belongs in sp-object.cpp, but I didn't want to put it there prematurely.
Yes, I would prefer that any new "methods" be added as real C++ methods. [ note: for SPObject and derivatives, methods cannot yet be virtual, as GObject doesn't cope with vtables, but ordinary methods are OK ]
Other than these things, the patch looks good -- I would just prefer a version that didn't introduce any new APIs.
-mental
On Tue, Aug 03, 2004 at 11:11:40PM -0400, MenTaLguY wrote:
Yes, I would prefer that any new "methods" be added as real C++ methods.
I'm not sure what is meant by `"methods"' above, but these days I tend to prefer non-member functions over member functions:
- Information-hiding principle suggests that if a function doesn't need access to private variables then it's better not to make it a member function.
- Member functions must be declared in the class definition, which tends to require more recompilation because one can't use a separate header file.
A related issue is that it's harder (impossible) to write private specializations: any specialization must be declared in the class definition too, so it isn't private other than through comments.
- The shorter names typical of member functions are harder to search for.
(Counter-argument: this is a matter of custom, there's no technical reason why one can't use long names for member functions, or even short names for non-member functions.)
pjrm.
On Wed, 2004-08-04 at 10:12, Peter Moulder wrote:
On Tue, Aug 03, 2004 at 11:11:40PM -0400, MenTaLguY wrote:
Yes, I would prefer that any new "methods" be added as real C++ methods.
I'm not sure what is meant by `"methods"' above,
I did mean member functions; I was feeling a bit illiterate last night. ^^;
but these days I tend to prefer non-member functions over member functions:
- Information-hiding principle suggests that if a function doesn't need access to private variables then it's better not to make it a member function.
Mixed feelings. I would mostly agree, though obviously polymorphism usually requires virtual member functions, which ought to be private or protected.
In some cases (e.g. "message sending") methods and certain types of mutatiors, I think a member function is also better for clarity.
In any case the specific functions under discussion were relatively simple accessors.
- Member functions must be declared in the class definition, which tends to require more recompilation because one can't use a separate header file.
IMO this should also be addressed by breaking large classes into smaller, simpler classes that do more specific things (and can get their own header files).
A related issue is that it's harder (impossible) to write private specializations: any specialization must be declared in the class definition too, so it isn't private other than through comments.
I'm not sure I follow ... are you referring to template specializations?
The shorter names typical of member functions are harder to search for.
(Counter-argument: this is a matter of custom, there's no technical reason why one can't use long names for member functions, or even short names for non-member functions.)
Whether member or no, I would encourage folks to spell words out and make names as long as necessary to avoid ambiguity (but no longer than that).
This brings up a related rule of thumb -- if it seems necessary to include a class' name in a function name, that function is probably better off being a member function (or at least split into member and non-member parts).
One additional disadvantage of member functions is that getting pointers to them is unweildy; the syntax for working with pointer-to-member-functionis awkward and the type itself can be very heavy-weight.
So I do prefer non-member functions on the basis that they are more easily composable via function pointers.
It might have been better for me to qualify my request as pertaining to specific kinds of functions (e.g. accessors), but I'm not quite sure exactly where the line lies. At this point it's a matter of taste that I've not managed to formalize well yet.
What I specifically don't want to see is more non-member functions like sp_object_get_whatever() or sp_object_set_whatever() introduced.
-mental
MenTaLguY wrote:
On Wed, 2004-08-04 at 10:12, Peter Moulder wrote:
On Tue, Aug 03, 2004 at 11:11:40PM -0400, MenTaLguY wrote:
Yes, I would prefer that any new "methods" be added as real C++ methods.
I'm not sure what is meant by `"methods"' above,
I did mean member functions; I was feeling a bit illiterate last night. ^^;
Same thing.
"method" is a more generic OO term for what are member functions in C++.
On Thu, 2004-08-05 at 00:15, Jon A. Cruz wrote:
Same thing.
"method" is a more generic OO term for what are member functions in C++.
Well, nearly. There are also static member functions which are member functions but not methods. For the purposes of the topic at hand static member functions are relevent too though.
-mental
Peter Moulder wrote:
On Tue, Aug 03, 2004 at 11:11:40PM -0400, MenTaLguY wrote:
Yes, I would prefer that any new "methods" be added as real C++ methods.
I'm not sure what is meant by `"methods"' above, but these days I tend to prefer non-member functions over member functions:
- Information-hiding principle suggests that if a function doesn't need access to private variables then it's better not to make it a member function.
Actually, I think it's sort of the other way.
Information-hiding says that if a function does need access to private variables, then needs to be a member function. However, the converse does not always follow.
Especially if the whole point is "hiding". :-)
That is, one should not even be able to tell if private or public variables are touched, so a paradigm where one can say "aha! This function is not a member function, therefore it does not touch private data. Aha! This function is a member function, therefore it does touch private data" does not seem to fall in line with information hiding.
- Member functions must be declared in the class definition, which tends to require more recompilation because one can't use a separate header file.
Suffer!!!!
:-)
Actually... just kidding. Compilation times should be considered, of course, but not as a main driving factor.
Code clarity, ease of maintenance and reduction of duplication should probably have a higher priority.
There are many ways to do things in C++, and I can even do Java-style inner classes. :-)
A related issue is that it's harder (impossible) to write private specializations: any specialization must be declared in the class definition too, so it isn't private other than through comments.
It depends.
If something truely needs private specializations, that can easily be done. However, if there is a generic way for things to be done, calling protected abstract member functions can be an ellegant solution.
The shorter names typical of member functions are harder to search for.
(Counter-argument: this is a matter of custom, there's no technical reason why one can't use long names for member functions, or even short names for non-member functions.)
Most modern tools compensate for this. If you look for all instances of "bar", a tool can know to look for "foo::bar" and not "baz::bar", etc.
On Tue, 3 Aug 2004, David Turner wrote:
Hi! I would like to introduce myself to the list and provide a patch for review. I'm novalis_dt on Sourceforge. I know Bryce from when he worked on the Worldforge project. I have recently started using Inkscape -- I'm not much of an artist, but I redid the front page of my website with Inkscape.
Welcome aboard Novalis, good to see you again. :-)
When I wrote the first version of this patch, I didn't understand the SPObject tree. I think I do now. I'm somewhat baffled that there's no sp_object_get_child_by_tagName function already -- am I just missing it? If not, it probably belongs in sp-object.cpp, but I didn't want to put it there prematurely.
By the way, one of the challenges for us has been that the codebase we started from was largely undocumented, so we try to encourage everyone to document (in Doxygen format) where they spot deficiencies. Sounds like you found one. ;-)
Bryce
participants (6)
-
Bob Jamison -
Bryce Harrington -
David Turner -
Jon A. Cruz -
MenTaLguY -
Peter Moulder