Hey developers,
This is the model I'm going to implement for documents to reference other documents:
SPDocument { GSList *child_documents; SPDocument *parent_document;
// Parent of parents until parent == NULL; SPDocument *getRootDocument(); }
Then in uri-reference, check paths of existing child_documents, load the referenced document and add to child_documents. Set parent_document.
That should keep things consistant.
The idea here is that we'll use this to track documents referenced in <use tags> as well as <image tags> and attributes like 'marker-*: url(...);' but not for non-vector graphics used in the image tag.
Thoughts? Advice?
Best Regards, Martin Owens
2014/1/21 Martin Owens <doctormo@...400...>:
Hey developers,
This is the model I'm going to implement for documents to reference other documents:
SPDocument { GSList *child_documents; SPDocument *parent_document;
// Parent of parents until parent == NULL; SPDocument *getRootDocument(); }
1. What if we edit 2 documents in one instance of Inkscape, and both of those documents refer to a third? We should be able to avoid creating an extra copy of the SP tree in that case. On the other hand, it's not clear to me what would be the preferred behavior in the following scenario:
1. Open a.svg, which refers to common.svg 2. Modify common.svg 3. Open b.svg, which also refers to common.svg
Are we supposed to display different contents of common.svg in windows for a.svg and b.svg, reflecting their different contents at opening time, or should the displayed content of common.svg change when the file content is modified on disk?
My opinion is that all external links should be updated whenever the referred resource changes, with the exception of the currently edited document - there should be a notification that it changed on disk, but no automatic reloading. If the currently edited document is referred to in another window, then that window should change to reflect its current on-disk state.
2. Do not use GSList and GListin new code, ever. Not only are they type-unsafe, they are implemented as plain lists without sentinel nodes, so append is O(N) even on a doubly-linked list. For a much better alternative, see src/display/drawing-item.h - I used a Boost intrusive list to implement a tree hierarchy. Another option is boost::ptr_list, which is suitable if the children should be destroyed along with the parent.
3. If child_documents and parent_document are supposed to be private, they should be called _child_documents and _parent_document.
4. Inkscape should detect cyclic references and display an error image in the place of references back to the root document (e.g. the one being edited).
Regards, Krzysztof
On Tue, 2014-01-21 at 17:42 +0100, Krzysztof Kosiński wrote:
- What if we edit 2 documents in one instance of Inkscape
Then there will be two sp-trees. It's a very minor issue at this point.
Are we supposed to display different contents of common.svg in windows for a.svg and b.svg, reflecting their different contents at opening time.
For now, yes.
or should the displayed content of common.svg change when the file content is modified on disk?
I'd reserve that kind of disk monitoring to a part 4 or 5 of the functionality. We understand what not having that does and can caveat for now. But eventually even the root document should be updated if it's changed on disk. Similar to the way gedit warns users of the same.
- Do not use GSList and GListin new code, ever.
Ah man, I just got it working with GSList and boost looks like a dogs breakfast of spaghetti code. Do I /have/ to use boost or can std::list do the job too? (not example in the code of boost::ptr_list, very unsafe to try and implement)
- If child_documents and parent_document are supposed to be private,
they should be called _child_documents and _parent_document.
public.
- Inkscape should detect cyclic references and display an error image
in the place of references back to the root document (e.g. the one being edited).
Yep, got that covered already.
Thanks for your great response Krzysztof, that GSList complaint especially :-)
Martin,
2014/1/21 Martin Owens <doctormo@...400...>:
Ah man, I just got it working with GSList and boost looks like a dogs breakfast of spaghetti code. Do I /have/ to use boost or can std::list do the job too? (not example in the code of boost::ptr_list, very unsafe to try and implement)
I'm not sure what you mean by boost::ptr_list being unsafe.
boost::ptr_list<T> works almost exactly like a std::list<std::unique_ptr<T>>, but iterators dereference to object references.
boost::intrusive::list allows you to compute an iterator to the element from the element itself in constant time, so it is very useful if an object needs to remove itself from the list.
Regards, Krzysztof
On 21-1-2014 18:54, Krzysztof Kosiński wrote:
2014/1/21 Martin Owens <doctormo@...400...>:
Ah man, I just got it working with GSList and boost looks like a dogs breakfast of spaghetti code. Do I /have/ to use boost or can std::list do the job too? (not example in the code of boost::ptr_list, very unsafe to try and implement)
I'm not sure what you mean by boost::ptr_list being unsafe.
boost::ptr_list<T> works almost exactly like a std::list<std::unique_ptr<T>>, but iterators dereference to object references.
boost::intrusive::list allows you to compute an iterator to the element from the element itself in constant time, so it is very useful if an object needs to remove itself from the list.
How large do we expect this list to be? I advise list<T> for now (moving onto unique_ptr when we make the switch to C++11), just for the sake of simplicity and not unnecessarily relying on external deps. I think there is a much higher chance of a dev knowing the semantics of list<unique_ptr<T>> than boost::ptr_list<T>.
Please also document that the pointers in the list are owning pointers (i.e. the SPDoc owns its children and deletes them when it itself is deleted).
regards, Johan
On Tue, 2014-01-21 at 18:54 +0100, Krzysztof Kosiński wrote:
I'm not sure what you mean by boost::ptr_list being unsafe.
Just that I've not yet gotten my inkscape sponsored c++ education (book) and messing about with patterns that don't have existing examples in the codebase fills me with a kind of dread.
I mean no disrespect to boost::ptr_list
Martin,
On Tue, 2014-01-21 at 19:04 +0100, Johan Engelen wrote:
How large do we expect this list to be? I advise list<T> for now (moving onto unique_ptr when we make the switch to C++11), just for the sake of simplicity and not unnecessarily relying on external deps. I think there is a much higher chance of a dev knowing the semantics of list<unique_ptr<T>> than boost::ptr_list<T>.
I'm very close to reverting my code back to GSList, that just worked first time and now that I've turned it into a std::list<SPDocument *> it's a pile of poo. Just segfaulting because it's not initialized. But of course no examples in the code base exist of initializing std::list types. It's like every example code works on magic but my code hate me.
I'm at a loss for how this is supposed to work.
Any help?
Martin,
---- Martin Owens <doctormo@...400...> wrote:
On Tue, 2014-01-21 at 19:04 +0100, Johan Engelen wrote:
How large do we expect this list to be? I advise list<T> for now (moving onto unique_ptr when we make the switch to C++11), just for the sake of simplicity and not unnecessarily relying on external deps. I think there is a much higher chance of a dev knowing the semantics of list<unique_ptr<T>> than boost::ptr_list<T>.
I'm very close to reverting my code back to GSList, that just worked first time and now that I've turned it into a std::list<SPDocument *> it's a pile of poo. Just segfaulting because it's not initialized. But of course no examples in the code base exist of initializing std::list types. It's like every example code works on magic but my code hate me.
I'm at a loss for how this is supposed to work.
Any help?
Do you know www.cplusplus.org? I is a helpful reference, with small code examples. http://www.cplusplus.com/reference/list/list/push_front/
I'm online on IRC now.
-Johan
Could you show us some code? What do you mean by "not initialized"?
Markus
-----Ursprüngliche Nachricht----- Von: Martin Owens [mailto:doctormo@...400...] Gesendet: Dienstag, 21. Januar 2014 22:37 An: Johan Engelen Cc: inkscape-devel@lists.sourceforge.net Betreff: Re: [Inkscape-devel] Href sub-document model
On Tue, 2014-01-21 at 19:04 +0100, Johan Engelen wrote:
How large do we expect this list to be? I advise list<T> for now (moving onto unique_ptr when we make the switch to C++11), just for the sake of simplicity and not unnecessarily relying on external deps. I think there is a much higher chance of a dev knowing the semantics of list<unique_ptr<T>> than
boost::ptr_list<T>.
I'm very close to reverting my code back to GSList, that just worked first time and now that I've turned it into a std::list<SPDocument *> it's a pile of poo. Just segfaulting because it's not initialized. But of course no examples in the code base exist of initializing std::list types. It's like every example code works on magic but my code hate me.
I'm at a loss for how this is supposed to work.
Any help?
Martin,
---------------------------------------------------------------------------- -- 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
2014/1/21 Johan Engelen <jbc.engelen@...2592...>:
How large do we expect this list to be? I advise list<T> for now (moving onto unique_ptr when we make the switch to C++11), just for the sake of simplicity and not unnecessarily relying on external deps. I think there is a much higher chance of a dev knowing the semantics of list<unique_ptr<T>> than boost::ptr_list<T>.
Boost include-only libraries are already a dependency, even for 2Geom.
The overhead of learning boost::ptr_list when you already know how to use std::list is minimal compared to the overhead of writing boilerplate destructor code on every object that contains an std::list of pointers.
std::unique_ptr is only available in C++11 so we cannot use it yet, and std::auto_ptr cannot be put into containers.
Here are some examples.
std::ptr_list<Object> list, list2 list.push_back(new Object(1, 2, 3)); list.push_back(new DerivedObject()); list.push_back(new Object(3, 2, 1)); list.erase(list.begin()); // calls desctructor std::auto_ptr<Object> x = list.release(list.begin()); // if you want to remove an object without destroying it
Regards, Krzysztof
Thanks for the push Krzysztof,
I got ptr_list working I think the way you wanted, have a look at r12969 and see if it's the way you imagined.
Best Regards, Martin Owens
On Wed, 2014-01-22 at 01:20 +0100, Krzysztof Kosiński wrote:
2014/1/21 Johan Engelen <jbc.engelen@...2592...>:
How large do we expect this list to be? I advise list<T> for now (moving onto unique_ptr when we make the switch to C++11), just for the sake of simplicity and not unnecessarily relying on external deps. I think there is a much higher chance of a dev knowing the semantics of list<unique_ptr<T>> than boost::ptr_list<T>.
Boost include-only libraries are already a dependency, even for 2Geom.
The overhead of learning boost::ptr_list when you already know how to use std::list is minimal compared to the overhead of writing boilerplate destructor code on every object that contains an std::list of pointers.
std::unique_ptr is only available in C++11 so we cannot use it yet, and std::auto_ptr cannot be put into containers.
Here are some examples.
std::ptr_list<Object> list, list2 list.push_back(new Object(1, 2, 3)); list.push_back(new DerivedObject()); list.push_back(new Object(3, 2, 1)); list.erase(list.begin()); // calls desctructor std::auto_ptr<Object> x = list.release(list.begin()); // if you want to remove an object without destroying it
Regards, Krzysztof
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
Hi Martin, Could you make the list and parent pointer private to SPDoc? And then add the const method SPDoc::find...() that returns NULL if nothing was found, and a SPDoc::linkChild(SPDoc *child) { child_documents.push_back(child); child->parent_document = this; }
(hope you can come up with better names for the methods)
Thanks, Johan
On 22-1-2014 4:21, Martin Owens wrote:
Thanks for the push Krzysztof,
I got ptr_list working I think the way you wanted, have a look at r12969 and see if it's the way you imagined.
Best Regards, Martin Owens
On Wed, 2014-01-22 at 01:20 +0100, Krzysztof Kosiński wrote:
2014/1/21 Johan Engelen <jbc.engelen@...2592...>:
How large do we expect this list to be? I advise list<T> for now (moving onto unique_ptr when we make the switch to C++11), just for the sake of simplicity and not unnecessarily relying on external deps. I think there is a much higher chance of a dev knowing the semantics of list<unique_ptr<T>> than boost::ptr_list<T>.
Boost include-only libraries are already a dependency, even for 2Geom.
The overhead of learning boost::ptr_list when you already know how to use std::list is minimal compared to the overhead of writing boilerplate destructor code on every object that contains an std::list of pointers.
std::unique_ptr is only available in C++11 so we cannot use it yet, and std::auto_ptr cannot be put into containers.
Here are some examples.
std::ptr_list<Object> list, list2 list.push_back(new Object(1, 2, 3)); list.push_back(new DerivedObject()); list.push_back(new Object(3, 2, 1)); list.erase(list.begin()); // calls desctructor std::auto_ptr<Object> x = list.release(list.begin()); // if you want to remove an object without destroying it
Regards, Krzysztof
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
Thanks Johan and Krzysztof for your help with this.
On Wed, 2014-01-22 at 18:43 +0100, Johan Engelen wrote:
Could you make the list and parent pointer private to SPDoc? And then add the const method SPDoc::find...() that returns NULL if nothing was found.
To do the infinite-loop prevention I had to move child->parent_document line into SPDocument::createDoc anyway. So those two lines happen when one specifies a parent when creating a document.
I've also moved the bulk of the code to document.cpp under a new createChildDoc function which we can use from image.cpp to load those new viewports I'm fancying. But also to make the two new variables private.
I decided to change xml/repr-io's warning about missing files, which I hit when I use'd a file that didn't exist; so from:** (inkscape:30541):
CRITICAL **: Inkscape::XML::Document* sp_repr_read_file(const gchar*, const gchar*): assertion `Inkscape::IO::file_test( filename, G_FILE_TEST_EXISTS )' failed CRITICAL **: void Inkscape::URIReference::attach(const Inkscape::URI&): assertion `document != NULL' failed
to:
WARNING **: Can't open file: /tmp/use/dead.svg (doesn't exist) WARNING **: Can't get document for referenced URI: dead.svg#grave
Which is much cleaner but I hope no one minds the change since it'll effect normal opens too. Now we have protection against infinite loops, dead links and specifying ones own document's filename.
Martin,
On Jan 21, 2014, at 10:04 AM, Johan Engelen wrote:
How large do we expect this list to be? I advise list<T> for now (moving onto unique_ptr when we make the switch to C++11), just for the sake of simplicity and not unnecessarily relying on external deps. I think there is a much higher chance of a dev knowing the semantics of list<unique_ptr<T>> than boost::ptr_list<T>.
Please also document that the pointers in the list are owning pointers (i.e. the SPDoc owns its children and deletes them when it itself is deleted).
Hi guys. Sorry that I hit this a bit late.
However, there is one very important thing here. For a container, std::list<> and the ilk should be the last choice, and only after management and performance have been carefully worked out. Instead, one should first use std::vector<>
Unless performance actually is measured to be significantly different, std::vector<> should be the 'go to' solution. People often think they have a case where a list is better, but once you start to count up times iterating the list vs times modifying it, std::vector<> is almost always better in practice.
On Jan 21, 2014, at 7:21 PM, Martin Owens wrote:
Thanks for the push Krzysztof,
I got ptr_list working I think the way you wanted, have a look at r12969 and see if it's the way you imagined.
BTW, in general a safer approach could be to use boost::shared_ptr<> / std::shared_ptr<>.
Of course, as with any "smart" pointer we developers have to remember that they are easy to misuse and could get out of hand or easily not do what you expect. In this specific case, we probably just need to avoid setting up cyclic loops (i.e. http://en.wikipedia.org/wiki/Reference_counting#Dealing_with_reference_cycle... ).
Using this type we'd get for a member declaration:
std::vector<boost::shared_ptr<MyType> > _children;
And to add one,
_children.push_back(boost::make_shared<MyType>(/* params for MyType's ctor*/));
These also have gotten into c++11, so if we want to later on we can add a little autoconf magic to have the proper one brought in at build time.
One of the biggest advantages of shared_ptr<> is that you won't get dangling pointers passed around that point to objects that have already been deleted. Boost::ptr_list does not provide this same safety.
On 28-2-2014 20:25, Jon Cruz wrote:
On Jan 21, 2014, at 10:04 AM, Johan Engelen wrote:
How large do we expect this list to be? I advise list<T> for now (moving onto unique_ptr when we make the switch to C++11), just for the sake of simplicity and not unnecessarily relying on external deps. I think there is a much higher chance of a dev knowing the semantics of list<unique_ptr<T>> than boost::ptr_list<T>.
Please also document that the pointers in the list are owning pointers (i.e. the SPDoc owns its children and deletes them when it itself is deleted).
Hi guys. Sorry that I hit this a bit late.
However, there is one very important thing here. For a container, std::list<> and the ilk should be the last choice, and only after management and performance have been carefully worked out. Instead, one should first use std::vector<>
Unless performance actually is measured to be significantly different, std::vector<> should be the 'go to' solution. People often think they have a case where a list is better, but once you start to count up times iterating the list vs times modifying it, std::vector<> is almost always better in practice.
Good point. We were focussed too much on choosing between GSList, boost::ptr_list, and std::list.
-Johan
On 28-2-2014 21:04, Jon Cruz wrote:
These also have gotten into c++11, so if we want to later on we can add a little autoconf magic to have the proper one brought in at build time.
Brief comment: let's not add cross-platform difficulties. We just flip the switch and require C++11.
:) Johan
On Feb 28, 2014, at 12:09 PM, Johan Engelen wrote:
On 28-2-2014 21:04, Jon Cruz wrote:
These also have gotten into c++11, so if we want to later on we can add a little autoconf magic to have the proper one brought in at build time.
Brief comment: let's not add cross-platform difficulties. We just flip the switch and require C++11.
Can't. :-P
We'll have people dependent on older versions of gcc for quite some time. However, C++11 is getting to be the default for more people.
Given that my main computer can't run anything newer than OSX 10.6, I'll be stuck in the no-C++11 camp for at least a few more years.
Given that my main computer can't run anything newer than OSX 10.6, I'll
be stuck in the no-C++11 camp for at least a few more years. Then why don't you simply use mac-ports? :) This works on Mac OS 10.6: http://wiki.inkscape.org/wiki/index.php/CompilingMacOsX#Compiling_with_clang _3.3_and_libc.2B.2B_.28c.2B.2B11_enabled.29_.282013.29
Regards, Markus
-----Ursprüngliche Nachricht----- Von: Jon Cruz [mailto:jon@...18...] Gesendet: Freitag, 28. Februar 2014 21:30 An: Johan Engelen Cc: inkscape-devel@lists.sourceforge.net Betreff: Re: [Inkscape-devel] Href sub-document model
On Feb 28, 2014, at 12:09 PM, Johan Engelen wrote:
On 28-2-2014 21:04, Jon Cruz wrote:
These also have gotten into c++11, so if we want to later on we can add a little autoconf magic to have the proper one brought in at build time.
Brief comment: let's not add cross-platform difficulties. We just flip the switch and require C++11.
Can't. :-P
We'll have people dependent on older versions of gcc for quite some time. However, C++11 is getting to be the default for more people.
Given that my main computer can't run anything newer than OSX 10.6, I'll be stuck in the no-C++11 camp for at least a few more years.
---------------------------------------------------------------------------- -- Flow-based real-time traffic analytics software. Cisco certified tool. Monitor traffic, SLAs, QoS, Medianet, WAAS etc. with NetFlow Analyzer Customize your own dashboards, set traffic alerts and generate reports. Network behavioral analysis & security monitoring. All-in-one tool. http://pubads.g.doubleclick.net/gampad/clk?id=126839071&iu=/4140/ostg.cl... _______________________________________________ Inkscape-devel mailing list Inkscape-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/inkscape-devel
On Feb 28, 2014, at 1:19 PM, Markus Engel wrote:
Given that my main computer can't run anything newer than OSX 10.6, I'll
be stuck in the no-C++11 camp for at least a few more years. Then why don't you simply use mac-ports? :) This works on Mac OS 10.6: http://wiki.inkscape.org/wiki/index.php/CompilingMacOsX#Compiling_with_clang _3.3_and_libc.2B.2B_.28c.2B.2B11_enabled.29_.282013.29
Yes...
However I've been down this road before.
:-)
To stay 100% stable (keep other builds from breaking, etc.), I have to keep with the OS's stock tools.
On Fri, 2014-02-28 at 11:25 -0800, Jon Cruz wrote:
Unless performance actually is measured to be significantly different, std::vector<> should be the 'go to' solution. People often think they have a case where a list is better, but once you start to count up times iterating the list vs times modifying it, std::vector<> is almost always better in practice.
This list is a low write, low read, low iteration, low count list. Your description doesn't say what the specific advantages are so it's hard to be sure what you're recommending.
Although you are right that a document maybe referenced by more than one href in the same document; although not in multiple opened documents because of where the list is.
At least for now I don't believe we're going to suffer any performance problems.
Martin,
Simply always use std::vector, it's *the* default container. Only change away from it if there are really good reasons...
-----Ursprüngliche Nachricht----- Von: Martin Owens [mailto:doctormo@...400...] Gesendet: Freitag, 28. Februar 2014 23:17 An: Jon Cruz Cc: Inkscape Devel List Betreff: Re: [Inkscape-devel] Href sub-document model
On Fri, 2014-02-28 at 11:25 -0800, Jon Cruz wrote:
Unless performance actually is measured to be significantly different, std::vector<> should be the 'go to' solution. People often think they have a case where a list is better, but once you start to count up times iterating the list vs times modifying it, std::vector<> is almost always better in practice.
This list is a low write, low read, low iteration, low count list. Your description doesn't say what the specific advantages are so it's hard to be sure what you're recommending.
Although you are right that a document maybe referenced by more than one href in the same document; although not in multiple opened documents because of where the list is.
At least for now I don't believe we're going to suffer any performance problems.
Martin,
---------------------------------------------------------------------------- -- Flow-based real-time traffic analytics software. Cisco certified tool. Monitor traffic, SLAs, QoS, Medianet, WAAS etc. with NetFlow Analyzer Customize your own dashboards, set traffic alerts and generate reports. Network behavioral analysis & security monitoring. All-in-one tool. http://pubads.g.doubleclick.net/gampad/clk?id=126839071&iu=/4140/ostg.cl... _______________________________________________ Inkscape-devel mailing list Inkscape-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/inkscape-devel
On Feb 28, 2014, at 2:16 PM, Martin Owens wrote:
On Fri, 2014-02-28 at 11:25 -0800, Jon Cruz wrote:
Unless performance actually is measured to be significantly different, std::vector<> should be the 'go to' solution. People often think they have a case where a list is better, but once you start to count up times iterating the list vs times modifying it, std::vector<> is almost always better in practice.
This list is a low write, low read, low iteration, low count list. Your description doesn't say what the specific advantages are so it's hard to be sure what you're recommending.
Although you are right that a document maybe referenced by more than one href in the same document; although not in multiple opened documents because of where the list is.
At least for now I don't believe we're going to suffer any performance problems.
The key point is that unless we have something high traffic for modifies but not iteration nor random access, then we should use vector over list. Sans performance issues, vector should be preferred.
In other words, for C++ coding "use a list" means "implement via std::vector"
participants (6)
-
unknown＠example.com -
Johan Engelen -
Jon Cruz -
Krzysztof Kosiński -
Markus Engel -
Martin Owens