Nathan C Summers <[EMAIL PROTECTED]> writes: > 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.
Sorry again, I know this is not the way communication should happen. My apologies for my lazyness. > > 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. Those were not the point, developing new things always breaks stuff temporarily. > > 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. (...) 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... > (...) 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? Why do you think I consider my time more important than yours? The mail I sent came ages too late, that's my fault. 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. 1.3 development would not be where it is now and we cartainly don't want to prevent valuable work to be done! > > 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!) 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. > > 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. 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. > > 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. 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+. > > 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. 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. > > 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). > 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. 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... > > 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... > > 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. ...but obviously you exactly do *not* want to do this. Great :) > 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). > 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. 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. > 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. > > (...) 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/ (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). Also, the GimpChannel and GimpChannelClass structs are in libgimpproxy/, involving stuff like BoundSeg being exported. 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... Keep on hacking! ciao, --mitch _______________________________________________ Gimp-developer mailing list [EMAIL PROTECTED] http://lists.xcf.berkeley.edu/mailman/listinfo/gimp-developer
