Reset position in FactTree only when needed

[rhertzog]

Right now FactTree.set_facts() is always resetting the position of the
view, bringing the user back to the top but this function is called
regularly to update with the latest data even when the user has not
changed anything in the selection widget... which is really annoying
when you are reviewing the list of facts.

Let's distinguish between both cases with a "reset=True" attribute that
we use when we generate an entirely new set of facts to be displayed.

This makes #623 obsolete. Thanks to @mwilck for the initial analysis.

Fixes #594

[matthijskooijman]

At first glance, this fix looks good. But I tested this by scrolling down and then adding a new fact, which seems to also reset the scroll. Is that intended?

Looking at the code, you added a reset parameter to FactTree.set_facts, but it seems that is only called from Overview.find_facts, which always passes reset=True? Should maybe find_facts() also have a reset parameter, which should be True everywhere except when called by on_facts_changed? Or is this the "WIP" part of this PR? I just tried this locally and it seems to work.

Also, maybe reset should be reset_scroll or scroll_to_top to be more explicit?

[rhertzog]

Yeah, I noticed something was fishy yesterday, that's why I moved it back to WIP until I could investigate what's wrong. I'll try out your suggested change and update the PR.

[rhertzog]

So here's the updated commit which also adds scroll_to_top to find_facts().

But contrary to what you suggested, I only passed scroll_to_top=True in two locations: in on_range_selected and in on_search_changed.

So on_facts_changed, on_row_delete_called and on_timeout (and __init__) are not resetting the position. The on_timeout was the most important one to not update since it's the one that gets triggered every 60 seconds to update the figure for the currently running activity...

I believe this is now ready to be merged.

[matthijskooijman]

So on_facts_changed, on_row_delete_called and on_timeout (and init) are not resetting the position. The on_timeout was the most important one to not update since it's the one that gets triggered every 60 seconds to update the figure for the currently running activity...

I wasn't sure about when delete triggered, I now see it happens when you delete a fact with the delete button, so makes sens to not reset the scroll.

As for timeout, I missed that one, agreed that it definitely should not reset the scrolling :-)

With these changes, I agree this can be merged. @mwilck, agreed?

[GeraldJansen]

This merits a comment in NEWS.md! Something like

## Upcoming changes in 3.0.3 (WIP)
* Fixed position in activity list upon refresh (issue 594)

[matthijskooijman]

This merits a comment in NEWS.md! Something like

Yeah, I was thinking to just generate NEWS entries for all relevant merges on release, but having one prepared is a good idea. As for the actual entry, I'd make it a bit more explicit:

* Do not reset scroll position of the activity list when the list is periodically refreshed. (issue 594)

[rhertzog]

Yeah, I find it better to generate them at release time otherwise you generate conflicts on open merge requests for no real gain. Maybe we could add the text in the commit message so that it's just copy & paste, or could even be scripted. I'm not changing anything to the MR right now but let me know if you want me to change something related to this.

[matthijskooijman]

Yeah, I find it better to generate them at release time otherwise you generate conflicts on open merge requests for no real gain. Maybe we could add the text in the commit message so that it's just copy & paste, or could even be scripted.

See #607 for previous discussion of exactly this.

[matthijskooijman]

Lacking further response from other maintainers, I've gone ahead and merged this.