patchGNU Octave - Patches: patch #9084, uitable implementation

 
 

patch #9084: uitable implementation

Submitter:  None
Submitted:  Thu 11 Aug 2016 07:43:44 PM UTC
   
 
Category:  Core : new feature Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  None Originator Email:  -email is unavailable-
Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 12 Dec 2018 07:46:41 PM UTC, comment #54: 

I'm closing this report as done. If some issues should arise, new bug reports can be filed or this report can be re-opened.

Markus Mützel <mmuetzel>
Group administrator
Fri 30 Nov 2018 06:09:37 PM UTC, comment #53: 

@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?

Markus Mützel <mmuetzel>
Group administrator
Wed 28 Nov 2018 04:03:09 PM UTC, comment #52: 

It is just adding "uitable" to the list:


diff -r 64b972120ec3 scripts/plot/util/struct2hdl.m
--- a/scripts/plot/util/struct2hdl.m    Wed Nov 28 09:47:35 2018 +0100
+++ b/scripts/plot/util/struct2hdl.m    Wed Nov 28 16:01:18 2018 +0000
@@ -167,7 +167,7 @@
     [h, s, p] = createhg (s, p, par, hilev);
   elseif (any (strcmp (s.type, {"uimenu", "uicontextmenu",...
                                 "uicontrol", "uipanel", "uibuttongroup",...
-                                "uitoolbar", "uipushtool"})))
+                                "uitoolbar", "uipushtool", "uitable"})))
     if (isfield (s.properties, "extent"))
       s.properties = rmfield (s.properties, "extent");
     endif


Guillaume <gyom>
Tue 27 Nov 2018 08:46:42 AM UTC, comment #51: 

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

Markus Mützel <mmuetzel>
Group administrator
Mon 26 Nov 2018 12:24:37 PM UTC, comment #50: 

Very pleased to see uitable implemented. It should be added in the list of known objects in struct2hdl.m:

http://hg.savannah.gnu.org/hgweb/octave/file/804e18e3e320/scripts/plot/util/struct2hdl.m#l170

Guillaume <gyom>
Thu 22 Nov 2018 09:21:26 PM UTC, comment #49: 

@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.

Markus Mützel <mmuetzel>
Group administrator
Thu 22 Nov 2018 12:15:00 PM UTC, comment #48: 

About https://hg.savannah.gnu.org/hgweb/octave/rev/b543cf12c63f Instead of naming the new liboctave function octave_str2double, could we make use of the octave namespace? All other functions in the oct-string.h header are in the the octave::string namespace too.

Carnë Draug <carandraug>
Group Member
Wed 21 Nov 2018 06:55:07 PM UTC, comment #47: 

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 Mützel <mmuetzel>
Group administrator
Mon 19 Nov 2018 01:54:03 PM UTC, comment #46: 

They apply in the order:
1st: part1
2nd: part2
3rd: part3

I'd prefer keeping the movement of the existing functions separate from the actual implementation of uitable.

Markus Mützel <mmuetzel>
Group administrator
Mon 19 Nov 2018 01:49:52 PM UTC, comment #45: 

@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?

Philip Nienhuis <philipnienhuis>
Group Member
Sun 18 Nov 2018 04:40:25 PM UTC, comment #44: 

@HG200: Thanks for reviewing the patches.

Your 1.) should be fixed with the attached updated part 3.

As to your 2.): liboctave is supposed to be a "more generic" library and independent from the interpreter. Thus, if octave_str2double moves to liboctave, its helper functions have to move there as well.
As to which exact files they should recide in, I guess Fstr2double is better being moved to strfns.cc.
The parts that move to liboctave probably better fit in the corresponding(?) file oct-string.cc.
Please let me know if these aren't the correct places for these functions.

Please see the attached updated patches.

@Philip: I don't know much about the GUI. But there might be some overlap with the variable viewer. Maybe you could raise this question on the mailing list once this is pushed.

