patch for xfce4-panel

[edscott wilson garcia]

Now the long answer...

On Fri, 2004-01-09 at 05:14, Jasper Huijsmans wrote: 
> 
> I am wading through the code now to get a feeling for how it works.
> There are some things that I don't really like. 
> 
> First, the compilation should check for the combo module, not just
> libdbh. If there isn't a .pc file now, we have to make one.

A bit more detail on the matter. There is a .pc file. Xffm will not
compile if xfce4-modules are not found. If you want to make
xfce4-modules mandatory for the panel (like xffm), it's your choice, but
it does not have to be. It can be optional as in xfrun4. 

> 
> Secondy, I don't like *.i files. Why not have a xfcombo.h / xfcombo.c ?
> I think I will try to convert it and see how it works.

Just a bit easier code quickly. If you use the .h/.c approach you will
need to declare the "in terminal" checkbox as extern so that it will
turn on/off with the autocompletion history.

> 
> More importantly, I have trouble finding out the intended API for the
> combo module. 

I am guilty of that (lack of time to document). Basically the functions
the module exports are defined in the xfc_fun structure.  
 
> 
> Looking at combo.h it seems that most of these things are duplicated in
> xfcombo.i. Why?

Some duplicated code is that which loads/unloads the module, which
cannot be in the module because to reach it you need the load/unload
code which is in the module, which cannot be in the module because to
reach it you need the load/unload code which... ad infinitum... 

The other duplicated code is the "in term" flag stuff, which is not in
the module because the combo is not specific to running programs (see
other uses in xffm). 

> 
> The header also defines some functions only if __XFFM_COMBO_C__ is
> defined. Should these simply be static functions in combo.c or did you
> mean something else?
> 

Just an anti-mistake safeguard. Since it is a .i file, it should not be
included twice. If it is included twice by mistake, compilation will
proceed. If it is included by two different files that will be linked
together, well... compilation will fail. You can't win them all. ;-)
Changing to .c/.h scheme will modify this.

> It seems to me there are too many things exposed to the user that should
> just be in the module. Let me try to explain the common usage pattern,
> as I see it:
> 
> * open the module, find module_init function that initializes the module
> function pointers.
> 
> (hmm, perhaps this can even be done automatically, gmodule automatically
> runs an init function if available)

indeed, that would improve the usability of the code. 

> 
> * create an auto-completing combo widget with history (file).

No, the combo widget is not created. It should exist beforehand. What is
created are the signal callbacks and history file to do the
autocompletion. You will notice the creation of the combo widget within
the patch for item_dialog.c. This behaviour is intentional: any program
that already has a combo widget can use the combo module for
autocompletion history with least effort (at least that's the
intention;)

> 
> * add callback: when you do completion run this function with this data
> (e.g. checkbox settings)

Yes, you can add an extra callback to do extra autocompletion tasks,
like turning the "on terminal" checkbox on/off. There is also a flag for
"hold", which is handy for executing commands in xterm where you do not
want the window to close before you can read the output. 

Optional. You can also add an entry callback for an activate or cancel
response by part of the user. In item_dialog.c this feature is not used.
In xfrun4 the activate callback part is used, but not the cancel. In
xffm some combos use both the activate and cancel callbacks. 


> 
> * set this text

Or clear it. Optional. Best to use the module functions than writing to
combo->entry directly. But you can also write directly to combo->entry.
That is what item_dialog.c does, write directly.

> * (when closing dialog) get text

Optional. Only if using the activate or cancel callbacks and you define
them this way. This is not used in item_dialog.c. Instead the value is
directly retrieved from combo->entry. Much simpler in this case.

> 
> * add command to history

Optional. Used in xfrun4 and xffm, but not in item_dialog.c. Commands
should not be added to history if they fail to execute. So if the panel
adds commands to history it should be from a different file, where the
command is actually executed.

> 
> This is the only interface I need. Everything else should be hidden from
> the user. Did I miss anything?

You probably are looking at more than what you need for item_dialog.c
;-) (See "optional" remarks above)


> Thinking about this, it seems to me that the history and the completion
> are actually separate functionality. Somthing like this may be possible:

No. Completion and creation of the related items combo which
automatically expands are done on basis of the history.

> 
> XfceHistory *xfce_history_init (const char *dbh_file);
> 
> void xfce_history_add (XfceHistory *history, const char *command);
> 
> void xfce_history_remove (XfceHistory *history, const char *command);
> 
> void xfce_history_save (XfceHistory *history);
> 
> GList *xfce_history_get_list (XfceHistory *history);
> 
> 
> GtkWidget *xfce_completion_combo_new (GList *history_list, 
>                                       void (*completion_callback),
>                                       gpointer data);

The way you suggest is the way I started it but it proved insufficient
and overwhelmingly complex to use. You can probably still see the
functions you suggest stuck inside the module as static functions.

The idea is, you have a combo widget. You want to make it autocomplete
and create a related items list which automatically displays. All with
minimum effort. 

> 
> 
> There should be some API for saving/restoring flags as well, I guess.
> BTW, why are the run flags saved in a different dbh file?
> 

Because they are not part of the combo module. The combo module is also
used for "goto" history, "find filter", "display filter", and others
which have no need for flags nor extra completion callbacks. I thought
about including the flags field anyway, but that is just not practical. 

> Anyway, what do you think about that API? Is that something we want to
> work toward? 

That is already in there. Those static function should be exported from
the module to allow user to use them directly. They are not exported
because the module is meant to be "least effort" to use. And to ensure
the way the history and autocompletion is used remains congruent on all
programs making use of it (to give the impression of a coherent desktop
environment;)

> 
> I like sharing of functionality in the modules, but I think as it is now
> the API is not clear enough for people to easily use this.
> 
> What do you think? Am I wrong? Do you have different ideas?

The API needs to be documented, that's true. But do we want "people" to
use the module? Two or three developers would be more than enough for
me.  

Aside the xfcombo.i file (which could be transformed to .c/.h format)
the  lines of code needed to implement libxfce4_combo within
item_dialog.c amount to 16 (not including the HAVE_DBH compiler
directive nor comments). Sixteen lines of code to get autocompletion and
related popuplist based on history (shared across applications). 
Maybe we could get it down to 10, or 8. But is that worth the effort? As
Joe once said, "if it's not broken, don't fix it".

Yeah, documenting the API, .c/.h approach to using the module would be
good. Xffm uses the .c/.h approach for modules. See src/modules.c,h.    
Edscott

> 
> 	Jasper
> 
> 
> _______________________________________________
> Xfce4-dev mailing list
> Xfce4-dev at xfce.org
> http://lunar-linux.org/mailman/listinfo/xfce4-dev