@Guillaume: I pushed that change with your user name here:
http://hg.savannah.gnu.org/hgweb/octave/rev/c27ecebf5b9b

Thanks for bringing this up.

Unrelated to uitable, I noticed that partypes doesn't contain "uipanel". I didn't knowingly use struct2hdl.m before. So I'm hesitant to make changes. But "uipanel" sounds like a valid parent for other graphics objects. Should it be added to that list?

@Guillaume: Do we need to do anything else apart from adding uitable to this list? Can you provide a patch please?

@Carnë: I moved the function here:
http://hg.savannah.gnu.org/hgweb/octave/rev/37e3aa267374

Thanks for the hint.

Also thanks Rik for cleaning up the new files.

I changed the demos to better comply to Octave's coding style and pushed here:
http://hg.savannah.gnu.org/hgweb/octave/rev/36e0e5b428e7

Thanks for providing this feature and to anyone who helped reviewing and testing.

Marking as ready for test.

@Markus:
How do the three patches relate to each other? In what order should they be applied?
Is it possible to make one complete patch from it?

My latest patch was file #44572, not #44579 that was by A.R. Burgers.

Anyway I hope this patch can be pushed soon. I've included it since ages ago in my builds and cross-builds.

One thing I'm wondering is if this patch (or uitable at large) doesn't duplicate functionality already in the Variable Editor (or better: the other way round).
Maybe in the future some code simplification and code deduplication is due.

The attached patches (part1 and part2) move the functions that Mike mentioned in comment #15 to lo-utils.cc. I also moved Fstr2double to utils.cc, effectively removing str2double.cc. Please let me know if they should better be moved to different files.

I also made the changeset from file #44579 into a complete Mercurial patch (part3) with Andrew Thornton as the author. Please let me know if this is not correct.

The implementation seems to be pretty complete apart from the smaller issues that are already mentioned in the comments before. IMHO, these can be tracked down with separate bug reports after the initial implementation was pushed.

