I'm working through creating a button in the Document Properties dialog to fit the canvas around the current selection or the full drawing. You can read a diff of my changes at:
http://www.ekips.org/comp/inkscape/inx/current.diff
I'm at the point now where I would like to hook the function up to the button. The question is how to best do this.
Do I create a Verb and hook it up through there? If so, where can I find an example of how to use a verb with a button on a dialog? I noticed that very few functions in Inkscape actually get to be Verbs. Should there be more verbs? From where should a verb get access to the active desktop/document?
Should I use sigc::bind() to pass the active desktop to the function directly in the dialog code? If so from where should I get a pointer to the current desktop? While I was looking around document-properties.cpp for a pointer to the active desktop, I found "static void on_activate_desktop (Inkscape::Application *, SPDesktop* dt, void*);". Some of the params aren't named. And while the desktop is named dt, it is never used. SP_ACTIVE_DESKTOP is used instead. Is SP_ACTIVE_DESKTOP evil?
Needless to say, I'm a little confused and I don't know what questions to ask to get the answers. I just want to hook my little button up the right way.
Aaron Spike
On 4/17/06, Aaron Spike <aaron@...749...> wrote:
Do I create a Verb and hook it up through there?
Yes, I think it's better to do this a verb.
If so, where can I find an example of how to use a verb with a button on a dialog?
I'm not sure about dialogs - looks like we didn't have it so far. But on toolbars, see sp_toolbox_button_new_from_verb in widgets/toolbox.cpp
I noticed that very few functions in Inkscape actually get to be Verbs. Should there be more verbs?
Absolutely. The test is simple: if a user may ever want to assign a key to the command or have it in the menu, it should be a verb. Also, if any other vector editor has a key for a similar action or has it in a menu, make it a verb as well.
From where should a verb get access to the active desktop/document?
They extract it from the action itself in one of the *::perform functions in verb.cpp. For example:
void EditVerb::perform(SPAction *action, void *data, void *pdata) { SPDesktop *dt = static_cast<SPDesktop*>(sp_action_get_view(action)); if (!dt) return;
switch (reinterpret_caststd::size_t(data)) { case SP_VERB_EDIT_UNDO: sp_undo(dt, SP_DT_DOCUMENT(dt)); break;
Should I use sigc::bind() to pass the active desktop to the function directly in the dialog code?
Hmm, here's a problem. You pass the desktop (i.e. view) when you get the action from verb, and you then make a button with that action. For example in toolbox.cpp:
sp_toolbox_button_new_from_verb_with_doubleclick(GtkWidget *t, Inkscape::IconSize size, SPButtonType type, Inkscape::Verb *verb, Inkscape::Verb *doubleclick_verb,
Inkscape::UI::View::View *view, GtkTooltips *tt) { SPAction *action = verb->get_action(view); if (!action) return NULL; ... GtkWidget *b = sp_button_new(size, type, action, doubleclick_action, tt);
This works for toolbar buttons because they are always on the same fixed desktop. But dialog buttons may apply, with their dialog, to different desktops. Do we have to recreate the action on each desktop switch (sensed by the dialog) and reassign that new action to the button? Mental, can you suggest a solution?
Is SP_ACTIVE_DESKTOP evil?
Not by itself. You just need to know where it's safe to use. For example within verbs you should avoid it (though currently it's still much used in functions called by verbs, this needs to be fixed). This is because verbs can at some time be called by scripts or in some weird situations where there's no current desktop, or where you want to act on a non-current one. But in e.g. dialog which uses it to find out which desktop is active, SP_ACTIVE_DESKTOP is perfectly OK.
-- bulia byak Inkscape. Draw Freely. http://www.inkscape.org
On 4/17/06, Aaron Spike <aaron@...749...> wrote:
bulia byak wrote:
if (!dt) return;Please contrast the above with "g_return_if_fail(!dt);".
If there's a difference in behavior, I don't know about it.
-- bulia byak Inkscape. Draw Freely. http://www.inkscape.org
bulia byak wrote:
On 4/17/06, Aaron Spike <aaron@...749...> wrote:
bulia byak wrote:
if (!dt) return;Please contrast the above with "g_return_if_fail(!dt);".
If there's a difference in behavior, I don't know about it.
Well I think I have the condition flipped, but... :-)
I'm wondering if g_return_if_fail() should be avoided because it is a glib thing. Like I've been told to use regular bool rather than gboolean types. It doesn't seem to save any key strokes either.
Aaron Spike
On Mon, Apr 17, 2006 at 10:30:57AM -0500, Aaron Spike wrote:
bulia byak wrote:
On 4/17/06, Aaron Spike <aaron@...749...> wrote:
bulia byak wrote:
if (!dt) return;
Please contrast the above with "g_return_if_fail(!dt);".
If there's a difference in behavior, I don't know about it.
Well I think I have the condition flipped, but... :-)
I'm wondering if g_return_if_fail() should be avoided because it is a glib thing. Like I've been told to use regular bool rather than gboolean types. It doesn't seem to save any key strokes either.
In code which doesn't otherwise use any gtk or glib functionality, it'd make sense to not use this. On the other hand, if the code is heavy with gtk calls, like the UI code often is, then it doesn't seem like it'd hurt to use the glib style.
The important thing is to be consistent; e.g., all files implementing, say, dialogs really ought to use just one approach or the other, and use it consistently throughout.
Bryce
On Mon, 17 Apr 2006 10:32:56 -0700, Bryce Harrington <bryce@...961...> wrote:
On Mon, Apr 17, 2006 at 10:30:57AM -0500, Aaron Spike wrote:
bulia byak wrote:
On 4/17/06, Aaron Spike <aaron@...749...> wrote:
bulia byak wrote:
if (!dt) return;
Please contrast the above with "g_return_if_fail(!dt);".
If there's a difference in behavior, I don't know about it.
Well I think I have the condition flipped, but... :-)
I'm wondering if g_return_if_fail() should be avoided because it is a glib thing. Like I've been told to use regular bool rather than gboolean types. It doesn't seem to save any key strokes either.
In code which doesn't otherwise use any gtk or glib functionality, it'd make sense to not use this. On the other hand, if the code is heavy with gtk calls, like the UI code often is, then it doesn't seem like it'd hurt to use the glib style.
g_return_if_fail() prints a warning on the console before returning. Usually it is used for testing preconditions whose failure doesn't warrant aborting execution.
I would recommend using it in any code where "weak" precondition testing is desired.
Incidentally, the equivalent of
if (!dt) return;
is:
g_return_if_fail(dt);
or
g_return_if_fail( dt != NULL );
Note the inversion.
-mental
On Apr 17, 2006, at 8:16 AM, bulia byak wrote:
This works for toolbar buttons because they are always on the same fixed desktop. But dialog buttons may apply, with their dialog, to different desktops. Do we have to recreate the action on each desktop switch (sensed by the dialog) and reassign that new action to the button? Mental, can you suggest a solution?
One general solution to decoupling this safely (and one that The GIMP uses), is to have things hooked to a "Context" object. This works as basically a mediator pattern.
When an action is created, it can be created to the Context instead of a Desktop directly. Then the Context follows the current Desktop (either as things are switched, or upon actual use by calling sp_active_desktop) and passes things along.
This context object could be built into the dialog class itself, or could be something separate. Also... it could be a separate class, or might just be a collection of aspects of code in the dialog itself. it doesn't matter quite as much how it's coded, just that this is the abstract design concept that would be involved.
Oh, one other thing to do would probably be to track the change of active desktop as it happens. Then the appropriate buttons in the dialog could be disabled and enabled as appropriate. In Aaron's case here that would involve a simple check on desktop switch to see if a non-empty selection is present. Then if so, the Context or dialog would disable or enable the proper Actions and the UI would do the helpful thing.
On 4/17/06, Jon A. Cruz <jon@...18...> wrote:
One general solution to decoupling this safely (and one that The GIMP uses), is to have things hooked to a "Context" object. This works as basically a mediator pattern.
When an action is created, it can be created to the Context instead of a Desktop directly. Then the Context follows the current Desktop (either as things are switched, or upon actual use by calling sp_active_desktop) and passes things along.
I know you like the "context" idea. But please compare your description above with the current situatuion: we have a global Application object which follows the current desktop and passes it along whenever you call SP_ACTIVE_DESKTOP. So what's the difference?
-- bulia byak Inkscape. Draw Freely. http://www.inkscape.org
On Mon, 17 Apr 2006 13:40:48 -0300, "bulia byak" <buliabyak@...400...> wrote:
I know you like the "context" idea. But please compare your description above with the current situatuion: we have a global Application object which follows the current desktop and passes it along whenever you call SP_ACTIVE_DESKTOP. So what's the difference?
Mainly that the fact is currently buried behind the implementation of inkscape_active_desktop(). But I also hope that there would no longer even be a global accessor to get "the" application object. Ideally, desktop objects would be told (and remember) the application object which created them.
The goal is that -- rather than obtaining anything from global functions or variables -- everything possible is either remembered in a member, or preferably passed directly as an argument. There are a few places where using global accessors is reasonable, but the problem is that in the remaining 95% of cases, they have proven too temping for people to use in place of passing arguments. End result: code becomes untestable and unscriptable.
I don't see a strong case for a context object separate from the application object (yet), but I could forsee us factoring some fields out of the application object in the future depending on what the needs of in-application scripting turn out to be.
-mental
On Apr 17, 2006, at 8:16 AM, bulia byak wrote:
On 4/17/06, Aaron Spike <aaron@...749...> wrote:
Is SP_ACTIVE_DESKTOP evil?
Not by itself. You just need to know where it's safe to use. For example within verbs you should avoid it (though currently it's still much used in functions called by verbs, this needs to be fixed). This is because verbs can at some time be called by scripts or in some weird situations where there's no current desktop, or where you want to act on a non-current one. But in e.g. dialog which uses it to find out which desktop is active, SP_ACTIVE_DESKTOP is perfectly OK.
Thanks, Bulia.
This is a very helpful description of the situation and the best practices to use for it.
On Mon, 17 Apr 2006, bulia byak wrote:
Absolutely. The test is simple: if a user may ever want to assign a key to the command or have it in the menu, it should be a verb. Also, if any other vector editor has a key for a similar action or has it in a menu, make it a verb as well.
I like this. What functions aren't verbs that should be? Seems we have a ton of verbs...
They extract it from the action itself in one of the *::perform functions in verb.cpp. For example:
void EditVerb::perform(SPAction *action, void *data, void *pdata) {
I'd like to mention here that I really don't like that we have a few subclasses of Verb with case statements in the perform functions. I did this (so I can bitch about it) mostly so that the diff when I changed everything to C++ would be understandable. But, with new verbs, I would strongly encourage them getting their own subclass of Verb if possible.
As and interesting side note (I've been meaning to mention, but keep forgetting, so I might as well now) all Effects have verbs. So, if someone is editing the menus or keybindings you should be able to just use the extension ID (ie org.inkscape.effect.bluredge) as the Verb ID to put it somewhere. I imagine this might be most useful for emulating other apps, where their standard functionality or keypress is implemented as an effect.
--Ted
On 4/17/06, ted@...11... <ted@...11...> wrote:
As and interesting side note (I've been meaning to mention, but keep forgetting, so I might as well now) all Effects have verbs. So, if someone is editing the menus or keybindings you should be able to just use the extension ID (ie org.inkscape.effect.bluredge) as the Verb ID to put it somewhere. I imagine this might be most useful for emulating other apps, where their standard functionality or keypress is implemented as an effect.
Cool, i didn't know that. I'll mention it in comments in the keyboard profile.
-- bulia byak Inkscape. Draw Freely. http://www.inkscape.org
On 4/17/06, ted@...11... <ted@...11...> wrote:
As and interesting side note (I've been meaning to mention, but keep forgetting, so I might as well now) all Effects have verbs.
I tried assigning a key to effect and it works, but the key is not shown in the menu.
-- bulia byak Inkscape. Draw Freely. http://www.inkscape.org
On Mon, 17 Apr 2006, bulia byak wrote:
On 4/17/06, ted@...11... <ted@...11...> wrote:
As and interesting side note (I've been meaning to mention, but keep forgetting, so I might as well now) all Effects have verbs.
I tried assigning a key to effect and it works, but the key is not shown in the menu.
Hmm, that's really odd. Mental do you have any ideas on this? I'm guessing it must be and ordering of when extensions are initialized versus when the key bindings file is read? The menu entires for the extensions are actually the same XML, and processed at the same time as the other menu entries, so they should be identical. I'm guessing there may be something with the keybindings being read before the extensions are initialized?
--Ted
On Mon, 2006-04-17 at 14:23 -0500, ted@...11... wrote:
Hmm, that's really odd. Mental do you have any ideas on this? I'm guessing it must be and ordering of when extensions are initialized versus when the key bindings file is read? The menu entires for the extensions are actually the same XML, and processed at the same time as the other menu entries, so they should be identical. I'm guessing there may be something with the keybindings being read before the extensions are initialized?
There really shouldn't be any problems, as far as that goes... however, if the extension shortcuts aren't getting registered with display="true" (the is_primary parameter of sp_shortcut_add), their shortcut won't get displayed in the menu.
-mental
On 4/19/06, MenTaLguY <mental@...3...> wrote:
There really shouldn't be any problems, as far as that goes... however, if the extension shortcuts aren't getting registered with display="true" (the is_primary parameter of sp_shortcut_add), their shortcut won't get displayed in the menu.
Oops! Indeed it looks like I forgot the display="true". With it, everything shows perfectly. Sorry for the false alarm.
-- bulia byak Inkscape. Draw Freely. http://www.inkscape.org
Thanks for all of the replies, but I'm having a little trouble wading through all of the information. It sounds like there are still missing pieces that remain to be coded. What is my next move?
Aaron Spike
Aaron Spike wrote:
I read through all of the advice and explanation here and on jabber multiple times and I now think I understand the suggestions. I've updated the diff above to my current state. Unfortunately 'g_message("fire_fit... no action");' gets executed. Under what circumstances would I not get an action back from a verb when requested?
Thanks, Aaron Spike
SP_VERB_SHOW_LICENSE exists in verbs.h but not in verbs.cpp. Is it coming or going?
(Thank you Jon and mental)
Aaron Spike
On 4/18/06, Aaron Spike <aaron@...749...> wrote:
SP_VERB_SHOW_LICENSE exists in verbs.h but not in verbs.cpp. Is it coming or going?
I think it needs to go - we had this command before but now the license is incorporated into the About screen.
-- bulia byak Inkscape. Draw Freely. http://www.inkscape.org
Aaron,
the button works fine, but just a few small suggestions:
- I think you can drop "current" from the button label
- Please add a tooltip (mentioning that with no selection, it fits to drawing)
- I'd prefer the button to not stretch to all available width
- Please fix the critical assert when it's pressed with no drawing - it must do nothing but not assert
- You know the mantra: Release Notes :)
Thanks!
-- bulia byak Inkscape. Draw Freely. http://www.inkscape.org
bulia byak wrote:
the button works fine, but just a few small suggestions:
- I think you can drop "current" from the button label
done.
- Please add a tooltip (mentioning that with no selection, it fits to
drawing)
done.
- I'd prefer the button to not stretch to all available width
I'd prefer you tell me how. ;-) I tried shrink_wrap_button() because it had a promising name, but it didn't seem to do anything. I even had to use a cast to get it to compile. shudder. If you can point me to a doc or an example that would help.
- Please fix the critical assert when it's pressed with no drawing -
it must do nothing but not assert
done.
- You know the mantra: Release Notes :)
pending.
Aaron Spike
On 4/19/06, Aaron Spike <aaron@...749...> wrote:
I'd prefer you tell me how. ;-) I tried shrink_wrap_button() because it had a promising name, but it didn't seem to do anything. I even had to use a cast to get it to compile. shudder. If you can point me to a doc or an example that would help.
I think I didn't find any way to do it either except setting fixed pixel size (eeeek, as JonCruz would say). Anyone has a better idea?
-- bulia byak Inkscape. Draw Freely. http://www.inkscape.org
On Wed, 19 Apr 2006, bulia byak wrote:
On 4/19/06, Aaron Spike <aaron@...749...> wrote:
I'd prefer you tell me how. ;-) I tried shrink_wrap_button() because it had a promising name, but it didn't seem to do anything. I even had to use a cast to get it to compile. shudder. If you can point me to a doc or an example that would help.
I think I didn't find any way to do it either except setting fixed pixel size (eeeek, as JonCruz would say). Anyone has a better idea?
I think this is a limitation of the 'attach_all' function. It is attaching all the widgets into the table with 'Gtk::FILL|Gtk::EXPAND'.
I guess you'd either have to attach the widget to the table by hand, or modify the function to take another array of parameters.
--Ted
participants (6)
-
unknown＠example.com -
Aaron Spike -
Bryce Harrington -
bulia byak -
Jon A. Cruz -
MenTaLguY