On Mon, May 09, 2005 at 07:05:47PM -0700, Jon Phillips wrote:
On Sun, 2005-05-08 at 13:33 -0700, Bryce Harrington wrote:
Today I think the new dialog code is pretty good. There is a base class we can use for dialogs, that provides the common behaviors, and a DialogManager to better track their memory and instantiation. The individual dialogs still need to all be converted over, but we've already got a handful converted, and there's another handful that are already in gtkmm but just need to be restyled to work with the new code. So now, the rest of the dialog conversion work *can* be done piece by piece, without risking problematic inconsistencies.
Did you use my new generic map structure implementation for managing dialogs??? I would like to get that in place soon... ;)
In fact yes, I did have fun playing with that! :-)
I made a few tweaks to the code. First, instead of making it a hash map of strings to Dialogs, it now uses a map of GQuarks to Dialogs. That should be somewhat more efficient. Also, I added GQuark versions of the add, remove, etc. routines.
There's just one more feature that must be added before we can rip out the current code and go entirely with your generic map structure.
You'll notice that in the current code, the functions are like this:
Dialog* DialogManager::getExportDialog() { if (_export_dialog == NULL) { _export_dialog = new Export; hide_dialogs.connect(sigc::mem_fun(*_export_dialog, &Export::onHideDialogs)); hide_f12.connect(sigc::mem_fun(*_export_dialog, &Export::onHideF12)); show_dialogs.connect(sigc::mem_fun(*_export_dialog, &Export::onShowDialogs)); show_f12.connect(sigc::mem_fun(*_export_dialog, &Export::onShowF12)); } return _export_dialog; }
Each of the dialogs has one function like that. The function basically looks to see if the export dialog has been instantiated, instantiates it if needed, and returns it. The fact that it auto-instantiates when called is very nice because a) it allows the caller to assume the dialog will always exist under normal circumstances, and b) it allows us to be lazy about instantiating it until the dialog is actually needed.
Anyway, we want to replace all this with your handy dandy genericized function:
Dialog* DialogManager::getDialog(GQuark q) { DialogMap::const_iterator iter = _dialog_map.find(q); if (iter != _dialog_map.end()) { return (*iter).second; // dialog found } else { return NULL; // dialog not found } }
However, you can see that in the case of a dialog not found, it doesn't instantiate it for us nicely, but instead returns NULL.
A good technique for providing this capability is to implement Factory methods (see a Design Patterns book for detail).
This probably means we will need to have each dialog include a create() function that returns a new'd Dialog* of its class, and then build a hash of GQuarks to create() function pointers. We would then also need a registration function to add new create() functions to the hash.
Once these things exist, we can eliminate all the existing get*Dialog() routines and go with your code exclusively.
I think that slow refactoring is key, but more importantly we decided that no more of the interface will be coded in GTK+, but gtkmm. I think if we decided this much much more will happen. Maybe it is not so realistic to force conversion from the pure top down effort, but if we set some simple rules, like WE NOW PREFER GTKMM, I think we will reach our goal quicker.
Yeah, I talked with mental about this and think it's correct that a slow refactoring is a more viable approach, even though it seems to take forever to get through. ;-) I think this is especially true for the view/desktop code.
Thus, I would like to propose we scrap the remaining plans for 0.42 in the roadmap, and go ahead and start the 0.42 release process.
Opinions?
I think the sooner the better. I feel the attn. span slowing on Inkscape, which is okay, but I think better to stay in the news and to keep feeding users new features, rather than keep them with the old copy of Inkscape...our quicker releases is one of our advantages...
Yup, my thoughts too.
Bryce