* Michael Natterer [020423 13:08]:

Hi Rock,

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.

We should not add further tools, but drastically reduce their number by separating their ui from the core. 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.

If a general plug-in api for in-image preview should be implemented, I guess most ImageMap tools should be changed into standard distributed plug-ins. (since curves, levels, color-balance etc. could be considered core functionality)

A standardised way to do in image previews would be what those tools need to become plug-ins instead. Since they probably should be distributed together with the gimp anyways,.. nothing would stop them from sharing some common code amongst themselves.

My color-correction plug-in ( http://hal9001.2y.net/yuvadj/ which is now much more stable and usable since I announced it,. is from an users standpoint probably not be different in the way it works from levels/curves etc.

-pippin

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

On 25 Apr 2002, Michael Natterer wrote:

Nathan C Summers writes:

On 23 Apr 2002, Michael Natterer wrote:

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.

Sorry again, I know this is not the way communication should happen. My apologies for my lazyness.

Apology accepted.

Same here :) After reading you mail, I understand the way libgimpproxy is meant to work lots better than before and some of my concerns don't apply any more...

I hoped that that was the case. :)

Why do you think that your time is orders of magnitude more important than mine?

Why do you think I consider my time more important than yours? The mail I sent came ages too late, that's my fault.

Consider the ratio between how much time it takes to look over the architecture and how much time it takes to implement it. I know that you don't really think your time is more important than mine, but you have to admit that's how it looks.

One reason it took so long is that I just didn't know how to write it without pissing you off, which is definitely *not* what I want.

Hey, I understand that that is a hard skill to learn. :)

1.3 development would not be where it is now and we cartainly don't want to prevent valuable work to be done!

I hoped that was the case as well.

As mentioned earlier on #gimp I find all this possibilities utterly cool and I don't object at all to having this feature. I'm just *very* concerned about exposing these nasty object system deatils to a library. You're right that the performance thing is a non-issue, given that the user can choose to module-load the plug-in and that all GdkEvent related uglyness should and will be handled by the display shell callbacks.

Sounds good. I don't want ugly implementation details exposed any more than you do.

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.

Well the factoring out of core functionality is the main goal of 1.4, so of course one of the goals is to make all tools (read: their functionality) usable without even initializing GTK+, so without any tool involved. The lack of detailed communication is a problem I admit, but the overall direction was quite clearly outlined IMHO.

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.

Agreed.

I said in the last email that I had an idea. How about we rename the current GimpTool class GimpInternalTool. We then create a GimpToolUI class that can be inherited from and which works just like the GimpPaintCore system does now. GimpPaintCore and friends would inherit from GimpToolCore. Since the cores might not be in the core, it would be nice if we could come up with a different name, but nothing comes to mind. Perhaps someone with a thesaurus could help. :)

The GimpInternalTool class would know how to talk nicely to the GimpToolUI and GimpToolCore classes. When a ToolCore registers itself, it informs the core which GimpToolUI subclass it's associated with, so we could have, for instance, one GimpPaintUI class that serves as the interface to all the GimpPaintCores.

The whole point of my work is that core tools and plugin tools are implemented practically identically.

Which is a good thing to do, given the core/ui separation is kept intact. It should be possible to load e.g. a paint core module into a GIMP binary which does not even link against GTK+.

I agree.

I would strongly prefer to initialize the GimpToolControl struct with default values in it's constructor and override them explicitly when needed. This way we can add or remove some of the hacks (or non-hacks) without the need to change every place where gimp_tool_control_new() is called.

Sounds good to me.

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.

Huh? Sorry if that is your impression. I was just overly concerned that adding more complexity to the tool system in it's current state doesn't ease the task of cleaning it up. I have no prepared list of items what needs to be done for the tools. They used to work just by chance for a long time and often they don't work correctly at all (like cursor updating).

Perhaps in the future it would be good to create detailed lists of what needs to be done.

There currently is no paintcore module API/ABI, but creating a restricted API for paint cores is much easier than for tools because there is only the core involved, not core + tools + GTK. As I said above, my concerns do not apply the way they did before, see below...

Good point. That does restrict it a bunch.

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.

This is exactly the point that hurted me most. From my impression you were about to proxy all our internal object and object class *structures* in libgimpproxy...
...but obviously you exactly do *not* want to do this. Great :)

That would hardly be proxying. I'd have to call it libgimpcopy. :)

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?

I would do it the same way probably, whithout exporting any structs. (the nasty GimpObject issue ignored).

We might be able to get around the GimpObject issue by making sure that none of the classes whose structs are exported use GimpObject.

The point was not temporary breaking stuff or forcing people to develop huge portions of code off-tree. We were really afraid you were about to do things we considered plain wrong. As this is not the case, please just forget about the reverting idea.

Already forgotten. :)

Ok, then please let us restore the separation of app/core/ from anything tool related. Currently, many core/ files include libgimptool/ stuff because GimpToolInfo is defined there. Actually, GimpToolInfo is a core object, so no part of core/ should need to include it from libgimptool/, it should be proxied in libgimpproxy/

I'm trying to remember why I moved GimpToolInfo in the first place. It made sense at the time, but now I don't see any reason for it to be in libgimptool at all. It's had a nice vacation in libgimptool, but I think it needs to go back home now.

(actually, GimpToolOptions needs to be objectified as GimpContext subclass and be the parameter struct for tool core implementations. Tools then create a view on this model containing all the widgets).

OK, but let's do what we currently do with the GimpFooCore classes so that we don't have to include the structure definitions for GimpContext and everything it inherits from in libgimpproxy.

Also, the GimpChannel and GimpChannelClass structs are in libgimpproxy/, involving stuff like BoundSeg being exported.

No need for those structs to be exported. Just didn't tell mkproxy not to export them.

But these are details not related to the real reason for this thread :) I just have to mention what hit my eyes apart from the original misassumption...

Understand. It's quite productive, though. This last bit is exactly the kind of stuff that should be discussed on gimp-developer more often.

Keep on hacking!

Will do. You too.

Rockwalrus