FactTree: don't reset y position in set_facts()

[mwilck]

set_facts() is called every minute to update the time of the current
activitiy. If the y position is reset to 0 in set_facts(), this causes
the fact list to be scrolled to the top once a minute, irritating users.

Fix it by simply not resetting the y position. This Resolves #594.

[mwilck]

@matthijskooijman, please help me with the failed "code linting" test. I made just trivial modifications. It seems that "flake8" can't be run at all. I've no clue about "flake8".

[markferry]

src/hamster/widgets/facttree.py:50:4: E111 indentation is not a multiple of four
src/hamster/widgets/facttree.py:53:66: W291 trailing whitespace
src/hamster/widgets/facttree.py:58:4: E111 indentation is not a multiple of four
src/hamster/widgets/facttree.py:62:4: E111 indentation is not a multiple of four
src/hamster/widgets/facttree.py:63:4: E111 indentation is not a multiple of four
src/hamster/widgets/facttree.py:512:58: W291 trailing whitespace

So legacy whitespace fixes.

[mwilck]

ups, what happened there ... ? Checking (edit: I was irritated by some artefact in my editor)

[mwilck]

src/hamster/widgets/facttree.py:50:4: E111 indentation is not a multiple of four
src/hamster/widgets/facttree.py:53:66: W291 trailing whitespace
...

That's what I meant, my patch didn't touch any of these lines.

[markferry]

Want to pull my https://github.com/markferry/hamster/tree/issue594?

[mwilck]

Want to pull my https://github.com/markferry/hamster/tree/issue594?

The two commits (one for fixing the issue, one for fixing the whitespace stuff) are well separated, so why not. Please send a PR to my issue594 branch.

[matthijskooijman]

Seems that flake8 was already broken in master: https://github.com/projecthamster/hamster/runs/717792799, it seems #596 broke it (I suspect that PR predates the automatic checking, so it probably did not show the failure).

@markferry, care to submit the flake8 fixes as a separate PR? I looked through the commit and it looks good, so I think we could just merge that right away.

[markferry]

@matthijskooijman #624

[matthijskooijman]

@mwilck, I merged the flake8 fixes from @markferry, could you rebase this PR on top of master (should leave just your commit in the PR).

I had a look at the commit itself, which seems simple enough, but I'm not quite sure how this stuff works. Maybe you already figured this out and can help me understand how this self.vadjustment is used exactly (in particular, is it connected to some widget or other way that makes it change value? I couldn't find where directly)? Also, is self.y special (e.g. defined/used by some gtk superclass or so), or just a simple attribute? How do both of these interact?

[mwilck]

I had a look at the commit itself, which seems simple enough, but I'm not quite sure how this stuff works. Maybe you already figured this out and can help me understand how this self.vadjustment is used exactly (in particular, is it connected to some widget or other way that makes it change value? I couldn't find where directly)? Also, is self.y special (e.g. defined/used by some gtk superclass or so), or just a simple attribute? How do both of these interact?

I figured it out so-so, by trial and error and debugging. I don't claim to understand every little detail.

self.y is a simple attribute, defined in the FactTree class itself. vadjustment is a Gtk adjustment Object, a helper class which can be used to make sure that the position always remains within reasonable limits, and is changed in reasonable steps.

It's obvious that the effect described i #594 would occur if we reset the y position once a minute. It's less obvious whether not doing so would break anything. I've tried to test test it and break it after my change, but for me it has worked well. I'd appreciate feedback from those who have reported #594. The code I changed is very old, it has existed basically since day 1 AFAICT.

[matthijskooijman]

Thanks. Looking more closely, I see that vadjustment and hadjustment are properties of the gtk.Scrollable superclass, so they are overridden by FactTree, but actually used by Scrollable. That is, when the user scrolls, I think the vadjustment value is modified, and on_scroll_value_changed is called, which updates self.y and then calls on_scroll.

Also, scrolling with pgup/pgdown updates self.y. Mouse scrolling calls on_scroll directly without updating self.y first, but on_scroll handles that case specially by changing the position and updating self.y (this could use a refactor at some point). Some other cases related to (de)selecting a fact also call on_scroll, thought I can't see how on_scroll uses the selected fact in any way.

Armed with all this knowledge, I wonder:

1. Is there value in resetting the scrolling when the FactTree is shown for the first time? Probably not, since self.y is already initialized at 0.
2. Is there value in resetting the scrolling when changing the facts? AFAIU with your PR if you're scrolled down a bit and then change to the e.g. the previous month, the scrolling is not reset. However, resetting the scroll would make sense here, I think. To solve this, I guess set_facts would need an extra parameter to distinguish between the "change to different facts view" or "the facts in the current view changed" (which I think could also mean a fact got added or removed, e.g. when starting a new activity).
3. If the scroll is not reset, is the self.vadjustment.set_value(self.y) still needed? If the scroll did not change, isn't the vadjustment still ok as well? I can imagine this is helpful to clamp the self.y value (e.g. when the facts list is smaller and self.y is now out of bounds), but then I wonder if self.vadjustment.value_changed() does not also need to be called to let the new value be processed? Also, this would only be useful when called after the limits of vadjustment have been updated, but that only happens later (in set_row_heights()). Finally, it seems that on_scroll already takes care of clamping, so this might not be needed at all.

