Re: [lmi] Code review: product editor

[Greg Chicares]

On 2007-3-26 13:48 UTC, Evgeniy Tarassov wrote:
> 
> AFAIU the current product editor branch is supposed to be disposed of

Okay.

> as soon as the changes are applied to the HEAD.

I'm working on applying them.

> Maybe we could simplify the process this way:
> 1) I send you some patches and do commits to a newly created branch
> 2) you revise the patches and decide what patches should be fixed or
> rewritten (like assigning accept/reject status for every patch)
> 3) the branch is disposed of
> 4) a new branch is created and I start fixing patches in the new branch
> 5) repeat the cycle

Sounds okay. Let me get through the current patches, and see how
this works. We're still exploring and trying to find the best way.

> This way we avoid the branch and HEAD being too different after couple
> of weeks.

Yes. We have to avoid that.

> The cycle could take a week or be even shorter.

I certainly hope we can make it shorter. But I have had many
distractions in the past few days.

Here's one thing I'm going to try doing a little differently.
When events in the office draw me away for a few days, I don't
remember every detail we've already discussed. It's inefficient
for me to retrace the entire path through an old message thread
and extract the conclusions later. As an experiment, where it
seems appropriate, I'm going to try putting more information in
inline comments, and copying them here for discussion. It may
be more efficient for me to update HEAD comments as we reach
conclusions here, as opposed to deferring that task until it
becomes difficult. It's just a limited experiment for now;
we'll see how well it works.

Right now I have some comments that we probably should consider
for "product editor: TNG" instead of trying to implement them
immediately. I've written them inline in HEAD so that they
won't get lost, and I'll copy them below for discussion.

[snip summary of some 'class database_tree_item_data' discussion]

However, shouldn't we rewrite this class completely instead?

Its purpose is to represent the information in struct db_names,
while deriving from class wxTreeItemData. Yet it contains only two
of that struct's four members--these:
   DatabaseNames       Idx;
   char const*         LongName;
but not these:
   DatabaseNames       ParentIdx;
   char const*         ShortName;
The fields not included are of course accessed by indexing a
db_names object from the vector returned by this function:
  std::vector<db_names> const& LMI_SO GetDBNames();
Even db_names::Idx is accessed that way, and I think our
discussions have raised the issue of whether that member's value
should be asserted to equal the value of the loop counter in
DatabaseView::SetupControls(). But why index by a sequential
loop counter instead of iterating across an available vector?
The former could break if enumerators ever fail to follow the
pattern 0, 1, 2, ... N-1; but iterating across a vector works
robustly without relying on any such assumption. And why have a
copy of 'LongName' here but not 'ShortName'?

I see two other designs to consider:

(1) This class holds only a DatabaseNames enum. That's enough to
find the corresponding struct db_names in the vector returned by
  std::vector<db_names> const& LMI_SO GetDBNames();
and we can then access that struct's members directly. And we
don't have to know that description() here is 'LongName' there.
Then the only thing we have to worry about is mapping between
that enum and wxTreeItemData::GetId().

(2) This class holds a db_names struct. To make that work, I guess
we'd have to rewrite that struct (I mean, char* members? in 2007?
Is that required for static initialization to work?) so that its
implicitly-defined member functions do the right thing, and hold a
copy of it here as the itemdata. (I don't know whether that'd make
populating the tree too slow.)

Or maybe even a third:

(3) Mixin programming:

  class database_tree_item_data
      :public wxTreeItemData
      ,public db_names
  {}; // Is any implementation actually required?

Is (3) an abuse of inheritance? AFAICT it satisfies the LSP.
I think it's either elegant or abhorrent; I'm not sure which.