Before we can think about a release, one thing we absolutely must do is clean up the current mess with dialogs. Everyone who did dialogs recently (and there were a lot) did it in their own incompatible way. (Personally, I attribute this to the lack of attention and leadership from the initiators of the gtkmm transition, but there's no use to argue about that now; let's just fix what we have.)
Recently I've been working on the Dialog and DialogManager classes in ui/dialog, and I almost fixed them with regard to things like transientizing, remembering geometry, and F12 behavior. There are remaining issues still but I'm pretty sure I'll be able to fix them.
Unfortunately, many dialogs do not use these classes or use them in a wrong way. The old gtk dialogs need not be touched at this time; they are OK as they are, and we can always convert them later. What needs to be fixed urgently is the recenty added gtkmm dialogs, all of which are broken in different ways.
The simplest dialog to use as a template is ui/dialog/messages.cpp; another is ui/dialog/memory.cpp. To create a dialog, you need to pass the prefs path (path in the preferences.xml for the dialog settings; make sure it exists in preferences-skeleton.h) and the verb that creates the dialog (make sure it exists in verbs.h/cpp). E.g.:
Messages::Messages() : Dialog ("dialogs.messages", SP_VERB_DIALOG_DEBUG)
You also need to register the dialog's factory in DIalogManager constructor and to make sure its verb in verbs.cpp does the right thing:
case SP_VERB_DIALOG_DEBUG: dt->_dlg_mgr->showDialog("Messages"); break;
Also don't forget to remove your own F12 handlers, event handles, transientizing code, title setting, size setting etc. if you have it in your dialog classes. This is all in Dialog now. Not only will this stuff finally work as it must, but this will lead to a significant reducing of code.
Here are the dialogs that need to be converted:
Align: Unless Aubanel Monnier (who coded it initially) shows up, I think I'll have to take this one.
View/Scripts and Path/Trace Bitmap: Bob, these are yours, so please take them. This needs to be done ASAP because currently these two are totally disfunctional - they don't show up when you call them. (This may be because they used ui/dialog code in some partial way and this is now broken after my changes there.)
Object/Grid Arrange: John, this one is yours, please convert it - also ASAP because it's broken in the same way as Bob's ones.
Object/Swatches and Help/About Extensions: Jon, these are yours. I know you did your own implementation but we now need to unify things. You can move whatever useful code you have into Dialog, but _that_ must be the root class of all dialogs now. No more duplication please. This will also fix the many problems your dialogs now have (such as popping up after being closed when any new dialog is opened).
Jon, Bob, John: Please respond to this message with your estimate as to when you will be able to work on this. If you cannot handle this now or in foreseeable future, we'll need to call up volunteers for this work. Of course, please also voice any questions or objections you might have.
Oh, and if anyone creates a new dialog now, please use ui/dialog/messages.cpp as a template. Don't copy from any other dialogs until they are fixed.
On 5/24/05, bulia byak <buliabyak@...400...> wrote:
Jon, Bob, John: Please respond to this message with your estimate as to when you will be able to work on this. If you cannot handle this now or in foreseeable future, we'll need to call up volunteers for this work. Of course, please also voice any questions or objections you might have.
I've done the tiledialog, and commited the changes.
cheers
Sim
On May 24, 2005, at 1:49 PM, bulia byak wrote:
Object/Swatches and Help/About Extensions: Jon, these are yours. I know you did your own implementation but we now need to unify things. You can move whatever useful code you have into Dialog, but _that_ must be the root class of all dialogs now. No more duplication please. This will also fix the many problems your dialogs now have (such as popping up after being closed when any new dialog is opened).
Yup. Definitely what needs to be done, as my dialog stuff was merely placeholder.
I'm wrestling with getting a dev environment going on the new box, so once that's set I should be able to knock that stuff out quickly.
bulia byak wrote:
View/Scripts and Path/Trace Bitmap: Bob, these are yours, so please take them. This needs to be done ASAP because currently these two are totally disfunctional - they don't show up when you call them. (This may be because they used ui/dialog code in some partial way and this is now broken after my changes there.)
I'll put this on my schedule. ;-)
Jon, Bob, John: Please respond to this message with your estimate as to when you will be able to work on this. If you cannot handle this now or in foreseeable future, we'll need to call up volunteers for this work. Of course, please also voice any questions or objections you might have.
Hmmm... Looking at my calendar, this could take as long as 3 weeks.
Oh wait!
It took 60 minutes. ^^ Done and committed. ;-)
Bob
On 5/25/05, Bob Jamison <rwjj@...127...> wrote:
It took 60 minutes. ^^ Done and committed. ;-)
Thanks :) Except that you forgot to add dialogs.trace to preferences-skeleton.h (I fixed that).
On Tue, May 24, 2005 at 05:49:09PM -0300, bulia byak wrote:
Before we can think about a release, one thing we absolutely must do is clean up the current mess with dialogs. Everyone who did dialogs recently (and there were a lot) did it in their own incompatible way.
I'm glad this is getting attention.
(Personally, I attribute this to the lack of attention and leadership from the initiators of the gtkmm transition, but there's no use to argue about that now; let's just fix what we have.)
No, much of the mess is due to work done before the gtkmm transition was really initiated. The dialogs that were transitioned into src/ui/dialog/ are all fairly consistent. The one exception being the Preferences dialog, however there had been discussions of redesigning it anyway.
It is true that the last two weeks I have not been putting attention into it, but I think you will find that due to the months I had put into it leading up to this, the project is in a much better position to establish a full solution than it ever was before.
The cause of the shift in attention is probably now obvious to those who follow slashdot. My company had a round of layoffs two weeks ago, and while I wasn't affected, I had to reprioritize my commitments. Anyway, I'm sorry this happened, and apologize if my lack of attention has forced people to scramble. However, the good news is that finally more people are getting seriously involved in this work.
Recently I've been working on the Dialog and DialogManager classes in ui/dialog, and I almost fixed them with regard to things like transientizing, remembering geometry, and F12 behavior. There are remaining issues still but I'm pretty sure I'll be able to fix them.
Thanks, I'm glad you took care of completing this. I had never really understood how all this stuff should work, and had been hoping you could put some effort into it. I hope that you found that with the common Dialog class, it means that this will solve the problem in a permanent way, instead of re-implementing the transient code for each new dialog.
Unfortunately, many dialogs do not use these classes or use them in a wrong way. The old gtk dialogs need not be touched at this time; they are OK as they are, and we can always convert them later. What needs to be fixed urgently is the recenty added gtkmm dialogs, all of which are broken in different ways.
The simplest dialog to use as a template is ui/dialog/messages.cpp; another is ui/dialog/memory.cpp. To create a dialog, you need to pass the prefs path (path in the preferences.xml for the dialog settings; make sure it exists in preferences-skeleton.h) and the verb that creates the dialog (make sure it exists in verbs.h/cpp). E.g.:
Messages::Messages() : Dialog ("dialogs.messages", SP_VERB_DIALOG_DEBUG)
You also need to register the dialog's factory in DIalogManager constructor and to make sure its verb in verbs.cpp does the right thing:
switch (reinterpret_caststd::size_t(data)) { ... case SP_VERB_DIALOG_DEBUG: dt->_dlg_mgr->showDialog("Messages"); break;
Btw, this is intended to only be an interim solution. Ultimately, I would like to see a mechanism that allows the dialogs to be registered in a way that would allow invoking them without passing a string. I.e., instead of a switch statement with cases to handle each verb define as above, you could replace the entire switch statement with just:
dt->_dlg_mgt->showDialog(reinterpret_caststd::size_t(data));
The advantage to this is more than just saving lines of code, though. By doing dialog invokation entirely dynamically, it will enable dynamic addition of dialogs to Inkscape at run time. This in turn means that Ted's dynamically loaded extensions will be able to add their dialogs to Inkscape when they're loaded. My hope is that by allowing extensions to be better decoupled from the core codebase, it will give us better ways of solving extension dependency issues more flexibly.
Also don't forget to remove your own F12 handlers, event handles, transientizing code, title setting, size setting etc. if you have it in your dialog classes. This is all in Dialog now. Not only will this stuff finally work as it must, but this will lead to a significant reducing of code.
Btw, if anyone has a better name for the F12 handler than "f12_handler", PLEASE feel free to rename it. ;-)
Also, I notice that in some cases people are using the 'Impl Trick' such as is shown in memory.*. I had demonstrated this trick a few months ago, and I still feel it is an eligant way to solve header inclusion issues, but the design of Dialog::Manager makes this completely unnecessary. Notice in verbs.cpp and other places that use dialogs, that with the new design the verb handler doesn't need to actually "know" what the dialog is, but just calls routines in the parent Dialog class. This means that the individual dialog headers do NOT need to be included in verbs.cpp or any other places that launch dialogs; thus, the impact of having the private gtkmm widget members defined in the .h is not propagated beyond Dialog::Manager.
Of course, there's nothing *wrong* with using the Impl trick, but I found that it conceptually simplifies the dialog code to not use it, which I feel means the dialog code will be easier for others to understand and maintain.
Thus, I would encourage using the messages.* dialog as the template to follow. This is Ishmal's old Debug dialog, that I converted into the new format. Note that I renamed it from Debug to Messages in order to keep it consistent with what it is called in the Inkscape menus. I notice that in the old dialogs there are major discrepancies between what the dialog is called within the application and what it is called in the code; this leads to unnecessary confusion. If you wish to rename the dialog inside the application, please also rename the file, so when people need to debug a dialog's behavior, it will be extremely obvious which file it is. ;-)
Also, there is a naming convention with the new dialog design, that jonadab and I tried to adhere to. First, do not include 'Dialog' in the dialog's name; it's redundant, and since the class is in the "Inkscape::UI::Dialog::" namespace already, it is unnecessary and just makes the name longer. ;-) Second, use '-' to separate words, so it should be 'clone-tiler.*' not 'clonetiler.*'. Third, make the name as close to the in-Inkscape name, as mentioned above.
Oh, and be attentive to how the name of the dialog shows up in various places. Remember that in Inkscape it shows up in the verb (i.e., the menu and statusbar) as well as in the title of the dialog itself. In some cases in the old dialogs I noticed this got to be inconsistent; it will look a lot better to users if it is consistent throughout.
Bryce
----- End forwarded message -----
On Wed, 2005-05-25 at 01:23, Bryce Harrington wrote:
Btw, this is intended to only be an interim solution. Ultimately, I would like to see a mechanism that allows the dialogs to be registered in a way that would allow invoking them without passing a string. I.e., instead of a switch statement with cases to handle each verb define as above, you could replace the entire switch statement with just:
dt->_dlg_mgt->showDialog(reinterpret_cast<std::size_t>(data));
...
Hmm. I'm not overly fond of this approach.
It seems to be refactoring the wrong pieces to me. I'd like to stick with strings or GQuarks to identify dialogs. There's no reason to tie dialogs to verbs directly. Refactorings should follow abstraction boundaries.
Rather than having a special case for verbs that spawn dialogs, it seems a better idea to provide the ability for a dynamically loaded extension to register a general-purpose callback as a verb. Remember, a verb that shows a dialog might well want to do some preparatory work on the document too.
Also, showDialog("dialogName") doesn't seem like an inordinate amount of code. I'd be more concerned about eliminating the cut-and-pasting of dt->_dlg_mgr and the associated exposure of internal details.
-mental
On 5/25/05, MenTaLguY <mental@...3...> wrote:
It seems to be refactoring the wrong pieces to me. I'd like to stick with strings or GQuarks to identify dialogs. There's no reason to tie dialogs to verbs directly.
But they are already tied to them directly. A verb is a non-optional parameter of a Dialog constructor. It's used for building the title bar string from the verb string and its shortcut, if any. I don't see any problem with that, and it's much better than duplicating the title in the dialog code (which lead to discrepancies, no shortcut display, extra work for translators etc).
So I'm for Bryce's suggestion. You create a dialog, create a verb for it too (even if it's not in menus).
On Wed, May 25, 2005 at 10:38:59AM -0400, MenTaLguY wrote:
On Wed, 2005-05-25 at 01:23, Bryce Harrington wrote:
Btw, this is intended to only be an interim solution. Ultimately, I would like to see a mechanism that allows the dialogs to be registered in a way that would allow invoking them without passing a string. I.e., instead of a switch statement with cases to handle each verb define as above, you could replace the entire switch statement with just:
dt->_dlg_mgt->showDialog(reinterpret_cast<std::size_t>(data));...
Hmm. I'm not overly fond of this approach.
It seems to be refactoring the wrong pieces to me. I'd like to stick with strings or GQuarks to identify dialogs. There's no reason to tie dialogs to verbs directly. Refactorings should follow abstraction boundaries.
Right, the above is just conceptually what I'm thinking. The idea being to eliminate the switch statement and replace it with a single call that passes some data item to indicate what dialog to launch. Whether that data item is a verb, or a string, or a GQuark, I don't know. You'll note that I added some GQuark support in Dialog::Manager. I sort of suspect that may be the ideal way to go.
Rather than having a special case for verbs that spawn dialogs, it seems a better idea to provide the ability for a dynamically loaded extension to register a general-purpose callback as a verb. Remember, a verb that shows a dialog might well want to do some preparatory work on the document too.
Ah, good idea. Of course, that's beyond the scope of dialogs.
Btw, gtkmm has its own entirely new idea of how to handle verbs (called 'Actions'), that is fairly incompatible with what is in verbs.*. Jonadab and I did most of the work to create that code, but it's quite different than how things are done in the current codebase. The dialogs system is not dependent on it, so it won't be an issue for a while, but I think you should take a long look at it.
My feeling is that the gtkmm Action system has features that our current verb system lacks. I think it may make it possible to do verbs more dynamically than currently. On the other hand, it feels a bit overly verbose; it seems adding a new menu item requires adding the same thing in two or three places, and I've wondered if there might be a better way to do that. It uses an XML file to connect actions to menu items, which is sort of convenient (no compiling!) but is a little awkward to troubleshoot sometimes - in a few cases I've mismatched the action between the code and the xml, such as plural in one place and single in the other, and you kind of need to know where all to look in order to fix it. Anyway, maybe this is just a minor quibble, so I think you should look at it and see what you think.
I would suggest that if we feel the current verbs system has advantages over the gtkmm Action system, that rather than avoiding use of Actions, that some consideration be put into helping improve Actions, since it seems to be the standard way for handlign menu events for Gtkmm apps.
Also, showDialog("dialogName") doesn't seem like an inordinate amount of code. I'd be more concerned about eliminating the cut-and-pasting of dt->_dlg_mgr and the associated exposure of internal details.
Correct. This was kind of a shortcut to getting the new dialog code integrated. I had thought that perhaps this could be done by adding a member routine to SPDesktop for launching dialogs. However, I opted to leave it exposed until that other issue was sorted out, regarding how dialogs should be shown.
Btw, if you look at the verb handlers for some of the other dialogs, some do more than merely show the dialog; some do some extra prep work. I haven't looked very deeply at that, but I suspect it may impose requirements that may make it inconvenient to just have a showDialog() routine. It may be necessary to have a getDialog() routine, to allow the verb handler a chance to make those customizations. However, I think a preferable solution would be to push that custom stuff down into the dialog's code itself, so all dialogs can be handled the same way. Again though, I haven't looked at those things at all, so I don't know if that's easy or hard to do.
Bryce
Quoting Bryce Harrington <bryce@...260...>:
I haven't looked very deeply at that, but I suspect it may impose requirements that may make it inconvenient to just have a showDialog() routine. It may be necessary to have a getDialog() routine, to allow the verb handler a chance to make those customizations. However, I think a preferable solution would be to push that custom stuff down into the dialog's code itself, so all dialogs can be handled the same way.
Probably what you are reaching for is the Template Method pattern (http://www.dofactory.com/Patterns/PatternTemplate.aspx), where showDialog() would act as the template method [not to be confused with C++ templates].
-mental
On Wed, May 25, 2005 at 03:51:19PM -0400, mental@...3... wrote:
Quoting Bryce Harrington <bryce@...260...>:
I haven't looked very deeply at that, but I suspect it may impose requirements that may make it inconvenient to just have a showDialog() routine. It may be necessary to have a getDialog() routine, to allow the verb handler a chance to make those customizations. However, I think a preferable solution would be to push that custom stuff down into the dialog's code itself, so all dialogs can be handled the same way.
Probably what you are reaching for is the Template Method pattern (http://www.dofactory.com/Patterns/PatternTemplate.aspx), where showDialog() would act as the template method [not to be confused with C++ templates].
It could be. Like I said, I haven't really analyzed the more complex dialogs to see why they have the custom behavior. It may even be possible to solve it just with the virtual functions.
I don't think this has to be sorted out for the 0.42 release. Possibly as people convert more dialogs it'll get solved dialog by dialog without any trouble.
Bryce
Since this can include moving/renaming classes, and even changing Makefile_inserts, people will likely need to run autogen.sh and a 'make clean;make' in the next few days to avoid missing symbols.
Bob
On 5/25/05, Bob Jamison <rjamison@...357...> wrote:
Since this can include moving/renaming classes, and even changing Makefile_inserts, people will likely need to run autogen.sh and a 'make clean;make' in the next few days to avoid missing symbols.
I did run make clean and autogen and configure, but I'm still getting:
ui/dialog/libuidialog.a(dialog-manager.o)(.gnu.linkonce.t._ZN8Inkscape2UI6Dialog57_GLOBAL__N_ui_dialog_dialog_manager.cpp_9D2164D3_478C0BDB6createINS1_10TileDialogEEEPNS1_6DialogEv+0x18): In function `Inkscape::UI::Dialog::Dialog* Inkscape::UI::Dialog::(anonymous namespace)::createInkscape::UI::Dialog::TileDialog()': /usr/lib/gcc/i586-mandrake-linux-gnu/3.4.1/../../../../include/c++/3.4.1/bits/stl_tree.h:444: undefined reference to `Inkscape::UI::Dialog::TileDialog::TileDialog()' ui/dialog/libuidialog.a(tracedialog.o)(.text+0x1d): In function `Inkscape::UI::Dialog::TraceDialogImpl::potraceProcess(bool)': ui/dialog/tracedialog.cpp:185: undefined reference to `Inkscape::Trace::Potrace::PotraceTracingEngine::PotraceTracingEngine()' ui/dialog/libuidialog.a(tracedialog.o)(.text+0xc6):ui/dialog/tracedialog.cpp:233: undefined reference to `Inkscape::Trace::Tracer::getSelectedImage()' ui/dialog/libuidialog.a(tracedialog.o)(.text+0xda):ui/dialog/tracedialog.cpp:236: undefined reference to `Inkscape::Trace::Potrace::PotraceTracingEngine::preview(_GdkPixbuf*)' ui/dialog/libuidialog.a(tracedialog.o)(.text+0x1c7):ui/dialog/tracedialog.cpp:260: undefined reference to `Inkscape::Trace::Tracer::trace(Inkscape::Trace::TracingEngine*)' ui/dialog/libuidialog.a(tracedialog.o)(.text+0x6a8): In function `Inkscape::UI::Dialog::TraceDialogImpl::~TraceDialogImpl()': ui/dialog/tracedialog.cpp:554: undefined reference to `Inkscape::Trace::Tracer::~Tracer()' ui/dialog/libuidialog.a(tracedialog.o)(.text+0x9c3):ui/dialog/tracedialog.cpp:554: undefined reference to `Inkscape::Trace::Tracer::~Tracer()' ui/dialog/libuidialog.a(tracedialog.o)(.text+0xdc8): In function `Inkscape::UI::Dialog::TraceDialogImpl::~TraceDialogImpl()': ui/dialog/tracedialog.cpp:554: undefined reference to `Inkscape::Trace::Tracer::~Tracer()' ui/dialog/libuidialog.a(tracedialog.o)(.text+0x1103):ui/dialog/tracedialog.cpp:554: undefined reference to `Inkscape::Trace::Tracer::~Tracer()' ui/dialog/libuidialog.a(tracedialog.o)(.text+0x1538): In function `Inkscape::UI::Dialog::TraceDialogImpl::~TraceDialogImpl()': ui/dialog/tracedialog.cpp:554: undefined reference to `Inkscape::Trace::Tracer::~Tracer()' ui/dialog/libuidialog.a(tracedialog.o)(.text+0x1878):ui/dialog/tracedialog.cpp:554: more undefined references to `Inkscape::Trace::Tracer::~Tracer()' follow ui/dialog/libuidialog.a(tracedialog.o)(.text+0x1ab4): In function `Inkscape::UI::Dialog::TraceDialogImpl::TraceDialogImpl()': ui/dialog/tracedialog.cpp:336: undefined reference to `Inkscape::Trace::Tracer::Tracer()' ui/dialog/libuidialog.a(tracedialog.o)(.text+0x3141):ui/dialog/tracedialog.cpp:536: undefined reference to `Inkscape::Trace::Tracer::~Tracer()' ui/dialog/libuidialog.a(tracedialog.o)(.text+0x37a6): In function `Inkscape::UI::Dialog::TraceDialogImpl::TraceDialogImpl()': ui/dialog/tracedialog.cpp:336: undefined reference to `Inkscape::Trace::Tracer::Tracer()' ui/dialog/libuidialog.a(tracedialog.o)(.text+0x4e01):ui/dialog/tracedialog.cpp:536: undefined reference to `Inkscape::Trace::Tracer::~Tracer()' ui/dialog/libuidialog.a(tracedialog.o)(.text+0x39e): In function `Inkscape::UI::Dialog::TraceDialogImpl::abort()': ui/dialog/tracedialog.cpp:278: undefined reference to `Inkscape::Trace::Tracer::abort()' collect2: ld returned 1 exit status
bulia byak wrote:
On 5/25/05, Bob Jamison <rjamison@...357...> wrote:
Since this can include moving/renaming classes, and even changing Makefile_inserts, people will likely need to run autogen.sh and a 'make clean;make' in the next few days to avoid missing symbols.
I did run make clean and autogen and configure, but I'm still getting:
ui/dialog/libuidialog.a(dialog-manager.o)(.gnu.linkonce.t._ZN8Inkscape2UI6Dialog57_GLOBAL__N_ui_dialog_dialog_manager.cpp_9D2164D3_478C0BDB6createINS1_10TileDialogEEEPNS1_6DialogEv+0x18): In function `Inkscape::UI::Dialog::Dialog* Inkscape::UI::Dialog::(anonymous namespace)::createInkscape::UI::Dialog::TileDialog()': /usr/lib/gcc/i586-mandrake-linux-gnu/3.4.1/../../../../include/c++/3.4.1/bits/stl_tree.h:444: undefined reference to `Inkscape::UI::Dialog::TileDialog::TileDialog()' ui/dialog/libuidialog.a(tracedialog.o)(.text+0x1d): In function `Inkscape::UI::Dialog::TraceDialogImpl::potraceProcess(bool)': ui/dialog/tracedialog.cpp:185: undefined reference to `Inkscape::Trace::Potrace::PotraceTracingEngine::PotraceTracingEngine()' ui/dialog/libuidialog.a(tracedialog.o)(.text+0xc6):ui/dialog/tracedialog.cpp:233: undefined reference to `Inkscape::Trace::Tracer::getSelectedImage()' ui/dialog/libuidialog.a(tracedialog.o)(.text+0xda):ui/dialog/tracedialog.cpp:236: undefined reference to `Inkscape::Trace::Potrace::PotraceTracingEngine::preview(_GdkPixbuf*)' ui/dialog/libuidialog.a(tracedialog.o)(.text+0x1c7):ui/dialog/tracedialog.cpp:260: undefined reference to `Inkscape::Trace::Tracer::trace(Inkscape::Trace::TracingEngine*)' ui/dialog/libuidialog.a(tracedialog.o)(.text+0x6a8): In function `Inkscape::UI::Dialog::TraceDialogImpl::~TraceDialogImpl()': ui/dialog/tracedialog.cpp:554: undefined reference to `Inkscape::Trace::Tracer::~Tracer()' ui/dialog/libuidialog.a(tracedialog.o)(.text+0x9c3):ui/dialog/tracedialog.cpp:554: undefined reference to `Inkscape::Trace::Tracer::~Tracer()' ui/dialog/libuidialog.a(tracedialog.o)(.text+0xdc8): In function `Inkscape::UI::Dialog::TraceDialogImpl::~TraceDialogImpl()': ui/dialog/tracedialog.cpp:554: undefined reference to `Inkscape::Trace::Tracer::~Tracer()' ui/dialog/libuidialog.a(tracedialog.o)(.text+0x1103):ui/dialog/tracedialog.cpp:554: undefined reference to `Inkscape::Trace::Tracer::~Tracer()' ui/dialog/libuidialog.a(tracedialog.o)(.text+0x1538): In function `Inkscape::UI::Dialog::TraceDialogImpl::~TraceDialogImpl()': ui/dialog/tracedialog.cpp:554: undefined reference to `Inkscape::Trace::Tracer::~Tracer()' ui/dialog/libuidialog.a(tracedialog.o)(.text+0x1878):ui/dialog/tracedialog.cpp:554: more undefined references to `Inkscape::Trace::Tracer::~Tracer()' follow ui/dialog/libuidialog.a(tracedialog.o)(.text+0x1ab4): In function `Inkscape::UI::Dialog::TraceDialogImpl::TraceDialogImpl()': ui/dialog/tracedialog.cpp:336: undefined reference to `Inkscape::Trace::Tracer::Tracer()' ui/dialog/libuidialog.a(tracedialog.o)(.text+0x3141):ui/dialog/tracedialog.cpp:536: undefined reference to `Inkscape::Trace::Tracer::~Tracer()' ui/dialog/libuidialog.a(tracedialog.o)(.text+0x37a6): In function `Inkscape::UI::Dialog::TraceDialogImpl::TraceDialogImpl()': ui/dialog/tracedialog.cpp:336: undefined reference to `Inkscape::Trace::Tracer::Tracer()' ui/dialog/libuidialog.a(tracedialog.o)(.text+0x4e01):ui/dialog/tracedialog.cpp:536: undefined reference to `Inkscape::Trace::Tracer::~Tracer()' ui/dialog/libuidialog.a(tracedialog.o)(.text+0x39e): In function `Inkscape::UI::Dialog::TraceDialogImpl::abort()': ui/dialog/tracedialog.cpp:278: undefined reference to `Inkscape::Trace::Tracer::abort()' collect2: ld returned 1 exit status
While trying to figure out the link-order problem, I looked at the link line:
g++ -Wall -W -Wpointer-arith -Wcast-align -Wsign-compare -Woverloaded-virtual -Wswitch -Wno-unused-parameter -g -O2 -o inkscape --export-dynamic main.o -Wl,--export-dynamic -Wl,-E -Wl,-rpath -Wl,/usr/lib/perl5/5.8.5/i386-linux-thread-multi/CORE libinkpre.a application/libinkapp.a dialogs/libspdialogs.a trace/libtrace.a svg/libspsvg.a widgets/libspwidgets.a display/libspdisplay.a helper/libspchelp.a libcroco/libcroco.a libnrtype/libnrtype.a libnr/libnr.a livarot/libvarot.a ui/view/libuiview.a ui/libui.a ui/dialog/libuidialog.a ui/widget/libuiwidget.a extension/libextension.a extension/implementation/libimplementation.a extension/internal/libinternal.a extension/script/libscript.a xml/libspxml.a util/libinkutil.a io/libio.a inkjar/libinkjar.a libinkpost.a debug/libinkdebug.a -L/usr/local/lib ....blah....
Notice that dialogs/libspdialogs.a is near the front, but ui/dialog/libuidialog.a is much farther down the list.
Looking in src/Makefile_insert, here is the link-order of the libs:
inkscape_private_libs = \ libinkpre.a \ application/libinkapp.a \ dialogs/libspdialogs.a \ trace/libtrace.a \ svg/libspsvg.a \ widgets/libspwidgets.a \ display/libspdisplay.a \ helper/libspchelp.a \ libcroco/libcroco.a \ libnrtype/libnrtype.a \ libnr/libnr.a \ livarot/libvarot.a \ ui/view/libuiview.a \ ui/libui.a \ ui/dialog/libuidialog.a \ ui/widget/libuiwidget.a \ extension/libextension.a \ extension/implementation/libimplementation.a \ extension/internal/libinternal.a \ extension/script/libscript.a \ xml/libspxml.a \ util/libinkutil.a \ io/libio.a \ $(inkjar_libs) \ libinkpost.a \ debug/libinkdebug.a
Since libuidialog.a will reference other Inkscape items, the same as libspdialogs.a, it should be at least as far up the list as that one. And since it references TileDialog, it should even be in front of it. So I changed its position to this:
inkscape_private_libs = \ libinkpre.a \ application/libinkapp.a \ ui/dialog/libuidialog.a \ dialogs/libspdialogs.a \ trace/libtrace.a \ svg/libspsvg.a \ widgets/libspwidgets.a \ display/libspdisplay.a \ helper/libspchelp.a \ libcroco/libcroco.a \ libnrtype/libnrtype.a \ libnr/libnr.a \ livarot/libvarot.a \ ui/view/libuiview.a \ ui/libui.a \ ui/widget/libuiwidget.a \ extension/libextension.a \ extension/implementation/libimplementation.a \ extension/internal/libinternal.a \ extension/script/libscript.a \ xml/libspxml.a \ util/libinkutil.a \ io/libio.a \ $(inkjar_libs) \ libinkpost.a \ debug/libinkdebug.a
And the link now looks like this.
g++ -Wall -W -Wpointer-arith -Wcast-align -Wsign-compare -Woverloaded-virtual -Wswitch -Wno-unused-parameter -g -O2 -o inkscape --export-dynamic main.o -Wl,--export-dynamic -Wl,-E -Wl,-rpath -Wl,/usr/lib/perl5/5.8.5/i386-linux-thread-multi/CORE libinkpre.a application/libinkapp.a ui/dialog/libuidialog.a dialogs/libspdialogs.a trace/libtrace.a svg/libspsvg.a widgets/libspwidgets.a display/libspdisplay.a helper/libspchelp.a libcroco/libcroco.a libnrtype/libnrtype.a libnr/libnr.a livarot/libvarot.a ui/view/libuiview.a ui/libui.a ui/widget/libuiwidget.a extension/libextension.a extension/implementation/libimplementation.a extension/internal/libinternal.a extension/script/libscript.a xml/libspxml.a util/libinkutil.a io/libio.a inkjar/libinkjar.a libinkpost.a debug/libinkdebug.a -L/usr/local/lib ....blah....
This works, so I committed it. Let me know if I broke anything.
Bob
Bob Jamison wrote:
And the link now looks like this.
g++ -Wall -W -Wpointer-arith -Wcast-align -Wsign-compare -Woverloaded-virtual -Wswitch -Wno-unused-parameter -g -O2 -o inkscape --export-dynamic main.o -Wl,--export-dynamic -Wl,-E -Wl,-rpath -Wl,/usr/lib/perl5/5.8.5/i386-linux-thread-multi/CORE libinkpre.a application/libinkapp.a ui/dialog/libuidialog.a dialogs/libspdialogs.a trace/libtrace.a svg/libspsvg.a widgets/libspwidgets.a display/libspdisplay.a helper/libspchelp.a libcroco/libcroco.a libnrtype/libnrtype.a libnr/libnr.a livarot/libvarot.a ui/view/libuiview.a ui/libui.a ui/widget/libuiwidget.a extension/libextension.a extension/implementation/libimplementation.a extension/internal/libinternal.a extension/script/libscript.a xml/libspxml.a util/libinkutil.a io/libio.a inkjar/libinkjar.a libinkpost.a debug/libinkdebug.a -L/usr/local/lib ....blah....
This works, so I committed it. Let me know if I broke anything.
Is it maybe time to fix /src/Makefile.am and /src/Makefile_inserts? Since link-order will likely get worse in the future, how about this idea....
Instead of linking together the set of individual $inkscape_private_libs (and their link problems) with the client apps to make the executables, concatenate each of their libxxx_a_OBJECTS variables together to make a single libinkscape.a and libinkscape.so. Link -THAT- with the client apps (inkscape, inkview, etc)
Benefits:
o This will remove the link-order problem within Inkscape entirely.
o Since inkscape and inkview objects will no longer be almost duplicates of each other (if we link dynamically), binary packages will be smaller.
o We could finally export an Inkscape API for others who wish to use it.
o The individual small .a's are still available for testers.
This is just an idea. But remember that we have been using a monolithic libinkscape.a on Win32 for 18 months now. We have never had a link order problem with it, or duplicate symbols (except for when a test .cpp file has a main() in it). So we know that it would work.
Bob
Bob Jamison wrote:
Instead of linking together the set of individual $inkscape_private_libs (and their link problems) with the client apps to make the executables, concatenate each of their libxxx_a_OBJECTS variables together to make a single libinkscape.a and libinkscape.so. Link -THAT- with the client apps (inkscape, inkview, etc)
Here is what I mean. Look at the entry for libinkscape_a_LIBADD.
For some reason, this entry isn't working at the moment. I think it is a syntax problem with one of the included files. I'm still looking at it. I have tried it with fewer included libs, and it does make a library just fine.
Bob
## Process this file with automake to produce Makefile.in
# ################################################ # # G L O B A L # # ################################################
# Should work in either automake1.7 or 1.8, but 1.6 doesn't # handle foo/libfoo_a_CPPFLAGS properly (if at all). # Update: We now avoid setting foo/libfoo_a_CPPFLAGS, # so perhaps 1.6 will work. AUTOMAKE_OPTIONS = 1.7 subdir-objects
INCLUDES = \ $(FREETYPE_CFLAGS) \ $(GNOME_PRINT_CFLAGS) \ $(GNOME_VFS_CFLAGS) \ $(XFT_CFLAGS) \ -DPOTRACE="potrace" \ $(INKSCAPE_CFLAGS) \ -I$(top_srcdir)/cxxtest
include Makefile_insert include application/Makefile_insert include dialogs/Makefile_insert include display/Makefile_insert include extension/Makefile_insert include extension/implementation/Makefile_insert include extension/internal/Makefile_insert include extension/script/Makefile_insert include helper/Makefile_insert include inkjar/Makefile_insert include io/Makefile_insert include libcroco/Makefile_insert include libnr/Makefile_insert include libnrtype/Makefile_insert include livarot/Makefile_insert include svg/Makefile_insert include utest/Makefile_insert include widgets/Makefile_insert include debug/Makefile_insert include xml/Makefile_insert include traits/Makefile_insert include algorithms/Makefile_insert include ui/Makefile_insert include ui/dialog/Makefile_insert include ui/view/Makefile_insert include ui/widget/Makefile_insert include util/Makefile_insert include trace/Makefile_insert
libinkscape_a_SOURCES =
libinkscape_a_LIBADD = \ $(libinkpre_a_OBJECTS) \ $(libinkpost_a_OBJECTS) \ $(application_libinkapp_a_OBJECTS) \ $(debug_libinkdebug_a_OBJECTS) \ $(dialogs_libspdialogs_a_OBJECTS) \ $(display_libspdisplay_a_OBJECTS) \ $(extension_implementation_libimplementation_a_OBJECTS) \ $(extension_internal_libinternal_a_OBJECTS) \ $(extension_libextension_a_OBJECTS) \ $(extension_script_libscript_a_OBJECTS) \ $(helper_libspchelp_a_OBJECTS) \ $(io_libio_a_OBJECTS) \ $(libcroco_libcroco_a_OBJECTS) \ $(libnr_libnr_a_OBJECTS) \ $(libnrtype_libnrtype_a_OBJECTS) \ $(livarot_libvarot_a_OBJECTS) \ $(svg_libspsvg_a_OBJECTS) \ $(trace_libtrace_a_OBJECTS) \ $(ui_dialog_libuidialog_a_OBJECTS) \ $(ui_libui_a_OBJECTS) \ $(ui_view_libuiview_a_OBJECTS) \ $(ui_widget_libuiwidget_a_OBJECTS) \ $(util_libinkutil_a_OBJECTS) \ $(widgets_libspwidgets_a_OBJECTS) \ $(xml_libspxml_a_OBJECTS)
inkscape2_SOURCES = main.cpp inkscape2_LDADD = libinkscape.a inkscape2_LDFLAGS = --export-dynamic
bin_PROGRAMS = inkscape inkscape2 inkview
noinst_LIBRARIES = \ libinkscape.a \ libinkpre.a \ application/libinkapp.a \ dialogs/libspdialogs.a \ display/libspdisplay.a \ extension/implementation/libimplementation.a \ extension/internal/libinternal.a \ extension/libextension.a \ extension/script/libscript.a \ helper/libspchelp.a \ io/libio.a \ libcroco/libcroco.a \ ui/libui.a \ ui/dialog/libuidialog.a \ ui/view/libuiview.a \ ui/widget/libuiwidget.a \ util/libinkutil.a \ debug/libinkdebug.a \ $(inkjar_libs) \ libnr/libnr.a \ libnrtype/libnrtype.a \ livarot/libvarot.a \ svg/libspsvg.a \ widgets/libspwidgets.a \ trace/libtrace.a \ xml/libspxml.a \ libinkpost.a
check_LIBRARIES = \ libnr/libtest-nr.a \ xml/libtest-xml.a
DISTCLEANFILES = \ helper/sp-marshal.cpp \ helper/sp-marshal.h \ inkscape_version.h
EXTRA_DIST = \ mkdep.pl \ mkfiles.pl \ make.exclude \ make.dep \ make.files \ make.ofiles \ Doxyfile \ algorithms/makefile.in \ application/makefile.in \ dialogs/makefile.in \ display/makefile.in \ extension/makefile.in \ extension/implementation/makefile.in \ extension/internal/makefile.in \ helper/makefile.in \ inkjar/makefile.in \ io/makefile.in \ io/crystalegg.xml \ io/doc2html.xsl \ libnr/makefile.in \ libnrtype/makefile.in \ livarot/makefile.in \ svg/makefile.in \ trace/makefile.in \ traits/makefile.in \ utest/makefile.in \ ui/makefile.in \ ui/dialog/makefile.in \ ui/view/makefile.in \ ui/widget/makefile.in \ util/makefile.in \ widgets/makefile.in \ xml/makefile.in \ extension/internal/gnome.cpp \ extension/internal/gnome.h \ extension/internal/win32.cpp \ extension/internal/win32.h \ helper/sp-marshal.list \ utest/utest.h \ utest/test-1ary-cases.h \ algorithms/find-if-before.h \ algorithms/find-last-if.h \ algorithms/longest-common-suffix.h \ traits/copy.h \ traits/function.h \ traits/list-copy.h \ traits/reference.h
EXTRA_PROGRAMS = \ inkview \ libnr/testnr
TESTS = \ test-all$(EXEEXT) \ attributes-test$(EXEEXT) \ dir-util-test$(EXEEXT) \ extract-uri-test$(EXEEXT) \ mod360-test$(EXEEXT) \ round-test$(EXEEXT) \ sp-gradient-test$(EXEEXT) \ sp-style-elem-test$(EXEEXT) \ style-test$(EXEEXT) \ display/bezier-utils-test$(EXEEXT) \ helper/units-test$(EXEEXT) \ libnr/in-svg-plane-test$(EXEEXT) \ libnr/nr-matrix-test$(EXEEXT) \ libnr/nr-point-fns-test$(EXEEXT) \ libnr/nr-rotate-test$(EXEEXT) \ libnr/nr-rotate-fns-test$(EXEEXT) \ libnr/nr-scale-test$(EXEEXT) \ libnr/nr-translate-test$(EXEEXT) \ libnr/nr-types-test$(EXEEXT) \ libnr/test-nr$(EXEEXT) \ util/list-container-test$(EXEEXT) \ xml/test-xml$(EXEEXT) \ xml/quote-test$(EXEEXT) \ xml/repr-action-test$(EXEEXT)
# streamtest is unfinished and can't handle the relocations done during # "make distcheck". Not needed for the 0.41 release. # io/streamtest$(EXEEXT)
# automake adds $(EXEEXT) to check_PROGRAMS items but not to TESTS items: # TESTS items can be scripts etc.
check_PROGRAMS = \ test-all \ attributes-test \ dir-util-test \ extract-uri-test \ mod360-test \ round-test \ sp-gradient-test \ sp-style-elem-test \ style-test \ display/bezier-utils-test \ helper/units-test \ libnr/in-svg-plane-test \ libnr/nr-matrix-test \ libnr/nr-point-fns-test \ libnr/nr-rotate-test \ libnr/nr-rotate-fns-test \ libnr/nr-scale-test \ libnr/nr-translate-test \ libnr/nr-types-test \ libnr/test-nr \ util/list-container-test \ xml/test-xml \ xml/quote-test \ xml/repr-action-test
# io/streamtest
test-all.cpp: $(libnr_test_nr_a_SOURCES) $(xml_test_xml_a_SOURCES) $(libnr_test_nr_includes) $(xml_test_xml_includes) $(top_srcdir)/cxxtest/cxxtestgen.pl --error-printer -root -o test-all.cpp $(libnr_test_nr_includes) $(xml_test_xml_includes)
test_all_SOURCES = \ test-all.cpp
test_all_LDADD = \ $(libnr_test_nr_LDADD) \ $(xml_test_xml_LDADD)
# ################################################ # # D I S T # # ################################################
dist-hook: mkdir $(distdir)/pixmaps cp $(srcdir)/pixmaps/*xpm $(distdir)/pixmaps
On Wed, May 25, 2005 at 06:12:31PM -0500, Bob Jamison wrote:
Bob Jamison wrote:
Instead of linking together the set of individual $inkscape_private_libs (and their link problems) with the client apps to make the executables, concatenate each of their libxxx_a_OBJECTS variables together to make a single libinkscape.a and libinkscape.so. Link -THAT- with the client apps (inkscape, inkview, etc)
Here is what I mean. Look at the entry for libinkscape_a_LIBADD.
For some reason, this entry isn't working at the moment. I think it is a syntax problem with one of the included files. I'm still looking at it. I have tried it with fewer included libs, and it does make a library just fine.
Would it make sense to consider breaking the libcroco, libnr, libnrtype, and livarot sections to separate library units? IIRC they're fairly well encapsulated from the rest of the code, and represent fairly good chunks of code that don't change too often, so leaving them separate might help build times a bit?
Bryce
I have a script to find an appropriate link order, based on nm --extern-only --defined-only and --undefined-only. More accurately, the script produces input suitable for tsort. tsort writes cycles to stderr, so we could ensure that each cycle of objects is in a single library.
If I use a simple `find' command to get a list of objects rather than use the correct list of objects from the makefile, then I find 186 distinct object files mentioned on tsort's stderr, and 198 object files not mentioned on stderr. Take those numbers with a handful of salt given that I didn't use the correct set of object files, but it should give us some idea of how many files would be taken care of by putting cycles into libs.
I attach the perl script.
I think this idea is more or less orthogonal to Bryce's proposal re existing self-contained libs. Btw, I think that proposal good regardless of whether we use tsort to form libs.
pjrm.
Quoting Bryce Harrington <bryce@...260...>:
Would it make sense to consider breaking the libcroco, libnr, libnrtype, and livarot sections to separate library units? IIRC they're fairly well encapsulated from the rest of the code, and represent fairly good chunks of code that don't change too often, so leaving them separate might help build times a bit?
Dependencies on (what will have been) libinkutil.a and on gc.o would still introduce circularity, at least for libnr and (I think) livarot.
-mental
On 5/25/05, Bob Jamison <rjamison@...357...> wrote:
This works, so I committed it. Let me know if I broke anything.
Thanks, works fine now.
A bug in trace: Import inkscape.png from the root dir of the distribution. Select and press Preview in Trace, with default settings. The preview shows only the outline of the mountain. However if you press Trace, the result is OK.
Is it just me, or is anyone else having link-order problems for things in /ui/dialog referencing things elsewhere?
Bob
On Wed, May 25, 2005 at 11:42:58AM -0500, Bob Jamison wrote:
Is it just me, or is anyone else having link-order problems for things in /ui/dialog referencing things elsewhere?
Yes, this was quite common while I was integrating the gtkmm code. I resolved it for the dialogs I did by adjusting the location of where the ui/dialog lib is listed in the higher level makefiles. It's quite possible that it may require additional adjustment to provide the dependencies needed by other dialogs.
Note that you may also need to shuffle the location of ui/widgets since the dialog code depends on that. Also, I seem to remember running afoul of a circular dependency with something in io/; I don't remember if I solved that or just worked around it. You may discover new circular dependencies for other dialogs. I think to prevent these sorts of dependency issues in the future, we need to be a bit more attentive of what depends on what in the codebase; this isn't easy but it's a good rigor to get into. This is probably directly related to the header file interdependency problem that we've known about and been working on, so solving one may help address the other.
Bryce
On Wed, 2005-05-25 at 10:51 -0700, Bryce Harrington wrote:
On Wed, May 25, 2005 at 11:42:58AM -0500, Bob Jamison wrote:
Is it just me, or is anyone else having link-order problems for things in /ui/dialog referencing things elsewhere?
Yes, this was quite common while I was integrating the gtkmm code. I resolved it for the dialogs I did by adjusting the location of where the ui/dialog lib is listed in the higher level makefiles. It's quite possible that it may require additional adjustment to provide the dependencies needed by other dialogs.
I get this in glom too. Mentioning the same sub-libs twice can also fix it sometimes. Apparently it's normal.
participants (9)
-
unknown＠example.com -
Bob Jamison -
Bryce Harrington -
bulia byak -
john cliff -
Jon A. Cruz -
MenTaLguY -
Murray Cumming -
Peter Moulder