2009/12/29 Ted Gould <ted@...11...>:

The problem being that the action/verb system was never moved to C++ really -- so I think that this pattern is still appropriate.  Now, whether the code should be migrated is a different story :)

I remember it being discused here that Verbs and SPActions were invented before GTK had the Gtk::Action class, which does roughly the same. Migrating actions to C++ classes should also involve evaluating to what extent those new GTK features can be used.

Regards, Krzysztof

On Dec 29, 2009, at 1:36 PM, Ted Gould wrote:

The problem being that the action/verb system was never moved to C++ really -- so I think that this pattern is still appropriate. Now, whether the code should be migrated is a different story :)

True, but not quite what we do...

Checking one of the existing actions, I see

void FileVerb::perform(SPAction *action, void *data, void */*pdata*/)

First point is that the "data" void* pointer contains the enum value of the verb from verbs.h. (Technically we should be calling the glib routines to stuff an integer into the pointer, etc. but we're doing manual casts at the moment).

That enum value is then fed to a switch. Then the actual *user* data for that is the "pdata" parameter contains possible user data. However, we see from the helpful warning cleanup work that some oh so helpful person did (gee, I wonder who that was? :-) ) that the "pdata" parameter is not used anywhere, since it is commented out.

So I think Alex has hit up against a limitation of the current approach. One of the reasons we're not using that data pointer is that we don't have a simple way to keep the behavior consistent for the UI and command-line cases. This then extends into the scripting and D-Bus realm also...

... and speaking of command-line. I think that's the place for Alex to look. (next mail coming soon)

Oh, and yes, we *REALLY* need to get it migrated. I think the general plan to go with strings instead of enum values will help quite a lot.

The only problem with the current implementation is that it isn't possible to pass temporary data. All that needs to be added to the current code is to free the pdata pointer if it isn't null:

1. To trigger an action that passes data to the listeners: void *pdata = malloc(sizeof(data_struct)); data_struct *d = (data_struct *)pdata; data_struct->x0 = 4.0; data_struct->y0 = 5.5; sp_perform_action(ACTION_EXPORT_AREA, pdata);

2. sp_perform_action passes pdata to the listeners for the event, casting pdata to (data_struct *). If the listener needs to save some of the data, it makes a local copy. void action_export_area(..., void *pdata) { data_struct *d = (data_struct *)pdata; set_area(d->x0, d->y0, ...) }

3. After serving all the listeners, sp_perform_action calls free on pdata.

DBus is meant for data communication between processes. It uses strings to represent the format of the data that is being passed back and forth. This is way overkill for an event system in a single application, because it's possible to pass around void pointers and cast to the correct data structure in the handler.

- Alex

On Thu, Dec 31, 2009 at 12:34 AM, Jon Cruz <jon@...18...> wrote:

On Dec 31, 2009, at 12:05 AM, Jon Cruz wrote:

...

... and speaking of command-line. I think that's the place for Alex to look. (next mail coming soon)

So... checking on the command-line help, I noticed one command that looks promising:

-a, --export-area=x0:y0:x1:y1             Exported area in SVG user units (default is the page; 0,0 is lower-left corner)

That one takes four parameters that are coordinates. Could easily take four that were page margins, etc.

So, I did a search on the codebase for that string. I found hits in only main.cpp:

