Re: Connect gnuplot_x11 to exterior application window

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

On Saturday 08 April 2006 07:43 am, Petr Mikulik wrote:
> > I've updated the patch, 1027032, for connecting to the external X11 window 
> > against the most recent CVS version.

I've looked at it.
It is IMHO a long way away from being suitable for cvs.

> > [Is SourceForge updating the CVS version? 

Unfortunately, no.  SourceForge suffered a major failure on the main
server about 10 days ago. The anonymous cvs copy has been frozen at the
pre-failure state, and won't be updated until the integrity of the
main server has been restored.  As of last night, there were still
whole projects and disk areas off-line.  We seem to be fortunate
in that the gnuplot cvs area is back on line, and so far as I can
tell it has no corruption.  But it may be a while yet before any
changes to the main cvs tree are mirrored to the anonymous server.

> > This left off where Ethan couldn't get the TCL demo to run on his system. 
> > Ethan, please give it a try and let me know if it is work.

I applied the patchset, re-configured and rebuilt.
Then from the source directory I fired up
	../demo/gpdemos.tcl
This opened a wish UI window, but it would not respond to any mousing
or keystrokes. I couldn't select or type in a directory or file name
for display. So the wish file-browser widget seems to be broken.

I tried again in a different directory:
	cd ../demo
	./gpdemos.tcl

This partially worked.  It allowed me to select and display
files, and it echoed mouse cursor positions.  However, no other
mousing functions worked.  No zoom, no 3D click-and-drag, 
no anything involving mouse buttons, no hot keys.

This patch segment looks utterly wrong to me, and may be part of
the problem:

#ifdef EXTERNAL_X11_WINDOW
    XSelectInput(dpy, plot->window, event_mask);
    XSync(dpy, 0);
#else
    ProcessEvents(plot->window);
#endif



> It would simply be bloating CVS with a 
> bunch of conditionals for slightly different code.

No kidding!

The code needs a *lot* of cleanup. The conditional code is hugely
redundant and unnecessary.  This is similar to the ugliness in the
binary file code, and I would rather not see the same mistakes repeated
elsewhere.  Here is one small example of utterly pointless conditional
code:

Add_Plot_To_Linked_List(int plot_number)
{
#ifdef EXTERNAL_X11_WINDOW
    plot_struct *psp;
    if (plot_number >= 0)
        /* Make sure plot does not already exists in the list. */
        psp = Find_Plot_In_Linked_List_By_Number(plot_number);
    else
        psp = NULL;
#else
    /* Make sure plot does not already exist in the list. */
    plot_struct *psp = Find_Plot_In_Linked_List_By_Number(plot_number);
#endif


No conditional code *at all* is needed here. 
In fact no change to the current code at this site is needed.
Instead there should be a single line of error-checking in 

    Find_Plot_In_Linked_List_By_Number(int plot_number)
+      if (plot_number < 0) return NULL;

So instead of adding a one-line sanity check that should probably be
there already, this patchset adds 9 lines of conditional code that 
does nothing at all but make the source harder to read.

This sort of thing is present all through the patchset.

> The amount of extra code is actually small but the patch is big because 
> "plot" is changed to "current_plot" in a lot of places.

Exactly.  That is, it adds conditional code segments whose two
branches differ only in the name of one variable. 
Why do that? It just adds a ton of pointless conditional code that
doesn't accomplish anything useful.

At a rough guess, this patchset can be re-worked to achieve exactly
what it does now with only about one fifth the number of conditional
code segments.  Let's aim for that.

-- 
Ethan A Merritt
Biomolecular Structure Center
University of Washington, Seattle 98195-7742