Re: brainstorming SVG_INTERACTIVE

[Ethan A M. <merritt@u.washington.edu>]

On Tuesday 30 December 2008, Ethan A Merritt wrote:
> On Tuesday 30 December 2008, pl...@pi... wrote:
> > Ethan A Merritt wrote:
> > > On Tuesday 30 December 2008, pl...@pi... wrote:
> > >> OK I've added set key clickable/noclickable cleaned up a bit and done
> > >> trivial testing.
> > >>
> > >> Here's the patch. Though I probably missed something out. ;)
> > >>
> > >> Thanks to Bill for the idea and the perl hack to do it.
> > >>
> > >> regards, Peter.
> > >>
> > >> gnuplot_svg_interactive.patch
> > > 
> > > Please upload it to the SourceForge site:
> > > 
> > >    https://sourceforge.net/tracker/?group_id=2055&atid=302055
> > > 
> > 
> > Patches item #2477391, was opened at 2008-12-30 17:29
> > Message generated for change (Tracker Item Submitted) made by Item Submitter
> > You can respond by visiting:
> > https://sourceforge.net/tracker/?func=detail&atid=302055&aid=2477391&group_id=2055
> > 
> > 
> > It probably needs tidying up a bit but could you make sure it applies
> > OK? Made against currect CVS.
> 
> 
> First quick comments on the patch:
> 
> 1) Remove all the residual garbage in the patch file caused by comparing
>    the CVS "hidden" files in your own cvs setup.  As it stands, this patch
>    will not apply against anyone else's cvs tree.
> 
> 2) Conversely, the patch does not contain your new .../term/svg subdirectory.
>    For that you would need to include the -N switch in your "diff" command,
>    but it also means that you need to run it on a clean directory tree rather
>    than one that contains all the *.o files and modified CVS directories.
> 
> 3) The patch contains some changes that I assume you made for local
>    convenience (e.g. removing the docs directory from the build dependencies).
>    Please remove these.
> 
> For these various reasons, it doesn't actually apply and "make" correctly.
> I attach a somewhat cleaner version of your patch, but obviously it still
> doesn't contain any of the missing files.
> 
> As to the code itself:
> 
> 1) Please no C++ style comments.  
>    Not everyone is using a C compiler that accepts them.
> 
> 2) Compilation produces the following error messages:
> 
> In file included from term.h:368,
>                  from term.c:1368:
> ../term/svg.trm: In function ‘SVG_PathOpen’:
> ../term/svg.trm:257: warning: ISO C90 forbids mixed declarations and code
> ../term/svg.trm:261: warning: suggest parentheses around assignment used as truth value
> ../term/svg.trm:267: warning: suggest parentheses around assignment used as truth value
> ../term/svg.trm: In function ‘SVG_init’:
> ../term/svg.trm:752: warning: ISO C90 forbids mixed declarations and code
> ../term/svg.trm: In function ‘SVG_put_text’:
> ../term/svg.trm:1134: warning: suggest parentheses around assignment used as truth value
> ../term/svg.trm:1142: warning: suggest parentheses around assignment used as truth value
> 
> unset.c: In function ‘reset_key’:
> unset.c:864: warning: missing braces around initializer
> unset.c:864: warning: (near initialization for ‘temp_key.bounds’)
> 
> 
> 3) I don't get this "event handler" business.  
>    term_api.h:
> 	typedef struct  ext {
> 	  char *id;
> 	  char *event_handler;
> 	} ext ;
> 
>    This obviously is not an event handler in the normal unix sense of the term.
>    Please add a comment explaining exactly what it is, and how various terminal
>    drivers are supposed to use it.   
> 
>    Actaully, it doesn't seem to be used anywhere.
>    Is it a remnant from an earlier version?
> 
> 4) Please document the use of term->set_id_click(), term->move_ex(), and so on.
>    The description must be complete enough so that someone can code up the
>    corresponding routines for other terminals.  
>    It seems that you only ever pass in strings "legend#", "line#", and "sample#".
>    Do these have to be strings at all? Can they be defined constants?   Enums?   
> 
> 5) It would be good to include a demo script, so that people can test the
>    patchset immediately without having to learn the details first.


More comments:

 - "set key clickable" works, but the current setting needs to be reported by
   "show key" and saved by save_set_all().

 - Coding style.  Please make new code conform to the surrounding coding style.
   Rather than 

        /* Draw key text in black */
        (*t->linetype)(LT_BLACK);
+//*** add legend id and onclick
+  if ( (key->clickable) && (term->set_id_click) ) (term->set_id_click) ("legend#","line#");

   Please use

        /* Draw key text in black */
        (*t->linetype)(LT_BLACK);
+       /* add legend id and onclick */
+       if (key->clickable && (*t->set_id_click))
+           (*t->set_id_click)("legend#", "line#");

 - Thinking out loud here ...
   As I understand it, the curent patch associates the action specifically with
   the text of the key title.  What if the plot has no title?
   Maybe it would be better to associate the action with the rectangular block
   that bounds the key title and associated key sample.  That would at least
   hypothetically allow associated the action with an image-mapped *.png plot
   in addition to the more obviously interactive terminal types.

 - I'd like to hear more suggestions or brainstorming about a generalized
   framework.  This patch associates a specific action (toggle on/off) with
   a click on the key title.  My earlier href patch associated a different
   action (link to URL) with a click on the key title.  Another suggestion from
   the first round of comments was to also allow the click to trigger various
   other actions (highlight the plot, erase other plots, grey out the plot).
   
   All of these options sound reasonable, but it means that "set key clickable"
   is not sufficient to specify the desired result.  OK, fine, we could make
   that "set key click=toggle" or "set key click=URL".  But do we need a
   new set of terminal entry points for each option, or can we share a single
   entry that handles all of the various options that apply to that terminal?

   Maybe we need a structure that describes the desired actions, and a 
   corresponding set of keywords to instantiate one.

   struct action {
	int type;    /* toggle, cycle, highlight, hyperlink, ... */
        char *url;   /* NULL unless action type is hyperlink */
        t_colorspec highlight_color;
   }

   struct legend_key {
	...
	struct action *action;
   }

      # default action is to toggle the plot on/off
        set key title action TOGGLE  
      # but individual plots can do something else, 
      # e.g. cycle grey/off/on/highlight 
        plot foo title "foo" action cycle, 
             baz title "baz" action link "file:///data/baz.dat" 



-- 
Ethan A Merritt