On 23 Apr 2002, Michael Natterer wrote: > Hi Rock, > > sorry I did not comment on this earlier, I was quite busy since > returning from Guadec. After a few weeks of silence, I figured that you didn't have anything more to say, so I started commiting again. I have my own schedule, too. > As mentioned in an earlier mail from Sven, Really I got the impression from that email that you were concerned mostly about the few bugfixes that accidentally got overwritten. The fact that you didn't send me anything else for weeks later reinforced that. Resurrecting the bugfixes was fairly trivial, as was weeding out the few remaining segfaults. They would have been in weeks earlier had you simply asked for them. > we are not at all happy with the introducion of plug-in tools and the > ongoing exposure of the internal object structure outside the core. I understand some of your objections. What I entirely do NOT understand is why it has taken you so long to say anything concrete about my plans. I had heard some vague comments on irc, but nothing solid, and certainly nothing I didn't think I had resolved long before now. There is absolutely nothing you say here that you couldn't have said after looking at the plans I gave with the patch and two emails I sent to the list on the 23rd and 24 of Febuary, and very little of it couldn't have been said after looking at the original RFC I posted on December 17 of last year. After getting no response to any of those messages, I assumed that there were no objections. I have spent all of my free time working on tool plugins from the time I sent in the first RFC until the present date. Months have passed. I'm pretty busy with school and work, so I haven't had much time to work on it, but I've done practically nothing else with my free time. Yet I heard nothing from you before now, even when I solicited comments repeatedly. Why do you think that your time is orders of magnitude more important than mine? > While I have no doubt that it's implementable in a nice and working > way, I think it is the wrong way to go. Tools are an internal > implementation detail that is closely related to ugly things like > GdkEvents and fast response to user input. I don't find the tool class to be particularly ugly, myself. It is low-level, of course, but it seems to be pretty logical. GdkEvents have to be dealt with by any plugin author. They're hardly obscure or ugly. It would be nice if they were delivered in terms of image coordinates, of course, but other than that, pretty much everything in the GdkEvent is needed by the plugin. At the minimum, it needs to know location, pressure, and which shift buttons are being pressed. Of course, for special cases it doesn't need to know all of that, but for a general purpose api, that much is needed. It would be very easy to change the code to use something other than GdkEvents, certainly no harder than it was before. We've discussed response time before. I expect that most people will load most tool plugins into the core most of the time. But it's nice to have a mode where a crashing plugin doesn't crash everything, and that is only possible if the plugin is in a different address space. Response time, of course, will suffer, but response time isn't everything, especially when debugging. Thus people could try out plugins in a safe mode before trusting them, and developers could more easily do a compile-test-debug cycle without spending a lot of time restarting gimp. (Well, there is another way to shorten the edit-compile-debug cycle for tools. We could implement checkpointing and restore the checkpoint on SIGSEGV... But that would be a nightmare!) > We should not add further tools, but drastically reduce their number > by separating their ui from the core. I agree that better ui/logic separation is needed with the tools. That gives me an idea...but that is for a different email. > Then we make the factored out core code (their actual functionality) > accessible via both the PDB and a module API. This is close to be > finished for the paint tools. In the end, there will be *one* paint tool > with a variety of paint_core implementations, each of them optionally > featuring it's own tool options. > > The same holds true for the ImageMap tools, they will probably all > collapse into one single tool which simply uses the image_map modules > which are compiled in or loaded. This is the first I've heard of any of these plans. I knew you had factored out the PaintCore, but I didn't know about your idea of eventually unifying all of the paint tools into one descendant of the GimpTool class. It is almost impossible for the rest of us to make improvements to Gimp when you keep on making huge changes without telling the other developers about your plans. Please spend a bit of time sharing your plans with the rest of us -- after all, it's much less than what you expect from us. > The tool itself is an ugly piece of code which is particularly hard to > maintain in in't current state, so I really want to keep it an > internal detail. Nobody should be forced to subclass GimpFooTool just > to get new functionality in. The way to go IMHO is subclassing > GimpFooCore (e.g. GimpPaintCore) to get new functionality and simply > let it handle by the single GimpPaintTool. This is all fine and good when you are writing something that falls under a handful of specialized categories, but even a lot of the existing tools don't fall under nice categories. For example, the move tool is pretty much unlike all the other tools and should probably stay that way. As another example, I'd like to write a tool that allows the user to interactively resize a layer. It makes a good DrawTool, but other than that it is not really like any other tool. What I'm trying to say is that while separating logic and ui is a good thing, both logic and gui need to be pluggable. > So why should a plug-in author subclass GimpPaintTool if the gimp core > does not even subclass it itself. (note it does currently, but all > paint tool subclasses in current cvs just handle different tool > options). In that case, the author would just subclass GimpPaintCore just like the core does. (Cores that aren't in the core and tools that aren't Tools...maybe we need more words in the English language... :) The whole point of my work is that core tools and plugin tools are implemented practically identically. > Why whould somebody want to deal with > > GimpToolControl *gimp_tool_control_new (gboolean scroll_lock, > gboolean auto_snap_to, > gboolean preserve, > gboolean handle_empty_image, > gboolean perfectmouse, > /* are all these necessary? */ > GdkCursorType cursor, > GimpToolCursorType tool_cursor, > GimpCursorModifier cursor_modifier, > GdkCursorType toggle_cursor, > GimpToolCursorType toggle_tool_cursor, > GimpCursorModifier toggle_cursor_modifier); > > where all parameters are internal implementation details (read: hacks). > About 50% of these paramaters were added to get rid of nasty bugs > related to cursor updating I'm aware of the origins of the parameters that follow the comment (I've since removed the comment, which was referring to whether they were necessary in the constructor, not in general). Still, I find the meanings to be very logical and easy to understand. I don't find that call to be revolting at all, especially in comparison to, say, FormatMessage() from the Windows API. (apoligies for mentioning FormatMessage() in polite company. ;) I wouldn't be opposed to having gimp_tool_control set more of its guts to default values and using setter functions to change them as needed. This was just the easiest way for me to make sure that they were set correctly and in exactly one place. > and the whole tool system needs much more attention before it actually > works nicely. This is the kind of vague statement you often make that helps no one. What exactly needs to be done? When you see room for improvement, instead of saying, "no one touch this area of code because it could be made better," make public what needs to be done. Itemize it. Then anyone can work on making it better. We can not make the Gimp better if we don't know what needs to be done. When someone independantly commits something that in their opinion makes it better, you just revert it. It really does seem like you don't want any contributions. Reverting should only be done under catastrophic conditions. > We might even want to add more hacks to GimpTool and friends. We might > want to change the way the tools interact with the display and GdkEvents > and we may want to do this in a stable version if it is needed to fix > bugs. This is nothing we want to expose to a library. I agree that the GdkEvents shouldn't be exposed directly to the tool class. There is nothing in the current setup that would proclude making that change; in fact, it is no harder to do so than it was before. Just because the directory the file is in has changed does not mean that the file is any harder to change. After the 1.4 release, of course, we are more restrained in what we can change. I don't imagine that this will be too much of a limitation, but we can always add a few reserved entries to the end of GimpTool and GimpToolControl just in case. > Moreover, libgimpproxy does no good thing to our internal object > system. Exposing internal stuff to a library automatically submits it > to binary compatibility issues, which is definitely not what we > want. While it's easy to stay compatible to a reduced module API like > for paint and image_map methods, it's a pain to do so for a whole > object system, it's inheritance tree, it's properties, signals and > whatever. Are you really going to change the inheritance tree midway through the stable branch? That hardly seems like a "stable" thing to do. I strongly disagree with your assertion that the paintcore module ABI is reduced at in any form from the way things are currently done in cvs. A class that inherits from PaintCore has almost exactly the same dependancies as a class that inherits from GimpTool. > For instance, you moved GimpObject (including overly ugly crap like > it's "disconnect" signal) to libgimpproxy. I did not move GimpObject. It's still in app/core where it belongs. I talked about moving GimpObject, and in fact the Feb 24 patch moved GimpObject, but it has never been moved in cvs and was only moved in my personal copy for a few days. It was there before I wrote gimp-mkproxy, which was a much better solution. It's true that gimp-mkproxy does copy the structure definition over to libgimpproxy. Short of making all tool-related objects not inherit from GimpObject, there's not much that can be done to get around that. > Once tool plug-ins want to be more than just a proof-of-concept, they > will need access to GimpDrawable, GimpImage and so on objects, > submitting them all to "don't change me" ABI issues. Wrong. GimpDrawable, GimpImage, etc, add _exactly_ zero ABI issues. As you can see by reading my mail message dated Febuary 24, the structure definitions of those objects do not need to be copied over. They are just treated as pointers to opaque types. This is not speculation; I have all of them working in my local tree, and I've explicitly set gimp-mkproxy to not copy over the structure definitions. Only the prototypes for a few neccessary functions are copied over. In the off chance that these functions are changed, we can simply provide wrapper compatiblity functions. (this will actually reduce time since parts of the core can use the compatiblity functions where appropriate instead of being ported over.) gimp-mkproxy gives razor-sharp precision control over what is exported and what is not. I would understand your objection if all of the dirty underwear of every object even remotely related to tools was exposed, but that is not the case at all. Other than GimpObject, not a single object structure needs to be exported. Even for GimpObject, not everything needs to be exposed; it is currently, but that's because making it compile and run was more important to me than figuring out exactly what the minimum amount of functionality needed was. We could remove the dependance on GimpObject as well, but it's probably not worth the effort. Looking at app/paint/gimperaser.c for an arbitrary example, I see that it depends on GimpDrawable as well. How are you going to magically make this dependancy go away for your "clean module ABI" any way other than how I have? The tool plugin ABI has the potential to be much more constrained than the current module ABI that has been around since the 1.1 days. I don't really imagine that anyone will use libgimpproxy other than tool-safe-mode, but I don't see any harm in allowing for the possibilitly. tool-safe-mode, of course, is just a process that knows how to load modules and proxy requests to the gimp core. > I vote for creating a branch in CVS and reverting the tool plug-in > stuff in the HEAD branch, so we can finally get rid of the tool > overkill instead of adding the possibility to add new ones. Making a branch just pushes the burdon of updating the tool plugin mechanism entirely back onto me again. The whole reason I wanted it committed in the first place was not because it was finished but because I estimate I was spending 80% of my time keeping it in sync with the changes in cvs and only 20% of my very limited gimp hacking time actually working on the code. As you can see, even though I tried hard to keep things in sync, a few changes were lost because of CVS's poor behavior with regards to moving files. Placing it in a branch on the cvs repository is no different from storing it on my hard drive in this aspect. I simply do not see why it's impossible to make the changes you've discussed with the tool plugin code in place. I don't even see how it could possibly be any harder. None of the changes that you have speculated about making would be any harder at all with the tool plugin code, and try as I might I can't think of any changes that can be made to the tool api that would be more difficult because of the tool plugin code. The changes I have made, while touching many files, are not that great. Even big changes to the code I have written are not that hard -- we could make tool plugins that inherit from the paint module, for instance, without changing GimpToolModule at all and, if you are using code that has been on on my hard drive for weeks but yet uncommitted, without changing libgimpproxy or anything else. It simply doesn't make any sense at all to tack tool plugins on at the last thing. That will ensure that it be nothing more than a hack instead of a well designed feature. If we let it in now we have time to modify the tool api and the tool plugin api together as a whole to ensure that it is done right. Rockwalrus