(file #45479, file #45480, file #45481)

Markus Mützel <mmuetzel>
Group administrator
Sun 18 Nov 2018 04:09:55 PM UTC, comment #43: 

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.

Philip Nienhuis <philipnienhuis>
Group Member
Sun 18 Nov 2018 03:08:36 PM UTC, comment #42: 

Hi,

I looked at the file #45472, file #45473 and file #45474

1.) The patch file #44579 used has a problem which can be made visible if a diff between file #44579 and the patch file #44572 is made. I think Philip Nienhuis patch (file #44579) is correct. To solve the problem you only need to change one line. Go to


/doc/interpreter/module.mk


and add


$(GRAPHICS_PROPS_SRC)


to the end of the line


%reldir%/plot-uitableproperties.texi: doc/interpreter/genpropdoc.m


Then you are done.

2.) I'am happy with "str2double" DEFUN in utils.cc and the implementation "octave_str2double" in lo-utils.cc. I just got a bit confused because the str2double helper functions (extract_num, is_imag_unit) have now been merged into lo-utils.cc as well. To my taste this was a little bit "nicer" before but it is not a big deal anyway.

3.) I agree to keep track of the open bugs in a separate bug report after the implementation has been pushed.

Thank you and have a nice day

Hg200 <hg200>
Sat 17 Nov 2018 06:35:17 PM UTC, comment #41: 

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)

Markus Mützel <mmuetzel>
Group administrator
Wed 18 Jul 2018 02:58:19 PM UTC, comment #40: 

Yes I can confirm the bug in comment #33 (on Windows 7 Enterprise 64 bit).

Philip Nienhuis <philipnienhuis>
Group Member
Wed 18 Jul 2018 02:45:49 PM UTC, comment #39: 

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)

A.R. Burgers <arb>
Tue 17 Jul 2018 07:58:35 PM UTC, comment #38: 

i made a diff between patch_9084_uitable_20180718.cset and it looks identical to my version. note for future patches: please remove _init_qt_.cc from the commit messages.

another question: can anybody confirm the bug reported in comment #33? i still can observe this bug but wasn't able to track it down in the source code. apart from this issue (octave sometimes even crashes) the patch is stable and works very good.

best regards

Hg200 <hg200>
Tue 17 Jul 2018 06:59:19 PM UTC, comment #37: 

... 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)

Philip Nienhuis <philipnienhuis>
Group Member
Tue 17 Jul 2018 02:19:17 PM UTC, comment #36: 

hi,

I made a comparison to a patch I created for the current development branch. in your patch the file

libinterp/corefcn/kron.cc

is changed. looks to me this has nothing todo with uitable. can you please check? if you like i can send you my patch or i can upload it here. let me know.

best regards

Hg200 <hg200>
Mon 16 Jul 2018 09:08:40 PM UTC, comment #35: 

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.

Mike Miller <mtmiller>
Group Member
Mon 16 Jul 2018 09:06:13 PM UTC, comment #34: 

I refreshed the 20180101 version of the patch against 4.4.1-rc1, see the attachment. With this patch demo uitable works.

There was one chunk reject which I haven't resolved yet:


--- libgui/graphics/__init_qt__.cc      Fri Dec 29 22:25:01 2017 +0100
+++ libgui/graphics/__init_qt__.cc      Mon Jan 01 00:44:12 2018 +0100
@@ -78,6 +78,9 @@
             root.set ("defaultuicontrolforegroundcolor",
                       octave_value (Utils::toRgb
                                     (p.color (QPalette::WindowText))));
+            root.set ("defaultuitableforegroundcolor",
+                      octave_value (Utils::toRgb
+                                   (p.color (QPalette::WindowText))));
             root.set ("defaultuipanelbackgroundcolor",
                       octave_value (Utils::toRgb (p.color (QPalette::Window))));
             root.set ("defaultuipanelforegroundcolor",
~


A.R. Burgers <arb>
Tue 02 Jan 2018 02:33:28 PM UTC, comment #33: 

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

Hg200 <hg200>
Mon 01 Jan 2018 10:36:41 AM UTC, comment #32: 

type "help uitable". in the "complex example" x and y coordinates are swapped & the figure does not appear correctly on low screen resolutions (i.e. laptop).

(file #42780)

Hg200 <hg200>
Sun 31 Dec 2017 07:27:20 PM UTC, comment #31: 

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)

Hg200 <hg200>
Sun 31 Dec 2017 11:04:22 AM UTC, comment #30: 

This (complete) patch includes:

1.) Workaround for the missing m_curData initialization as the Table constructor is called. I would appreciate if somebody can review this workaround (description see comment #23)
2.) Warning '__go_uitable__ Undocumented' is removed
3.) Update to HG TIP

