Re: [lmi] product editor patch

[Greg Chicares]

On 2008-02-07 19:10Z, Vaclav Slavik wrote:
> 
> I submitted updated product editor patch here:
> https://savannah.nongnu.org/patch/index.php?6411

On 20080217T1517Z I committed most of these changes to HEAD. I'm
still so heavily involved in a taxation side-project that only
now, six days later, have I even found time to mention this--but
I think HEAD now has essentially all the product-editor changes.

The only exception is 'stratified_charges.[ch]pp', where I have
not yet folded in the last layer of error trapping. I approached
these two files cautiously because, unlike the other product-
editor changes in this patchset, they affect the production
system we distribute to end users. Indeed, they were associated
with the
  "Not all alert function pointers have been set."
issue recently discussed:
  http://lists.nongnu.org/archive/html/lmi/2008-02/msg00025.html
That issue demonstrates the value of the extra error checking you
added, which revealed two separate issues that had been lurking
elsewhere for years; it also shows why I take such a conservative
approach to changing files that affect the production system.

For those two files, I split the changes into seven separate
commits spanning [20080216T0321Z, 20080217T1513Z]. That's just my
usual way of understanding and reviewing code. It's exactly what
I do before committing code I've written myself, which often ends
up looking very different from my first fully-tested prototype,
so please don't take it amiss.

I believe I've incorporated every idea in your patch except the
extra error-trapping layer in read_entity() and write_entity().
There, I think I understand what you did and why you did it, but
I'm not yet sure that's the best possible place to do it--maybe
we should (someday, later on) look for a way to trap errors like
these in the GUI, but that's a decision we can defer.

Those are the only two files in the intersection between the
product editor and the production system. I committed the changes
to the product-editor-only subsystem with trivial modifications
only in a few spots, IIRC: those changes don't affect production,
so extreme caution isn't necessary; and they are obviously major
improvements.

> It's against CVS HEAD, so it may contain some unapplied 
> editor-unrelated patches

Yes: mostly small changes, to about seventeen files. I'm going to
keep them in abeyance for now, and focus on moving forward with
the product editor and a few other projects of my own.

Wendy and I have concluded that it's time to release the new
product editor to selected end users this month. It'll be good to
reach that milestone.

> -- let me know if you want me to edit them 
> out. There are still some problems with it; in particular, I'm aware 
> of the following ones:

I made my own list, without reading yours first; mine turned out
to be about the same as yours. Taking your points slightly out of
order:

> * MDI children don't enforce minimal size need to show all controls (I 
> only have unpolished version of this change locally, the fix should 
> be partially in wx).
> 
> * wxSpinCtrl controls in rounding editor are too wide (will be fixed 
> in wx, but not done yet).
> 
> * MultiDimGrid's axes selection controls change their position and 
> size slightly when you enable an axis, because the window's layout is 
> redone (needs small wx change or ugly workaround).

These layout issues are only nuisances, not obstacles that would
keep us from releasing the product editor. Only our most advanced
end users will be using it for now, and I don't think they'll be
concerned when we explain that these things will be fixed inside
wx within a few months. We don't need any workarounds meanwhile.

> Some of previously reported bugs were fixed either entirely or
> partially in wxWidgets and so the fixes won't be visible until
> wx-2.8.8:
>
> * automatic sizing of grid's columns/rows labels
>
> * scrolling in policy editor if the window is too small (TABing didn't
> scroll the control that has focus into view and scrollbars weren't
> considered when computing min size in earlier versions)
>
> * wxGrid indicates focus by using different colour for selection and
> hiding cell cursor when it doesn't have focus since 2.8.8

Same as above: we can wait a little longer for these cosmetic
refinements.

> * Bold labels with table's description near the top of database editor
> don't wrap and don't fit into the window if it's too small or the
> label too long.

Would it be easy to make TreeGridViewBase::grid_label_ a
multiline control? I don't think we'll ever need a string much
longer than this one:

"Ledger type: 0=illustration reg, 1=NASD, 2=prospectus illustration, \
3=group private placement, 4=offshore private placement, 5=private \
placement subject to illustration reg, 6=individual private placement, \
7=variable annuity"

