Bug 461354 - [GTK] Find a better solution for inserting elements into Table/Tree/List
Summary: [GTK] Find a better solution for inserting elements into Table/Tree/List
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: SWT (show other bugs)
Version: 4.5   Edit
Hardware: PC Linux
: P3 normal (vote)
Target Milestone: 4.7   Edit
Assignee: Platform-SWT-Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords: helpwanted
: 465178 493572 (view as bug list)
Depends on: 495909
Blocks: 417258 457464 487467 493357
  Show dependency tree
 
Reported: 2015-03-04 04:01 EST by Sravan Kumar Lakkimsetti CLA
Modified: 2016-06-15 09:58 EDT (History)
9 users (show)

See Also:


Attachments
List.getTopIndex() problem (1.04 KB, text/x-java)
2015-03-04 04:01 EST, Sravan Kumar Lakkimsetti CLA
no flags Details
Tree.getTopIndex() problem (1.09 KB, text/x-java)
2015-03-04 04:02 EST, Sravan Kumar Lakkimsetti CLA
no flags Details
small test program in C (2.99 KB, text/plain)
2015-04-15 06:44 EDT, Jonny Lamb CLA
no flags Details
test program using the scroll to cell call (3.31 KB, text/x-csrc)
2015-04-15 08:26 EDT, Sravan Kumar Lakkimsetti CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Sravan Kumar Lakkimsetti CLA 2015-03-04 04:01:54 EST
Created attachment 251271 [details]
List.getTopIndex() problem

When we add new items to table/tree/list, the items does not get added immediately. We need to wait for all events to be processed before calling getTopIndex kind of apis. currently if we add new items, set selection and try to get the top index it is giving wrong results. in between if we put a eventloop to process all the pending events then we can get the correct values for topindex.

There are couple of snippets added here to reproduce the problem. These will fail consistently in GTK3 mode.
Comment 1 Sravan Kumar Lakkimsetti CLA 2015-03-04 04:02:33 EST
Created attachment 251272 [details]
Tree.getTopIndex() problem
Comment 2 Sravan Kumar Lakkimsetti CLA 2015-03-31 09:04:25 EDT
@jonny.lamb

I did some analysis on this issue. What I noticed is in GTKTreeModel it takes some time to add new elements. in the mean time if there is any other call like set selection is received that also goes into the queue. 

At this particular point if we call other functions to retreive the top index in the viewer, the result we are getting is not accurate. If we add a eventloop to process all the pending events we are getting the correct results

currently List.getTopIndex works on GTK2 but doesnot work in the GTK3. 

Can you please give us your view on this?
Comment 3 Sravan Kumar Lakkimsetti CLA 2015-04-14 05:34:40 EDT
@jonny.lamb

Did had a chance to look at this problem. We have two weeks left for M7 to end. Can you please let me know your views on this issue?