**

If we run "demo uitable" the first figure clears as the second figure appears. Running the code directly the first figure does not clear:


f = figure;
m = magic(8);
t = uitable(f, 'Data', m, 'ColumnWidth', { 40 });
f = figure;
m = magic(8);
t = uitable(f, 'Data', m, 'ColumnWidth', { 40 }, 'ColumnEditable', true);


I can reproduce this 'CLF' behaviour of the first figure with following test code. Looks like it is a general behaviour (not related to uitable):


%!demo
%! f = figure;
%! plot(rand(1,10));
%!demo
%! f = figure;
%! plot(rand(1,10));




(file #42772)

Hg200 <hg200>
Sat 30 Dec 2017 10:49:05 PM UTC, comment #29: 

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 Nienhuis <philipnienhuis>
Group Member
Sat 30 Dec 2017 02:25:31 AM UTC, comment #28: 

gah - a copy/paste error. you get the point ...


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

Hg200 <hg200>
Sat 30 Dec 2017 01:30:13 AM UTC, comment #27: 

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

Hg200 <hg200>
Fri 29 Dec 2017 09:50:17 PM UTC, comment #26: 

I've just put in the extra line "m_curData = ..." and crossed my fingers :-)
There is a build warning:

:
  TEXI2PDF doc/interpreter/octave.pdf
mk-doc-cache.pl: function '__go_uitable__ Undocumented.  ' may be undocumented at ../dev_p/doc/interpreter/mk-doc-cache.pl line 133.
:


Looking at _go_uitable_ I see there is a documentation placeholder but maybe the tex formatting isn't correct.

When running Octave and doing "demo uitable" the second demo leaves a blank figure 1 behind.

Running the examples on the Matlab doc page work well after converting from Matlab's new graphics object syntax.

One thing I would like is if the default position were such that a table is centered better, or fills more of a default new figure window than the square somewhere in the lower left region.
But I see that the competition merely does the same -  another chance for octave to be better.

Nice work !

Philip Nienhuis <philipnienhuis>
Group Member
Mon 25 Dec 2017 02:21:18 PM UTC, comment #25: 

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)

Hg200 <hg200>
Mon 25 Dec 2017 12:26:11 PM UTC, comment #24: 

Thanks for your work.
Where's your patch? or do you rather want to wait until you've fixed it?

Philip Nienhuis <philipnienhuis>
Group Member
Sat 23 Dec 2017 11:45:13 AM UTC, comment #23: 

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

Hg200 <hg200>
Mon 22 May 2017 02:08:55 AM UTC, comment #22: 

Please help me to install this patch on windows 10.

Thanks in advance





Sanjeev <sanju22>
Mon 02 Jan 2017 05:33:20 AM UTC, comment #21: 

Hi, Andy

I found another issue, and fixed it.

6. Run following command, click first cell, type 1<Return> 10 times quickly, and uitable data property become corrupt.

