Bug 325638

Comment 1 Patrick Tasse 2010-09-17 13:36:04 EDT

Created attachment 179139 [details]
Proposed patch on TmfVirtualTable.java

Legal Message: I, Patrick Tasse, declare that I developed attached code from
scratch, without referencing any 3rd party materials except material licensed
under the EPL. I am authorized by my employer to make this contribution under
the EPL.

Comment 2 Francois Chouinard 2010-09-18 01:03:47 EDT

Thanks Patrick.

I know you are using a Windows machine so it may be related.

In my environment (gtk), your patch has the following problems:

- No scrollbar is displayed (horizontal or vertical)
- I had an ArrayOutOfBoundException when scrolling the view
 (line 329 - e.item = fTable.getSelection()[0])

The exception could be a glitch but disappearance of the scrollbars (at least the vertical one) is a problem.

[0] Overall

I see that you removed some fields from the class and made them local to handleTableKeyEvent(). Good thing.

I also see that you removed a few other fields altogether and simplified some calculations. More on this later :-)

[1] Slider

I could get the vertical scrollbar back by removing the following code in createSlider():
//	if ((style & SWT.V_SCROLL) == 0) {
//		fSlider.setVisible(false);
//	}

In that case, the addition of the SWT.V_SCROLL to the slider style in the method call becomes superfluous.

I guess you would like the virtual table to behave *exactly* like a standard table (a nice goal) and remove the slider - acting as a vertical scrollbar - if there are less than events than the table can display.

I think that having the slider always present is a very minor annoyance since very few traces will have less than the 10-or-so events that the table usually displays.

[2] Table horizontal scrollbar

Adding an horizontal scrollbar to the table had the following problem:

- Set table columns so the horizontal bar will be displayed
- Open a trace
- Resize the column so the scrollbar disappears
- Go to the end of the trace

In my case, you end up with an empty at the end.

The problem seems to happen in the calculation of the number of visible rows. I noticed that in your patch you removed a variable that was used to handle that particular case :-)

Anyway, the problem I had with the horizontal scrollbar was that the widget doesn't seem to be notified when you resize the *columns* - which controls the appearance/disappearance of the horizontal scrollbar. This in turn affects the number of lines visible - to which the table is oblivious - and introduces problems in the handling of scrolling.

But, of course, it would be nice to have an H_SCROLL :-)

[3] ArrayOutOfBoundException

I guess there are cases where *no* row is selected...

Comment 3 Patrick Tasse 2010-09-23 11:08:03 EDT

Created attachment 179465 [details]
Proposed patch on TmfVirtualTable.java and TmfEventsTable.java

The original patch is missing a necessary change in TmfEventsTable.java. The styles SWT.H_SCROLL and SWT.V_SCROLL must be provided by the user of TmfVirtualTable.java in the constructor's style. The scrollbar styles are now added there. In addition, the Table-intended styles are now passed to the createTable() method, they are filtered out of the Composite's super constructor. Other styles are passed to the Composite and filtered out of the Table. The supported styles are now enumerated in the javadoc comment.

The ArrayOutOfBoundException happens when a non-selection-related key is pressed while the current selection is not visible. In addition to adding a bounds check, a default branch is added to prevent broadcasting unnecessary selection events on non-selection-related key events.

The handling of partially visible last row is done using the new variable fFullyVisibleRows (somewhat replacing fPartialRowVisible boolean variable). It is intended behaviour that going to the last event (with END key, slider or mousewheel) will position the table so that the last event is on the last fully visible row with a partial blank row at the bottom. This is because Windows doesn't move the table up on a pixel precision when a partially-visible row is selected, like Linux does. So in Windows putting the last event in the very last row would make it impossible to have a full view of the last event when the table size was not an exact multiple of the table item height.

In some cases (suspected older GTK versions) the Table.getItemHeight() method returns the wrong value in Linux and this causes an incorrect value of fFullyVisibleRows variable, which can make it impossible to view the last few events. A workaround is added to calculate the actual table item height from the difference in position of two consecutive rows.

Finally the unnecessary fTableItems[] array is removed since TableItems can be retrieved directly from the Table. This simplifies some of the methods.

Legal Message: I, Patrick Tasse, declare that I developed attached code from scratch, without referencing any 3rd party materials except material licensed under the EPL. I am authorized by my employer to make this contribution under the EPL.

Comment 4 Patrick Tasse 2011-02-23 17:15:16 EST

Created attachment 189654 [details]
Proposed patch on TmfVirtualTable.java and TmfEventsTable.java

Additional fixes:

- Add 'frozen row' API to keep a specified number to top rows visible at all times.

- Prevent the slider from being traversed with tab key.

- Prevent handling table selection when selection is empty.

- Fix to delay notification to selection listeners until table items are all refreshed. Notification is delayed when e.doit is set to false at return of the SWT.SetData event, and in that case the notification will be sent when refresh() is called later.

- Remove TmfTimeSynchSignal broadcasting from virtual table.

- Add API to get item count.

- Add API to get item at Point location.

- Prevent unnecessary refresh when table resize does not add or delete visible items.

- Add API to get selection index.

Legal Message: I, Patrick Tasse, declare that I developed attached code from
scratch, without referencing any 3rd party materials except material licensed
under the EPL. I am authorized by my employer to make this contribution under
the EPL.

Comment 5 Francois Chouinard 2011-03-29 18:25:46 EDT

Patch committed. Thanks Patrick.

Comment 6 Francois Chouinard 2011-07-22 14:49:19 EDT

Delivered with 0.8