A broader question I have is why self.y is needed at all, I would think the vadjustment object is intended to be the canonical storage of the scroll value. I suspect some evolutionary reasons, but it is probably too much to refactor the scroll handling completely here.

[mwilck]

1. Is there value in resetting the scrolling when the FactTree is shown for the first time? Probably not, since `self.y` is already initialized at 0.

Exactly.

2. Is there value in resetting the scrolling when changing the facts? AFAIU with your PR if you're scrolled down a bit and then change to the e.g. the previous month, the scrolling is _not_ reset. However, resetting the scroll would make sense here, I think.

It happens today if the new view had less rows than necessary to fill the screen (unlikely with months, but well reproducible by using days and possibly shrinking the window a bit).

But I fail to see why a reset to 0 would be mandated in the general case, where both before and after the switch there are more lines than can be displayed at one time. In general, when we switch to en entirely new view, we have zero knowlegde what the user is looking for. The user will have switched the view and will be prepared to scroll on way or the other. I see no particular reason why keeping the current position (as my patch does) would be any worse than resetting to 0. Actually, there'd be good reasons to use a different default, e.g. "50%". The only case where IMO a reset to 0 would be the natural thing to do is when the user switches from one time frame of a certain type (month, say) to the following time frame of the same type (i.e. May 2020 -> June 2020).

To solve this, I guess set_facts would need an extra parameter to distinguish between the "change to different facts view" or "the facts in the current view changed" (which I think could also mean a fact got added or removed, e.g. when starting a new activity).

Do what you like, I'd prefer to keep it simple for now.

3. If the scroll is _not_ reset, is the `self.vadjustment.set_value(self.y)` still needed?

The first thing I tried was to simply skip the self.y = 0 statement. Be assured that that is not sufficient to fix the issue. self.vadjustment.set_value(self.y) is necessary, definitely.

A broader question I have is why self.y is needed at all, I would think the vadjustment object is intended to be the canonical storage of the scroll value. I suspect some evolutionary reasons, but it is probably too much to refactor the scroll handling completely here.

Valid question, but again I'd like to keep things simple for the time being. I just provided a simple fix for the reported problem. If you have a better patch, go ahead :-) Otherwise, please play around with this patch a bit, and judge the user experience yourself.

[matthijskooijman]

The only case where IMO a reset to 0 would be the natural thing to do is when the user switches from one time frame of a certain type (month, say) to the following time frame of the same type (i.e. May 2020 -> June 2020).

Yeah, that's indeed the case I was thinking about. But even there, I guess that not resetting is also reasonable, and never resetting is indeed significantly simpler than supporting two cases.

The first thing I tried was to simply skip the self.y = 0 statement. Be assured that that is not sufficient to fix the issue. self.vadjustment.set_value(self.y) is necessary, definitely.

Hm, any idea why? Would be nice to understand this better and maybe add a comment with reasoning for this. OTOH, if it works, I guess that having this line without a comment is not terrible, as the scrolling code is a bit messy and underdocumented already anyway.

In summary, I agree that the current patch is actually ok. I'll see if I can test it a bit as well, though I still haven't set up an easy way to build git versions (I'm using my WIP .deb package rather than manual installs, which should be able to do git builds, but I haven't entirely figured out how). I'll report back when I get this working.

[mwilck]

I still haven't set up an easy way to build git versions (I'm using my WIP .deb package rather than manual installs, which should be able to do git builds, but I haven't entirely figured out how).

Me neither. I keep forgetting how to fix the gsettings issue... Want to know how I created this patch? I hand-edited the installed .py file 😁 (with a patch of this size, that still works).

[GeraldJansen]

To get around that gsettings issue, I have used this script to run git hamster from the source tree:

#!/bin/sh
cd /d/gj/hamster # <== git clone directory: adjust or remove
[ -f build/data/gschemas.compiled ] || {
  mkdir -p build/data
  glib-compile-schemas --targetdir=build/data data
}
export GSETTINGS_SCHEMA_DIR=build/data
pkill -ef hamster.*service 2>&1 >/dev/null
python3 src/hamster-service.py &
python3 src/hamster-cli.py $*

[mwilck]

To get around that gsettings issue, I have used this script to run git hamster from the source tree:
...

This is nice. We should add it to the repo. I'd drop the cd on top, but otherwise it's exactly what we need.

[GeraldJansen]

Okay, I added the script to get around the gsettings issue at PR #625. Withdrawn.

[rhertzog]

I submitted a new PR (#648) because I believe that we should reset the position when we display a new set of facts.

I believe this PR can be closed. I'm also really annoyed by this behavior so I hope that with this supplementary change it can be merged quickly. I haven't seen any downside in my (limited) testing so far.

[matthijskooijman]

Closing this in favor of #648.