c$ find . -name '*.cpp' -exec grep -H 'export-area' {} ; ./main.cpp:    {"export-area", 'a', ./main.cpp:    {"export-area-drawing", 'D', ./main.cpp:    {"export-area-page", 'C', ./main.cpp:    {"export-area-snap", 0, ./main.cpp:            || !strncmp(argv[i], "--export-area-drawing", 21) ./main.cpp:            || !strncmp(argv[i], "--export-area-page", 18) ./main.cpp:        g_warning ("--export-use-hints can only be used with --export-id or --export-area-drawing; ignored."); ./main.cpp:        g_warning ("You cannot use --export-area-page and --export-area-drawing at the same time; only the former will take effect."); ./main.cpp:            g_warning ("EPS cannot have its bounding box extend beyond its content, so if your drawing is smaller than the page, --export-area-page will

Chasing that one with ""'s down, I see    {"export-area", 'a',     POPT_ARG_STRING, &sp_export_area, SP_ARG_EXPORT_AREA,

Aha! That tells us it is using the library support for parsing command-line options in a consistent manner. The parameters then are stored in that sp_export_area variable.

Doing a search, it can be seen as defined in main.cpp as static gchar *sp_export_area = NULL;

Uh oh. We see a bit of "evilness" sneak in where it is reinitialized: static void resetCommandlineGlobals() {

That's not so good, since global variables are in general not such a good thing. But we can ignore that for now...

Down in sp_do_export_png, we see that the string is parsed, used to set "area" which is in turn used to actually do the export of the image right there.

:-(

So it appears to not use the verb mechanism at all. Which means we're stuck for a "pretty" solution right off hand. Perhaps we will need to see how the DBus work is leveraging things.

So, Alex, what I think this shows is that you're not missing something obvious. However, Inkscape's internals appear to be, and really need to have that fixed.

2009/12/31 Jon Cruz <jon@...18...>:

yes... But the point of the verb mechanism is to support generic functionality decoupled from the input source it came from. These include:

• User clicking in the GUI
• Command-line invoking functionality
• DBus invoked functionality
• Live extension invoked functionality.

Using verbs as a quasi-binding to internal API feels wrong to me. We should have a clearly defined internal API decoupled from the UI which is usable from each of those contexts, then we wouldn't need to pass parameters to verbs or do any other kind of weird and ugly stuff.

I also note that "Command-line invoking functionality" is ambiguous. If this relates to scripting Inkscape, then that would be covered by DBus bindings and a convenience Python library that uses them. If it's about export options accessible from the command line, then the internal API could trivially be used to implement them.

I think a common problem we have in many places is that useful functionality is hidden in obscure functions with poorly defined interface, because whoever wrote them didn't have the courage to modify core classes, and instead tucked away his code in his own file. (I was severely affected by this when redoing the clipboard.) For example, resizing the page, pasting one SVG document into another, and removing unused defs and other invisible objects should all be methods of SPDocument.

Regards, Krzysztof

On Dec 31, 2009, at 1:56 AM, Krzysztof Kosiński wrote:

I also note that "Command-line invoking functionality" is ambiguous. If this relates to scripting Inkscape, then that would be covered by DBus bindings and a convenience Python library that uses them. If it's about export options accessible from the command line, then the internal API could trivially be used to implement them.

I think a common problem we have in many places is that useful functionality is hidden in obscure functions with poorly defined interface, because whoever wrote them didn't have the courage to modify core classes, and instead tucked away his code in his own file. (I was severely affected by this when redoing the clipboard.) For example, resizing the page, pasting one SVG document into another, and removing unused defs and other invisible objects should all be methods of SPDocument.

Not really.

I think one of the things causing you difficulties (aside from those in the current implementation itself) is from missing the concepts of encapsulation and modularity.

Although, yes, many functions might be more appropriately moved to methods on classes (that's a lingering C to C++ migration issue) there are some things that should be kept to an API.

The verbs system was intended for that purpose, and should really just be tuned to work better instead of being thrown out completely.

Also, the clipboard was severely broken in the recent refactorings. We probably need to revisit that as a first step. That gives a good chance for us to restore some of the modularity, remove some of the rogue tile bug patterns and get things moving forward.

Oh, and in the long run, remember that it has been a goal for a while now to expose things to scripting languages via a live DOM interface. The initial DBus work did not have time in just the summer of code to fully move to that, but that was stated as a follow-up refactoring.

2009/12/31 Alex Leone <acleone@...400...>:

The only problem with the current implementation is that it isn't possible to pass temporary data.  All that needs to be added to the current code is to free the pdata pointer if it isn't null:

1. To trigger an action that passes data to the listeners:

void *pdata = malloc(sizeof(data_struct)); data_struct *d = (data_struct *)pdata; data_struct->x0 = 4.0; data_struct->y0 = 5.5; sp_perform_action(ACTION_EXPORT_AREA, pdata);

This is very, very ugly. Verbs are not intended as a way to expose the internal API, and as such they aren't intended to take parameters. You should just remove the verb call, modify the function that resizes the page to take the extra margin parameters, and call it directly from the button callback. Don't forget to call sp_document_done() to add an undo stack entry.

Regards, Krzysztof