Thanks
Sravan
Comment 4 Jonny Lamb CLA 2015-04-14 12:54:59 EDT
(In reply to Sravan Kumar Lakkimsetti from comment #3)
> Did had a chance to look at this problem. We have two weeks left for M7 to
> end. Can you please let me know your views on this issue?

Yes I've been looking into this recently and doing lots of debugging. As expected, the problem isn't to do with a delay in adding items to the GtkListStore, but in allocating space to the widget so we can check what's showing at the top. Although the code path in finding this out is very weird, I don't quite see why simply realizing the view doesn't do the right thing before reaching the main loop.

I'll keep you updated.
Comment 5 Jonny Lamb CLA 2015-04-15 06:44:48 EDT
Created attachment 252405 [details]
small test program in C

I rewrote the basic idea here using some basic GTK+ in C, to be able to ignore lots of complexity in the SWT code.

I get the same outcome and although there is an ugly workaround (gtk_widget_show_all(window)) I'm not sure how that helps in our case.
Comment 6 Sravan Kumar Lakkimsetti CLA 2015-04-15 08:26:56 EDT
Created attachment 252408 [details]
test program using the scroll to cell call
Comment 7 Sravan Kumar Lakkimsetti CLA 2015-04-15 08:33:18 EDT
I added scroll to cell call which we use to scroll to the respective row. in this case even gtk_widget_show_all call is not working.

Here are the lines I added before first get_top_row call. 

  model = gtk_tree_view_get_model (GTK_TREE_VIEW(view));
  gtk_tree_model_iter_nth_child (model, &iter, 0, 50);
  path = gtk_tree_model_get_path (model, &iter);
  gtk_tree_view_scroll_to_cell (GTK_TREE_VIEW(view), path, 0, false, 0, 0);
  gtk_widget_show_all (window);

the output I got is 
row at top has index 0 and text: this is string number 0
in idle: row at top has index 50 and text: this is string number 50
in idle: row at top has index 50 and text: this is string number 50
in idle: row at top has index 50 and text: this is string number 50
in idle: row at top has index 50 and text: this is string number 50
in idle: row at top has index 50 and text: this is string number 50
in idle: row at top has index 50 and text: this is string number 50
in idle: row at top has index 50 and text: this is string number 50
in idle: row at top has index 50 and text: this is string number 50

I think we should get 50 on the first row also
Comment 8 Marc-André Laperle CLA 2015-04-21 11:04:26 EDT
(In reply to Sravan Kumar Lakkimsetti from comment #2)
> currently List.getTopIndex works on GTK2 but doesnot work in the GTK3. 

Since this is GTK3 only, should this be be tagged (title) as [GTK3] and added as a dependency to bug 441566?
Comment 9 Sravan Kumar Lakkimsetti CLA 2015-04-22 01:47:35 EDT
(In reply to Marc-Andre Laperle from comment #8)
> (In reply to Sravan Kumar Lakkimsetti from comment #2)
> > currently List.getTopIndex works on GTK2 but doesnot work in the GTK3. 
> 
> Since this is GTK3 only, should this be be tagged (title) as [GTK3] and
> added as a dependency to bug 441566?

This problem exists in GTK 2 as well. There is a workaround in SWT which works only for GTK2. If we remove the workaround this List.getTopIndex() also fails
Comment 10 Sravan Kumar Lakkimsetti CLA 2015-04-22 06:39:10 EDT
*** Bug 465178 has been marked as a duplicate of this bug. ***
Comment 11 Marc-André Laperle CLA 2015-04-22 09:49:22 EDT
(In reply to Sravan Kumar Lakkimsetti from comment #9)
> (In reply to Marc-Andre Laperle from comment #8)
> > (In reply to Sravan Kumar Lakkimsetti from comment #2)
> > > currently List.getTopIndex works on GTK2 but doesnot work in the GTK3. 
> > 
> > Since this is GTK3 only, should this be be tagged (title) as [GTK3] and
> > added as a dependency to bug 441566?
> 
> This problem exists in GTK 2 as well. There is a workaround in SWT which
> works only for GTK2. If we remove the workaround this List.getTopIndex()
> also fails

I see! Thanks for the explanation!
Comment 12 Sravan Kumar Lakkimsetti CLA 2015-04-28 06:06:58 EDT
(In reply to Jonny Lamb from comment #5)
> Created attachment 252405 [details]
> small test program in C
> 
> I rewrote the basic idea here using some basic GTK+ in C, to be able to
> ignore lots of complexity in the SWT code.
> 
> I get the same outcome and although there is an ugly workaround
> (gtk_widget_show_all(window)) I'm not sure how that helps in our case.

Hi Jonny,

Any update on this bug?

Thanks
Sravan
Comment 13 Jonny Lamb CLA 2015-04-28 10:02:49 EDT
(In reply to Sravan Kumar Lakkimsetti from comment #12)
> Any update on this bug?

No update I'm afraid, and now I'm working on something else so don't have time to look at this right now, sorry about that.
Comment 14 Lars Vogel CLA 2015-05-05 01:50:08 EDT
I think this one is relatively critical, clients expect the correction selection to be returned to them. This bug also causes our platform.ui tests to fail under Ubuntu.

Can this be addressed in RC1?
Comment 15 Sravan Kumar Lakkimsetti CLA 2015-05-18 08:01:12 EDT
I raised this issue with GNOME bugzilla as well https://bugzilla.gnome.org/show_bug.cgi?id=749537 So moving this to 4.5.1 for now
Comment 16 Sravan Kumar Lakkimsetti CLA 2015-08-04 05:12:58 EDT
We haven't received update from GTK regarding this issue. So moving it to 4.6
Comment 17 Eric Williams CLA 2016-04-15 11:26:52 EDT
(In reply to Sravan Kumar Lakkimsetti from comment #9)
> This problem exists in GTK 2 as well. There is a workaround in SWT which
> works only for GTK2. If we remove the workaround this List.getTopIndex()
> also fails

Would you be able to point me to this workaround?
Comment 18 Sravan Kumar Lakkimsetti CLA 2016-04-18 04:57:07 EDT
(In reply to Eric Williams from comment #17)
> (In reply to Sravan Kumar Lakkimsetti from comment #9)
> > This problem exists in GTK 2 as well. There is a workaround in SWT which
> > works only for GTK2. If we remove the workaround this List.getTopIndex()
> > also fails
> 
> Would you be able to point me to this workaround?

Hi Eric,

Please look at List#showSelection() there is a gtk_tree_view_scroll_to_point call is there eventhough scroll_to_cell call is there. you need to use gtk_tree_view _scroll_to_cell only.

Because of the gtk_tree_view_scroll_to_point this is working on GTK2.

Hope this helps
Sravan
Comment 19 Eric Williams CLA 2016-04-18 11:37:08 EDT
(In reply to Sravan Kumar Lakkimsetti from comment #18)
> (In reply to Eric Williams from comment #17)
> > (In reply to Sravan Kumar Lakkimsetti from comment #9)
> > > This problem exists in GTK 2 as well. There is a workaround in SWT which
> > > works only for GTK2. If we remove the workaround this List.getTopIndex()
> > > also fails
> > 
> > Would you be able to point me to this workaround?
> 
> Hi Eric,
> 
> Please look at List#showSelection() there is a gtk_tree_view_scroll_to_point
> call is there eventhough scroll_to_cell call is there. you need to use
> gtk_tree_view _scroll_to_cell only.
> 
> Because of the gtk_tree_view_scroll_to_point this is working on GTK2.
> 
> Hope this helps
> Sravan

Thanks that's actually quite useful.

It seems this issue manifests itself mostly when using the arrow keys to navigate selection. For example, selecting an item at the bottom of the visible area and then pressing the down arrow will cause the bug. Scrolling to a random item and selecting it doesn't cause any problems. In the cases where the bug does happen it's an off-by-one error.
Comment 20 Sravan Kumar Lakkimsetti CLA 2016-04-18 12:56:48 EDT
(In reply to Eric Williams from comment #19)
> (In reply to Sravan Kumar Lakkimsetti from comment #18)
> > (In reply to Eric Williams from comment #17)
> > > (In reply to Sravan Kumar Lakkimsetti from comment #9)
> > > > This problem exists in GTK 2 as well. There is a workaround in SWT which
> > > > works only for GTK2. If we remove the workaround this List.getTopIndex()
> > > > also fails
> > > 
> > > Would you be able to point me to this workaround?
> > 
> > Hi Eric,
> > 
> > Please look at List#showSelection() there is a gtk_tree_view_scroll_to_point
> > call is there eventhough scroll_to_cell call is there. you need to use
> > gtk_tree_view _scroll_to_cell only.
> > 
> > Because of the gtk_tree_view_scroll_to_point this is working on GTK2.
> > 
> > Hope this helps
> > Sravan
> 
> Thanks that's actually quite useful.
> 
> It seems this issue manifests itself mostly when using the arrow keys to
> navigate selection. For example, selecting an item at the bottom of the
> visible area and then pressing the down arrow will cause the bug. Scrolling
> to a random item and selecting it doesn't cause any problems. In the cases
> where the bug does happen it's an off-by-one error.

This problem is easily reproducible if you use setSelection on the list. Try using the setselection on an element which is not visible, this problem can be easily reproduced. You can see this problem only in the getTopIndex call.  setSelection reveals the selected item on the UI. 

Try this 
create a list with 1000 entries
show the list in the UI with selected item at index 0
now do a set selection with index 500
call getTopindex on the list this should return 500. but you'll get 0

put a eventloop to process all events after setSelection getTopindex returns 500.
Comment 21 Eric Williams CLA 2016-04-18 13:19:14 EDT
(In reply to Sravan Kumar Lakkimsetti from comment #20)
> This problem is easily reproducible if you use setSelection on the list. Try
> using the setselection on an element which is not visible, this problem can
> be easily reproduced. You can see this problem only in the getTopIndex call.
> setSelection reveals the selected item on the UI. 
> 
> Try this 
> create a list with 1000 entries
> show the list in the UI with selected item at index 0
> now do a set selection with index 500
> call getTopindex on the list this should return 500. but you'll get 0
> 
> put a eventloop to process all events after setSelection getTopindex returns
> 500.

Doesn't getTopIndex() return the index of the top most *visible* item? I.e. if you have a list that displays 4 items before you have to scroll to see the next one, wouldn't getTopIndex() return the first item?
Comment 22 Eric Williams CLA 2016-04-18 13:28:04 EDT
Disregard comment 21, I understand now.
Comment 23 Eric Williams CLA 2016-04-18 15:54:37 EDT
It seems there is also an additional test failure related to this bug: test_setTopItemLorg_eclipse_swt_widgets_TreeItem() fails.
Comment 24 Sravan Kumar Lakkimsetti CLA 2016-04-25 07:41:37 EDT
This causes only test failures this does not cause any UI problems as GTK doesnot refresh before any pending events. I believe this is not a major problem. We will check this post 4.6
Comment 25 Eric Williams CLA 2016-05-12 14:16:22 EDT
*** Bug 493572 has been marked as a duplicate of this bug. ***
Comment 26 Eric Williams CLA 2016-06-06 09:06:30 EDT
I think a reasonable solution is to keep track of the topIndex ourselves by using a variable. As Sravan mentioned, the UI itself scrolls properly and calling getTopIndex() after scrolling works fine, since GTK processes the events.

The cases causing problems are the "programmatic" cases, where topIndex is set using setTopIndex() and no UI scrolling is being done by the user. We can handle this case similar to the way we did in Text, where two approaches are used to get the topIndex(), depending on how it was set.

I'll upload a patch shortly.
Comment 27 Eclipse Genie CLA 2016-06-06 09:33:14 EDT
New Gerrit change created: https://git.eclipse.org/r/74663
Comment 28 Eric Williams CLA 2016-06-06 12:26:55 EDT
I should also point out that there seems to be another bug whereby getTopIndex() will always return 0 due to gtk_tree_view_get_path_at_pos() returning false. This seems to happen when you run a test case, i.e. call list.setItems() and then list.set/getTopIndex() in quick succession. This happens on GTK2 and GTK3.

Running the test snippet and calling set/getTopIndex() doesn't cause an issue. I will investigate.
Comment 30 Leo Ufimtsev CLA 2016-06-10 12:37:08 EDT
Merged to master. Thank you Eric for the fix.