List selection has strange behaviour with scrolling

[demarey]

You can see an example of this strange behaviour here: https://github.com/pharo-project/pharo-launcher/issues/444 This behavior was introduced in https://github.com/pharo-spec/Spec/pull/838.

Here is a small script you can use to reproduce:

presenter := SpTablePresenter new.
presenter
    items: (1 to: 100) asArray;
    openWithSpec. "stop execution here"

presenter selectIndex: 36. "execute and look at the presenter. Should look ok"
presenter selectIndex: 38. "execute and look at the presenter. Here, scrolling should not have happened"

I discussed with @guillep of this issue. Here is a short summary:

• API was designed to decouple scrolling from selection
• The PR:
  • introduces coupling of scrolling and selection
  • uses private methods (ex: https://github.com/pharo-spec/Spec/blob/master/src/Spec2-Adapters-Morphic/SpAbstractMorphicListAdapter.class.st#L37)
• We should use presenter verticalAlignment desiredVisibleRow: indexinstead

Now a design question is: should selection scroll?

• for single selection, why not?
• what happens for multiple selection when all selected elements do not fit in the visible elements?

@guillep proposes to add this method to the presenter:

selectIndex: anInteger scrollToSelection: scrollToSelection
    self selectIndex: anInteger
    scrollToSelection ifTrue: [ 
        self verticalAlignment desiredVisibleRow: anInteger ].

@estebanlm @StevenCostiou @tesonep your opinion?

[estebanlm]

yes, I agree with that design

[guillep]

Sorry for coming late to this, I was in meetings all morning. I chatted a bit with Christophe, he sumarized well our discussion.

Another possibility is that users that want to scroll to selection should do manually. This would avoid adding new API, for something that is already doable.

self selectIndex: anInteger.
self verticalAlignment desiredVisibleRow: anInteger.

I think it's important also to understand what desiredVisibleRow: is (for users, and even if somebody wants to replace it by another mechanism). desiredVisibleRow: is a hint to tell that we would like a particular row to be visible or not in the screen. It's up to the backend to implement how to react to that (even if it could be ignoring).

The morphic backend uses FTTable scrolling, and behaves as follows as of Pharo8:

• if the row is already visible in the screen, do nothing
• if the row is above (before?) the visible rows, scroll up to it until the desired row becomes the first visible row.
• if the row is below (after?) the visible rows, scroll down to it until the desired row becomes the last visible row.

For more info, there are some tests in here:

In GTK (probably this is already done 😄, but just for completeness) this could be implemented using https://developer.gnome.org/gtk3/stable/GtkTreeView.html#gtk-tree-view-scroll-to-cell

Learning from the GTK approach, scrolling could be extended with an alignment property too... But that becomes a feature request, since it could impact all backends :))

I'll make a PR with some comments on desiredVisibleRow:.