whose 224 characters should fit on two or three lines. I think
it would be better not to use boldface here anyway.

> * There's views synchronizing code in documents classes. See here for
> PolicyDocument, but it's not limited to this:
> http://lmi.tt-solutions.com/codestriker/codestriker.pl?topic=3151189&action=view&mode=2&brmode=1&fview=-1

I think I'd rather apply only changes that you've reviewed
yourself. Even though I merged vast blocks of your product-
editor-only changes with hardly any alterations of my own, I
did read every single line, so I think you'd devise better
names for this pair of functions:
    void update_document() const;
    void update_from_document();
Adding
  #include <wx/version.h> // Mark this file as wx dependent.
to 'policy_document.hpp' prevents 3000+ lines of diagnostics,
BTW. For product-editor-only files, right now, I think the
right criterion for accepting a patch is that it's
 - better in at least one way, and
 - not remarkably worse in any way.
Please let me know if there are any other changes you'd like me
to apply now that meet that standard in your opinion, and I'll
probably commit them all right away. The criterion becomes much
more stringent once we put this in production; the code-freeze
date for this month's release can't be stretched much beyond
Monday.

At this point let me mention two other things I noticed:

* For msw, I thought the numpad '*' and '/' keys normally worked
like '+' and '-', but for all tree-control branches at once: i.e.,
"expand all" and "collapse all", somewhat as described here:

http://88.191.26.34/XYwiki/index.php/Tree

But when I tried that with a treeview control in ms "control spy",
only '*' had the effect I expected, and only when the root node
was selected, so I guess wx is just doing whatever msw normally
does. Nevertheless, if you think this would be a good enhancement,
then please feel free to add it.

* A '.db4' file opens with the tree control bottom aligned, with
its third (logical) item focused. I'd rather have it top aligned,
with the first item focused, unless that contradicts the way msw
normally does things. (Maybe this is observable only with the
800x600 resolution I use.)

But those two items aren't high priorities. Anyway, returning to
your list:

> * Axis checkboxes issue discussed on the ML.

That's not high on my list of issues. Let's disregard it for now;
see discussion of next item.

> * I think we can avoid dynamic_cast<> in 
> MultiDimAdjustableAxis<AdjustControl,BaseAxisType>::ApplyAdjustment 
> and RefreshAdjustment and axis_adjust_wins_ don't need to be 
> untyped "wxWindow*". I'm not happy with this code, I _think_ it can 
> be simplified, but it's not entirely trivial and I didn't want to do 
> it until I get more familiar with the code by working on more obvious 
> things first.

Let's not worry about that for now. That's a detail of execution,
but it's time to step back, take a fresh look, and ask whether
we've got the right design.

End users are still using a legacy product editor. They'll soon
be seeing the wx-based replacement for the first time, so they'll
look at each difference between the two GUIs and ask whether the
new one is really better. In many important ways, it is not.

Furthermore, I have to ask myself whether I can maintain the new
code. I compare the legacy program's 'ihs_dbview.cpp' to its
replacement spread across these files:
  database_view.cpp         multidimgrid_safe.hpp
  database_view.hpp         multidimgrid_safe.tpp
  database_view_editor.cpp  multidimgrid_tools.cpp
  database_view_editor.hpp  multidimgrid_tools.hpp
  multidimgrid_any.cpp      product_editor.cpp
  multidimgrid_any.hpp      product_editor.hpp
and I'm convinced that we made some fundamentally wrong design
decisions long before you became involved.

This is not in any way to discount the heroic efforts that you've
made to salvage this thing. That's a crucial achievement. Until
now, we've faced a bitter choice between maintaining the legacy
application (which is unrealistic--in one word, it's "OWL"), or
OTOH not adding any new field to the product files (which has
been extremely painful for the past three years). Now, that
obstacle can finally be removed: we can add new fields, and we
can also change the format of the underlying files--which means
we can expunge lots of ugly code like 'ihs*pios.?pp'. So we're
much better off today.

Going forward, I would say that we have two working prototypes,
each with its own advantages and disadvantages, and that the
right way to move forward is to start afresh.