[Octave-patch-tracker] [patch #9084] uitable implementation

[Mike Miller]

Follow-up Comment #1, patch #9084 (project octave):

I took a very brief look at this patch, I have not tried to apply or build it.
Some comments:

* Code formatting and style looks very good and consistent with existing
Octave coding standards
* Some long lines could be broken at operators
* Maybe the changes that expose some libinterp functions could be extracted
into a separate changeset
* Is it safe to call these libinterp number formatting functions from the Qt
thread? I haven't looked to see if these functions touch any interpreter
state
* Would one or more %!demo blocks be in order in addition to the @example
blocks in the doc string? There's no shame in duplicating a demo between
@example and %!demo, since they serve different purposes
* The NEWS file should announce uitable as a new function, either in its own
bullet point or in the "other new functions" list, at your discretion

Overall looks like a fairly clean patch, assuming it applies cleanly and
builds.

Have you tried it with both Qt 4 and 5 now that the default branch supports
build with either?

If you want to make this more likely to be reviewed quickly, maybe make some
more examples about how to use/test this function with a variety of different
arguments. Basically write a recipe for an interactive test that someone like
me who knows nothing about uitable can run through and verify that it works,
since it can't be automatically tested.

    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/patch/?9084>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/