> uitable('ColumnFormat',{'numeric'},'ColumnEditable',true,'Data',zeros(10,1),'CellEditCallback',@(h)disp(get(h,'Data')'),'CellSelectionCallback',@()rand(3000));


Cause of this issue is asynchronous of writing data property, using gh_manager::post_set.
So, I added Table::m_curData attribute, and read/write it synchronously.

Updated version is attached.

Regards,
Teppei

(file #39340)

Anonymous
Fri 30 Dec 2016 01:10:21 PM UTC, comment #20: 

Hi, Andy

Thanks to your reply.

> OK, so using the uitable-20161209.diff, I can't seem to replicate issue 1 on either QT4 or QT5 on linux.

I checked 1st issue on Linux+Qt4.8.2 and WinXP+Qt4.8.7(MXE-Octave), using patch_9084_uitable-20160902.diff.
I fixed 1-4 issues (uitable-20161209.diff is fixed version), so you can't replicate issue 1 using this patch, maybe.

> I noticed a missing break from one of the switch statements with the keys

Yes, I found it, too.
Sorry.

> 2. I've think I've fixed the focus issue.

Thanks to fix it.
I think your approach (setFocusPolicy NoFocus) is more smart, but produces some side effects; Arrow keys moves current selection 2 cells when a combobox selected.
I fixed it by removing Arrow/Tab key callbacks.

> I've not implemented point 3 as I'm not sure what to do and need your advice about it.

Sorry for lack of explanation.
I added Up/Down/(Shift+)Enter/(Shift+)Tab key callbacks, and changed cell selection behaviors in times of clicking combobox, in patch uitable-20161209.diff.
I want to edit table cells by keyboard, using these callbacks.

And, I found another issue; CellEditCallback event don't accept arguments (ex. set(obj,'CellEditCallback',{@fh,arg1,arg2})
I fixed it by changing Table::sendCellEditCallback function.
-  if (properties<uitable> ().get_celleditcallback ().is_function_handle ())
+  if (!(properties<uitable> ().get_celleditcallback ().is_empty ()))

Updated version is attached.

Regards,
Teppei

(file #39330)

Anonymous
Tue 20 Dec 2016 03:25:17 PM UTC, comment #19: 

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)

Anonymous
Sun 18 Dec 2016 05:31:13 PM UTC, comment #18: 

I can confirm issues 1 and 2 from comment #7.
As to 3. and 4., I haven't played too much with it.

So I think this cset needs some polishing before it can be pushed. But I do hope it can be incorporated soon in Octave.

Philip Nienhuis <philipnienhuis>
Group Member
Thu 08 Dec 2016 08:50:41 PM UTC, comment #17: 

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)

Anonymous
Tue 22 Nov 2016 08:53:40 PM UTC, comment #16: 

OK so now 4.2 is out, has anyone had a chance to take a look at this patch? I'd like to get this in to octave as it's a really useful ui component.

Anonymous
Mon 26 Sep 2016 08:04:19 PM UTC, comment #15: 

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.

Mike Miller <mtmiller>
Group Member
Mon 26 Sep 2016 06:38:40 PM UTC, comment #14: 

I suppose this might not make it into 4.2.x; the feature freeze is way behind us.
But I am not a core dev. I asked Mike (comment #9) but didn't get response yet.

Philip Nienhuis <philipnienhuis>
Group Member
Sun 25 Sep 2016 05:51:59 PM UTC, comment #13: 

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

Anonymous
Fri 02 Sep 2016 02:12:17 PM UTC, comment #12: 

After fixing the failed hunks and teting I produced a refreshed cse (attached).
Seems to work fine again.


(file #38407)

Philip Nienhuis <philipnienhuis>
Group Member
Fri 02 Sep 2016 01:54:47 PM UTC, comment #11: 

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.

Philip Nienhuis <philipnienhuis>
Group Member
Fri 02 Sep 2016 01:09:03 PM UTC, comment #10: 

Too bad, doesn't apply anymore, again :-(

:
  GEN      libinterp/dldfcn/qr.oct
  GEN      libinterp/dldfcn/symrcm.oct
  GEN      src/octave-cli-4.1.0+
  CXXLD    src/octave-gui
libgui/.libs/liboctgui.so: undefined reference to `QFont QtHandles::Utils::computeFont<uitable>(uitable::properties const&, int)'
collect2: error: ld returned 1 exit status
Makefile:14700: recipe for target 'src/octave-gui' failed
make[2]: *** [src/octave-gui] Error 1
make[2]: Leaving directory '/home/philip/devel/octdev/oct410+tmp'
Makefile:26170: recipe for target 'all-recursive' failed
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory '/home/philip/devel/octdev/oct410+tmp'
Makefile:9902: recipe for target 'all' failed
make: *** [all] Error 2


I think the str2double.h line in libinterp/corefcn/module.mk had better move down to the NOINSTALL_COREFCN_INC.


Philip Nienhuis <philipnienhuis>
Group Member
Sat 20 Aug 2016 07:20:22 PM UTC, comment #9: 

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?

Philip Nienhuis <philipnienhuis>
Group Member
Fri 19 Aug 2016 08:27:31 PM UTC, comment #8: 

Heya!

I knew it was too easy just creating the st2double.h file without wiring it in.

Here's another patch with a fixed (I hope) module.mk - Have I added it to the right place.

Andy

(file #38286)

Anonymous
Fri 19 Aug 2016 05:46:12 PM UTC, comment #7: 

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.

Philip Nienhuis <philipnienhuis>
Group Member
Thu 18 Aug 2016 08:02:25 PM UTC, comment #6: 

OK here's it updated to the latest head.

(Please be aware that the final demo is affected by bug #44776.)

Andy

(file #38260)

Anonymous
Wed 17 Aug 2016 08:11:59 PM UTC, comment #5: 

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 :-)

Philip Nienhuis <philipnienhuis>
Group Member
Sat 13 Aug 2016 10:42:25 PM UTC, comment #4: 

Hi Mike,

Here's an updated patch:

  • The other long lines have been broken
  • I've exposed some demos and a complex demo, and added a quite complex example
  • (Unfortunately the complex example can't be added as a demo as you can't make functions in the demo environment.)
  • NEWS file is updated


It compiles under QT4 and QT5 however, I'm finding that the QT5 install is a little unstable (without using uitable) - this may be a fault of my libraries.



(file #38203)

Anonymous
Fri 12 Aug 2016 10:50:43 PM UTC, comment #3: 

I mean Mike of course!

Anonymous
Fri 12 Aug 2016 04:58:48 PM UTC, comment #2: 

Hi Matt,

Thanks for looking at this.

I'm not sure I understand how you want the separate changeset to be generated. Do you just want a diff just containing the changes:

  • libinterp/corefcn/pr-output.cc: Make rational_approx non-static - needed for columnformat in uitable
  • libinterp/corefcn/pr-output.h: Expose rational_approx in pr-output.h
  • libinterp/corefcn/str2double.cc: Expose str2double1 as str2double_internal - needed to interpret data edits
  • libinterp/corefcn/str2double.h: Expose str2double_internal


They're really very small changes - moving a static function into the non-static space and sticking a stub in to the header file.

Both functions are completely stateless and would therefore be safe - I would have reimplemented them otherwise.

I'll add a demo to my next patch set and prod the NEWS file - I knew I'd forgotten something.

Anonymous
Thu 11 Aug 2016 08:51:13 PM UTC, comment #1: 

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.

Mike Miller <mtmiller>
Group Member
Thu 11 Aug 2016 07:43:44 PM UTC, original submission:  

Hi!

A previous project of mine required uitable which is not implement in octave. This has meant that this project does not work on octave. I have therefore implemented this.

It's quite a large patch as the API matlab presents, although initially simple has lots of complexity.

This patch has implemented the almost all of these features, however the following caveats are present:

  • BackgroundColor: Matlab allows more than 2 BackgroundColors with RowStriping - I have only implemented this for 2.


  • FontSize: In matlab this does not affect the font size of the row/column headers. This patch applies this to everything.


  • The representation of complex values may be different from those shown in matlab. All other representations should be the same.


  • When editing data, extremely large integers may overflow (i.e. those that are greater than int64max) rather than get fixed to int64max.


Please note: I have adjusted the visibility of a few functions str2double1 and rational_approx. This is in order to avoid duplicating this work.

I would appreciate any stylistic comments and help to get this work merged as it has been a lot of work.

Anonymous

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #45479:  patch9084_uitable_part1_v2.patch added by mmuetzel (23KiB - application/octet-stream)
file #45480:  patch9084_uitable_part2_v2.patch added by mmuetzel (8KiB - application/octet-stream)
file #45481:  patch9084_uitable_part3_v2.patch added by mmuetzel (98KiB - application/octet-stream)
file #45472:  patch9084_uitable_part1.patch added by mmuetzel (23KiB - application/octet-stream)
file #45473:  patch9084_uitable_part2.patch added by mmuetzel (8KiB - application/octet-stream)
file #45474:  patch9084_uitable_part3.patch added by mmuetzel (98KiB - application/octet-stream)
file #44571:  patch_9084_uitable_20180514.cset added by philipnienhuis (102KiB - application/octet-stream)
file #44572:  patch_9084_uitable_20180718.cset added by philipnienhuis (101KiB - application/octet-stream)
file #44568:  patch_9084_uitable-20180717.diff added by arb (100KiB - text/x-patch - 20180716 version was incomplete because I hadn't hg-added the new files ...)
file #44567:  patch_9084_uitable-20180716.diff added by arb (26KiB - text/x-patch - patch refreshed against 4.4.1-rc1)
file #42780:  patch_9084_uitable-20180101.diff added by hg200 (102KiB - text/x-patch - see comment #32 (help examples fine tuning))
file #42774:  patch_9084_uitable-20171231_v2.diff added by hg200 (102KiB - text/x-patch - see comment #31 (fix scrambled output))
file #42724:  patch_9084_uitable-20171223_nowarnings.diff added by hg200 (102KiB - text/x-patch - see comment #25 (contains the bug))
file #39340:  patch_9084_uitable-20170102.diff added by None (103KiB - text/x-diff)
file #39330:  patch_9084_uitable-20161230.diff added by None (103KiB - text/x-diff)
file #39287:  patch_9084_uitable-20161220.diff added by None (110KiB - text/x-patch)
file #39179:  uitable-20161209.diff added by None (107KiB - text/x-diff)
file #38286:  uitable-20160819.diff added by None (99KiB - text/x-patch)
file #38260:  uitable-20160818.diff added by None (98KiB - text/x-patch)
file #38203:  uitable-20160813.diff added by None (98KiB - text/x-patch)
file #38181:  uitable-20160811.diff added by None (88KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by gyom (Posted a comment)
  • -email is unavailable- added by mmuetzel (Updated the item)
  • -email is unavailable- added by arb (Posted a comment)
  • -email is unavailable- added by hg200 (Posted a comment)
  • -email is unavailable- added by sanju22 (Posted a comment)
  • -email is unavailable- added by None (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 25 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-12-12 mmuetzel StatusReady For Test Done
        Open/ClosedOpen Closed
    2018-11-21 mmuetzel StatusIn Progress Ready For Test
    2018-11-19 mtmiller Carbon-CopyRemoved 80942 -
    2018-11-18 mmuetzel StatusNone In Progress
    2018-11-18 mmuetzel Attached File- Added patch9084_uitable_part1_v2.patch, #45479
        Attached File- Added patch9084_uitable_part2_v2.patch, #45480
        Attached File- Added patch9084_uitable_part3_v2.patch, #45481
    2018-11-17 mmuetzel Attached File- Added patch9084_uitable_part1.patch, #45472
        Attached File- Added patch9084_uitable_part2.patch, #45473
        Attached File- Added patch9084_uitable_part3.patch, #45474
    2018-07-18 arb Attached File- Added patch_9084_uitable-20180718.diff, #44579
    2018-07-17 philipnienhuis Attached File- Added patch_9084_uitable_20180514.cset, #44571
        Attached File- Added patch_9084_uitable_20180718.cset, #44572
    2018-07-17 arb Attached File- Added patch_9084_uitable-20180717.diff, #44568
    2018-07-17 arb Attached File- Added patch_9084_uitable-20180716.diff, #44567
    2018-01-01 hg200 Attached File- Added patch_9084_uitable-20180101.diff, #42780
    2017-12-31 hg200 Attached File- Added patch_9084_uitable-20171231_v2.diff, #42774
    2017-12-31 hg200 Attached File- Added patch_9084_uitable-20171231.diff, #42772
    2017-12-25 hg200 Attached File- Added patch_9084_uitable-20171223_nowarnings.diff, #42724
    2017-12-23 hg200 Carbon-CopyRemoved hg200 -
    2017-12-23 hg200 Carbon-Copy- Added hg200
    2017-01-02 None Attached File- Added patch_9084_uitable-20170102.diff, #39340
    2016-12-30 None Attached File- Added patch_9084_uitable-20161230.diff, #39330
    2016-12-20 None Attached File- Added patch_9084_uitable-20161220.diff, #39287

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code