Since the main focus of 0.47 is refactoring, I'm posting some ideas for discussion I've come up with while refactoring the preferences code (the final version of which just made it into SVN).
1. id-clashes.cpp - this is a stopgap solution at best. The roadmap proposes to solve this problem by making SPObjects ref-aware. I think this isn't a good idea, as it uses a canvas level API to solve an XML level problem. The real solution would be to make the XML tree CSS-aware (to detect ID references in CSS), and handle this transparently by special-casing the "id" attribute so that setAttribute("id", "something") would automatically update every ID in the document. An alternative approach would be to have an XML::Document level API like alterID(gchar const *oldid, gchar const *newid) which would do the same.
2. Tools tree. This is one of the last big chunks of GObject left in the codebase. It's also rather messy - for example, every tool has a gradient dragger but in reality it only makes sense for the Node and Gradient tools. Converting this hierarchy to C++ would be a great step forward. It would also get rid of toolbox.cpp.
3. Namespace proliferation. Many namespaces contain just a few members and their fully qualified names are very long. The record is namespace depth of 4 for internal bitmap extensions and a few other things. Is this level of granularity really necessary? There's also the UI namespace - its purpose is unclear to me because Inkscape::UI::Widget::SuperGizmo conveys no additional information compared to Inkscape::Widget::SuperGizmo. It's obvious to me that widgets and dialogs are parts of the UI, so why put Widget and Dialog namespaces in another namespace? On top of that, names of many classes are redundant, like Inkscape::UI::Dialog::SomeDialog. It should be either Inkscape::UI::SomeDialog or Inkscape::Dialog::Some. BTW: The "using namespace" directive is discouraged in the coding practices.
4. Transferring data between Inkscape and other apps. The new clipboard code is a step in the right direction (by the way - thanks for everyone for fixing the issues with it I weren't able to find), but the "import/export drawing data as some format" functionality should be split off into another function/object, so that the same code is used for importing and exporting via drag-and-drop, the Import and Export dialogs and the clipboard.
5. Memory leak problem (?). Inkscape is constantly eating memory even when idle. Is anyone aware of why is this happening? Also of note is the fact that creating a single rect can take up to 250 kilobytes.
6. Verbs. It looks like verbs.cpp is a reimplementation of Gtk::Action with some extra features. I don't know this part of Inkscape very well but it looks like we could create an Inkscape::UndoableAction deriving from Gtk::Action and use it instead of verbs. We could then use Gtk::UIManager to build menus and toolbars.
Please let me know if you have any suggestions / objections regarding those refactoring problems, and whether they make sense to you.
Regards, Krzysztof Kosiński.
On Tue, Oct 21, 2008 at 1:28 AM, Krzysztof Kosiński wrote:
codebase. It's also rather messy - for example, every tool has a gradient dragger but in reality it only makes sense for the Node and Gradient tools.
That was a joke, right? :-)
It's not messy, it's useful. Of course, if someone like switching between tools all the time just to get size of a rectangle and position of its gradient fill right, they are free to do so. But please please do not upset usability nerds among us :)
Alexandre
On Mon, Oct 20, 2008 at 6:28 PM, Krzysztof Kosiński <tweenk.pl@...400...> wrote:
- Tools tree. This is one of the last big chunks of GObject left in the
codebase. It's also rather messy - for example, every tool has a gradient dragger but in reality it only makes sense for the Node and Gradient tools. Converting this hierarchy to C++ would be a great step forward. It would also get rid of toolbox.cpp.
Gradient dragger does indeed make sense in many tools, not only in those two - or rather, its absense would make little sense. I agree that tools code and especially the gigantic toolbox.cpp may benefit from some refactoring, but gradient dragger is pretty orthogonal to all this and that's a good thing.
- Memory leak problem (?). Inkscape is constantly eating memory even when
idle. Is anyone aware of why is this happening? Also of note is the fact that creating a single rect can take up to 250 kilobytes.
I would vote for this one, as it gives the most immediate benefit to the users. I have recently done some work on this, mostly fixing ref/unref pairs on reprs and deleting closed documents from memory, but obviously more work is needed here.
- Verbs. It looks like verbs.cpp is a reimplementation of Gtk::Action with
some extra features. I don't know this part of Inkscape very well but it looks like we could create an Inkscape::UndoableAction deriving from Gtk::Action and use it instead of verbs. We could then use Gtk::UIManager to build menus and toolbars.
I think undoability is the property of XML tree and need not be introduced into verbs, which may represent any kind of action at all, be it changing the document or not. As far as I remember, right now we already can directly add verbs to toolbars, no?
bulia byak wrote:
Gradient dragger does indeed make sense in many tools, not only in those two - or rather, its absense would make little sense. I agree that tools code and especially the gigantic toolbox.cpp may benefit from some refactoring, but gradient dragger is pretty orthogonal to all this and that's a good thing.
Oops, obviously it makes sense in other tools too (like the shape tools). However, some tools might not need the dragger (like the paint bucket or tweak), so placing it at the second level of the class hierarchy (e.g. Tool<-GradientEditor<-Shape<-Rectangle) seems wise.
bulia byak wrote:
I think undoability is the property of XML tree and need not be introduced into verbs, which may represent any kind of action at all, be it changing the document or not. As far as I remember, right now we already can directly add verbs to toolbars, no?
I was a bit unclear. Inkscape::UndoableAction would be just a specific kind of Action. Gtk::Action or a derived class would replace Inkscape::Verb. The verbs that don't modify the XML tree would just derive from Gtk::Action, so verbs won't have to be undoable. There's also SPAction, but I have no idea what it does yet...
I know about the toolbars. Still, I think GTK and other libraries Inkscape depends on are preferred over custom code that does the same - in this case, Gtk::UIManager to generate menus and toolbars from XML descriptions and Gtk::Action to describe actions (verbs).
Regards, Krzysztof Kosiński.
On Oct 20, 2008, at 2:28 PM, Krzysztof Kosiński wrote:
- id-clashes.cpp - this is a stopgap solution at best...
Probably the #1 first step here would be to remove the legacy behavior of adding an id attribute on every object. They should only get added as needed, and be left out for the vast majority of elements.
- Tools tree. This is one of the last big chunks of GObject left
in the codebase. It's also rather messy - for example, every tool has a gradient dragger but in reality it only makes sense for the Node and Gradient tools. Converting this hierarchy to C++ would be a great step forward. It would also get rid of toolbox.cpp.
Converting to C++ is good, but it wouldn't get rid of the need for toolbox.cpp. It basically has a legacy naming now, but the general approach it currently has should probably be preserved. It is the place where the switch to dynamic UI and GtkActions has had the most traction.
- Namespace proliferation...
BTW: The "using namespace" directive is discouraged in the coding practices.
Yes, "using namespace..." is definitely bad, especially in .h files. However using a namespace{} block or "using" statements with specific classes is quite helpful.
- Memory leak problem (?). Inkscape is constantly eating memory
even when idle. Is anyone aware of why is this happening? Also of note is the fact that creating a single rect can take up to 250 kilobytes.
Please note that we have garbage collection as a factor, so eating memory may or may not be a leak. Tracking info down needs to keep this in mind.
- Verbs. It looks like verbs.cpp is a reimplementation of
Gtk::Action with some extra features. I don't know this part of Inkscape very well but it looks like we could create an Inkscape::UndoableAction deriving from Gtk::Action and use it instead of verbs. We could then use Gtk::UIManager to build menus and toolbars.
Actually Verbs and SPActions predate GtkAction itself. Verbs do a bit more than GtkAction, and there is some per-desktop tie-in to be considered. Work to move to standard GtkActions as much as possible has been ongoing for quite some time.
GtkUIManager *can* do dynamic UI, but its XML has taken the wrong approach. It's a very pixel-tweaking oriented approach, which is quite different from what we need (a more abstract representation). Oh, and toolbox.cpp has been converted to mainly use GtkUIManager internally. We've just not exposed the XML directly since it needs to be translated from a more suitable XML format that end users get to see.
Jon A. Cruz wrote:
- id-clashes.cpp - this is a stopgap solution at best...
Probably the #1 first step here would be to remove the legacy behavior of adding an id attribute on every object.
This is a good idea in itself, but I think it's rather loosely related to the original problem - all defs still need an ID if they are to be used anywhere, and it's the defs that cause the ID collision problems.
Converting to C++ is good, but it wouldn't get rid of the need for toolbox.cpp.
With an action-based UI, why not have virtual methods that register the actions and build the interface? Most of toolbox.cpp functionality would be split between the tool classes. The remaining few bits could be handled by the base class or a separate controller object. Probably the toolbox widget would need extensive modification but I believe it is possible to separate everything into independent tool objects.
Jon A. Cruz wrote:
Please note that we have garbage collection as a factor, so eating memory may or may not be a leak. Tracking info down needs to keep this in mind.
In the memory dialog the standard malloc value keeps increasing over time even when nothing is done. On the other hand, the libgc value doesn't change. Maybe the changing values are due to the reallocation of strings for display in the memory dialog and there is no leak at all?
Jon A. Cruz wrote:
GtkUIManager *can* do dynamic UI, but its XML has taken the wrong approach. It's a very pixel-tweaking oriented approach, which is quite different from what we need (a more abstract representation).
I don't know about any pixel-tweaking in GtkUIManager, or for that matter any presentational things except separators. (See UI Definitions in the link below). http://library.gnome.org/devel/gtk/stable/GtkUIManager.html
Regards, Krzysztof Kosiński
participants (4)
-
Alexandre Prokoudine -
bulia byak -
Jon A. Cruz -
Krzysztof Kosiński