(file #45472, file #45473, file #45474)

I had indeed mixed in an unrelated kron.cc change, apologies.
Attached a version without that part, but possibly superfluous.

Hoping for an inclusion of uitable in the distribution.

(file #44579)

... and then, to add to the confusion, I have kept this patch updated to the default branch, but didn't bother to upload yet as nobody seemed interested.
Attached are latest (relative to d1216d085280) and an earlier one from May 14 valid for the default branch at that moment, the latter maybe only relevant for historians :-)

I can't say I have used it much yet but I tried it with some code I found on the web and AFAICS uitable works fine.


(file #44571, file #44572)

You can drop that hunk of the patch completely, we are no longer setting default colors based on the Qt color palette for any widget properties.

issue the command sequence:

htable = uitable('Data', [8 9]);
set(htable,'ColumnEditable',[true true]);
set(htable,'ColumnFormat',{'numeric',{'1', '2'}});

then click the popup containing "9" with the mouse and change the value into 2. if that is done then execute following two commands:

set(htable,'ColumnFormat',{'numeric','numeric'});
set(htable,'ColumnEditable',[true true]);

the second cell (containig the 2) is frozen now and cannot be changed any more. but it should be editable as can be seen by issuing:

disp(get(htable,'ColumnEditable'));

i don't have much more time to play with it. my final conclusion about uitable is that almost all features (which are are really quite a lot) are implemented. coding style looks good. maybe needs some more thorough testing. so i also would like to see uitable in future octave releases.

Best regards Hg200

execute the command

uitable('ColumnEditable',true,'Data',[11 21; 12 22; 13 23],'CellEditCallback',@(h)disp(get(h,'Data')));

and modify cells in the second column. the output is scrambled. an entry at the position x=1;y=0 (where 21 is stored) overwrites the entry at position x=0;y=2 (where 13 is stored).

the reason is that at several places octave_idx_type is calculated incorrectly. as example at table.cc Table::itemChanged() we have

if (data.iscell ()){...}
else{
  data.fast_elem_insert (row + col * data.columns (), new_value);
  ...

but it should be

data.fast_elem_insert (row + col * data.rows (), new_value);

data.rows() gives the total number of rows of the table; row and col the actual position where we are. if we are in column n the index must be the actual column n total number of rows of the table (because we have to count n rows and not columns). hence i have changed all row + col xyz.columns () statements into row + col * xyz.rows ().

the second "help uitable" example is working much better now but x,y coordinates are still scrambled. however what ever is the reason the output is now identical to what matlab gives if i run the same code.

last year 2017 patch

(file #42774)

Sorry I lost track in comments #27 % 28 + a private email.

Please apply any changes, however tiny, into a new cset, preferrably a complete one or a tiny one to apply on top of file #42724.

As to the mCur_Data init - I haven't seen any crashes / segfaults yet and everything works AFAICS.

Philip,

"demo uitable" examples are working fine here and generating octave.pdf does not yield any warnings ;-((( Luckily i could find the warning somewhere else:

GEN      doc/interpreter/doc-cache
mk-doc-cache.pl: function '__go_uitable__ Undocumented.  ' may be undocumented at ./doc/interpreter/mk-doc-cache.pl line 133.

I am so sorry about missing that ;-(. So after some digging i found in ./libinterp/corefcn/graphics.cc line 11689 something very suspicious:

DEFUN (_go_uitable_, args, ,
       "-*- texinfo -*-\n\
@deftypefn {} {} _go_uitable_ (@var{parent})\n\
Undocumented internal function.\n\
@end deftypefn")
{
  GO_BODY (uitable);
}

Could you kindly change this part into

DEFUN (_go_uitoolbar_, args, ,
       doc: /* -*- texinfo -*-
@deftypefn {} {} _go_uitoolbar_ (@var{parent})
Undocumented internal function.
@end deftypefn */)
{
  GO_BODY (uitoolbar);
}

and start over a new make and check weather the warning disappears and the examples are working fine now? That was my fault. I could have seen this ...

Regarding fine tuning of the examples - I know. As an example the demo is running on one of my two laptops fine at the other laptop not. I also did compare with MATLAB. The reason is that in my second laptop the screen height of the display does not match the figure size in the example. But it is not such a big issue - therefore I like to postpone.

I am very afraid of this serious problem of the missing "m_curData" initialization. In my opinion it is a serious bug and i wonder how this was ever okay?!? There is an updateData(). I feel that is the right place to put an initialization but there are some other things i don't understand - hmm.

Greetings Hg200

Well - if we write m_curData after Table::Table() somewhere at line 444 by inserting "m_curData = octave_value(tp.get_data ());":

Table::Table (const graphics_object& go, QTableWidget* tableWidget)
    : Object (go, tableWidget), m_tableWidget(tableWidget), m_curData(),
      m_blockUpdates(false)
  {
    uitable::properties& tp = properties<uitable> ();
    m_curData = octave_value(tp.get_data ());
    Matrix bb = tp.get_boundingbox (false);

the problem dissappears.

However i am unsure about the consequences: m_curData is a private. Where is m_curData() coming from and at which point (file, line) is Table::Table() instantiated? I personally would expect m_curData should be set as the object is instantianated.

So for those who want to give it a try i created a patch from my local HG directory. This patch is identical to  patch_9084_uitable-20170102.diff but refers to HG ID 9a9b2e0596f5ab013b5180177d4d010934f58c7b. I havent included the above mentioned line because i cannot say wheather this a "bad-style" fix. The only contribution i made is the removal of the deprecated warnings by changing

is_complex_type -> iscomplex
is_bool_type -> islogical
is_numeric_type -> isnumeric
is_integer_type -> isinteger
is_empty -> isempty
is_cell -> iscell

The patch compiles with QT5 and QT4 without any warnings (Fedora 25).

Best regards HG200

(file #42724)

I merged the "file #39340:patch_9084_uitable-20170102.diff" to the current mercurial snapshot (9a9b2e0596f5). I did compile against QT5. Some deprecated warnings do occur (is_bool_type -> islogical,is_empty -> isempty etc.) but nothing harmful.

Because the author has asked for feedback, i can say that the appearance of this uitable patch is better than the uitable implementation in Matlab. In Matlab the slider position always resets as the table is modified what is really bothering and lots of people are complaining about that. To change this behaviour you have to use "undocumented Matlab". Here the slider remains at its actual position. Way better. Please don't change this behaviour and keep everything as it is.

I can say everything works fine (e.g. does not segfault, the two callbacks are okay etc.). As long as i set the table properties via set()

> f = figure();
> htable=uitable('ColumnEditable',true);  
> set(htable,'data',zeros(9,3));

everything is fine but if i create the table directly

> uitable('ColumnEditable',true,'Data',zeros(9,3));

what's happening is that as a cell is modified the cell disappears and a message "Table data is not editable at this location" appears at the promt.

I could track this down to table.cc
void Table::itemChanged (QTableWidgetItem* item)
octave_value data = octave_value(m_curData);

where we find

if (row < data.rows () && col < data.columns ())

The two  values data.rows () and data.columns () remain zero in the second initialization case but are set correctly to the table dimensions if the table is setup with set().

I am to unexperienced with octave but this looks to me not necessarily like a bug in Table.cc but maybe something has changed in the octave interface in the past year.

Best regards Hg200

Hi!

OK, so using the uitable-20161209.diff, I can't seem to replicate issue 1 on either QT4 or QT5 on linux. What versions of QT and on which platforms are you using? I noticed a missing break from one of the switch statements with the keys - and I've fixed this. I'll keep looking though.

2. I've think I've fixed the focus issue. I think simplest way to show this is:

figure('Name', 'FocusTest', ...
  'KeyPressFcn', @(x,y) disp('Figure'), ...
  'Position', [100 100 300 300])
uitable('ColumnFormat',{'numeric',{'one','two'}}, ...
  'ColumnEditable',true, ...
  'Data',{1,'one';2,'two'}, ...
  'Position', [50 50 172 64], ...
  'KeyPressFcn', @(x,y) disp('Table'))
uicontrol('Style','pushbutton', ...
  'String','Dummy', ...
  'Position', [100 200 100 100], ...
  'KeyPressFcn', @(x, y) disp('UiControl'))

Then just go around typing and see if focus is grabbed.

Now for 3. The KeyPressFcn results are the same as those produced by uicontrol etc. by octave. I know that this is not the same as matlab produces but I think it's out of scope for this changeset to fix this. (It would be a relatively simple change though: just add the two fields, one for the Source and one for a string representing 'KeyPress') However, I didn't do this for the uitable component in order to be consistent with the way uicontrol does this.

Similarly for ButtonDownFcn, the way octave does this is not compatible with way Matlab does this at the moment. I'm happy to implement this correctly for the uitable component but it's a little pointless if its not going to work for any other component properly.

I can add both those changes if they're required but, it's a question who to be consistent with: Matlab or Octave.

4. The checkbox centering with the QHBoxLayout seems to work well. Sorry I missed that - I'm sorry I missed what happens if you change the size by hand - it's such a shame that the Stylesheet trick doesn't do it, I thought it did do it - What platform are you using?

I've uploaded another version which should pull in against the current trunk - changeset 22925. I've not implemented point 3 as I'm not sure what to do and need your advice about it.

Andy

(file #39287)

Thanks to great works, Anonymous!
I tested this patch on 4.2, almost works fine.
But I found some problems, and fixed it.

I wish anonymous' patch to be merged, too.

1. Run following command, click first cell, push right key two times, then segfault occur.
     > uitable('ColumnFormat',{'numeric',{'one','two'}},'ColumnEditable',true,'Data',{1,'one';2,'two'});
   -> Stop KeyPress event propagation by returning true in the Table::eventFilter function.

2. Run following command, move mouse cursor over combobox, then focus stealed.
     > uitable('ColumnFormat',{'numeric',{'one','two'}},'ColumnEditable',true,'Data',{1,'one';2,'two'});
     > uicontrol('Style','pushbutton','String','Dummy');
   -> Stop FocusIn/FocusOut event propagation by returning true in the Table::eventFilter function.

3. KeyPress/MouseButtonPress event behaviors are different from MATLAB.
   -> Implement these event callbacks.

4. Checkbox is centerized, only when column size is initial value.
   -> Centerize checkbox using QHBoxLayout.


(file #39179)

I haven't had time to review the later revisions since I first looked at this. I agree that it will probably have to wait until after the 4.2 release at this point.

Looking again at str2double very briefly now, I am thinking it may make sense to move the str2double utility function into liboctave/util (and maybe the same for rational_approx).

Still have not had a chance to actually apply and use/test this patch yet.

Hey, so what's the status of this patch? Is there anything I can do to get it merged?

Actually importing the latest patch (re-d/lded) gives a number of failed hunks in a.o., Backend.cc, Canvas.cc, etc. due to various recent changes in dev Octave.

I do not know if the str2double item in module.mk is correct (Mike?) - it should be in the dist but probably not in the final installation.

If other reviewers more knowledgeable with Qt internals approve (see Mike's points 3 & 4 in comment #2) I think this cset can be pushed. It is a valuable addition IMO. It does work fine in Windows and Mageia linux.

Mike, who could say s/th on your Qt-related comments?

Thank you Andy, applies, builds and cross-builds fine and the examples (in the texinfo header) look nice.

A little glitch I forgot to mention as well is that str2double.h isn't included in the "make dist" target - some mention somewhere in libinterp/module.mk is required. I've added it manually to the dist archive to get Octave built.

Doesn't compile anymore after changeset 22296:8b18f46f6427 (revamp double, single, int type conversions) - complains about missing ov-type-conf.h that was deleted in above revision :-)

I mean Mike of course!

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.